Extending replace_picture() to scale images:#9
Extending replace_picture() to scale images:#9m-entrup wants to merge 5 commits intotimhoffm:masterfrom
replace_picture() to scale images:#9Conversation
Large images are always scaled. The aspect ratio is kept. The new kwarg `do_not_scale_up`can disable upscaling. Centering is always active.
Raising an error if `shapes_to_replace` is empty. Wrapping `open()`into a `with` statement.
pptx_blueprint/__init__.py
Outdated
| shape.text = new_text | ||
|
|
||
| def replace_picture(self, label: str, filename: _Pathlike) -> None: | ||
| def replace_picture(self, label: str, filename: _Pathlike, *, do_not_scale_up: bool = False) -> None: |
There was a problem hiding this comment.
What is the motivation / usecase szenario for do_not_scale_up?
I would want the following behaviors in the future (might be in a future PR).
scale:
All image scaling preseves the aspect-ratio of the original image.
- 'fit-width': Scale the image to the width of the placeholder.
- 'fit-height': Scale the image to the height of the placeholder.
- 'fit': Scale the image to fit inside the placeholder.
Note, that do_not_scale_up and scale are sort of orthogonal.
Naming: One might want to use scale_policy instead of do_not_scale_up; with values "shrink-only", "enlarge-only", None.
scale_policy
There was a problem hiding this comment.
With my implementation the image does always fit into the size of the replaced shape. By calculating the aspect ratio I am able to defect if height or width is the limiting dimension. This is the same as scale='fit'.
I don't like the idea of adding the options 'fit-width' and 'fit-height'. Allowing the image to get larger than the placeholder will result in overlapping with other shapes.
The scale_up=False is useful if an image is only a little to small. Scaling it up by a view percent will reduce the image quality. By preventing up scaling we will get the best image quality.
There was a problem hiding this comment.
I don't like the idea of adding the options 'fit-width' and 'fit-height'. Allowing the image to get larger than the placeholder will result in overlapping with other shapes.
Not saying that they should be default. But we shouldn't assume too much on the user's intention. I can imagine situations in which you'd really want to force the width/height - no matter the cost.
I agree that preventing upscaling might be desirable. Similarly, somebody might want to prevent downscaling since it reduces the information in the image.
Splitting up the label first using `_parse_label()`. Using the results as inputs for `_find_shapes()`.
`do_not_scale_up=False`is replaced by `scale_up=True`. Code using the kwarg is changed to use the negation. The doc string is extended to explain the `replace_image()` better.
`open()` can handle path-like objects. We do not need to convert `str` to `Path`. If a file does not exist òpen()` raises a meaningful exception. It is not necessary to raise a `FileNotFoundError` manually.
|
|
||
| def replace_picture(self, label: str, filename: _Pathlike) -> None: | ||
| def replace_picture(self, label: str, filename: _Pathlike, *, scale_up: bool = True) -> None: | ||
| """Replaces rectangle placeholders on one or many slides. |
There was a problem hiding this comment.
Minor rewording please: ... on one or many slides with an image.
| """Replaces rectangle placeholders on one or many slides. | ||
|
|
||
| The aspect ratio of the image is not changed. | ||
| To large images are always resized. |
| left=old_shape.left, | ||
| top=old_shape.top, | ||
| ) | ||
| # Scaling the image if `scale_up == True`: |
There was a problem hiding this comment.
According to the DocString, images too large for the old_shape are resized to fit into old_shape. This feature is still missing!
There was a problem hiding this comment.
I really think that (at least) scaling the image should be delegated to a private method to improve readability.
| # Centering the image at the extent of the placeholder: | ||
| img_shape.top += int((old_shape.height - img_shape.height) / 2) | ||
| img_shape.left += int((old_shape.width - img_shape.width) / 2) | ||
| del slide_shapes[slide_shapes.index(old_shape)] |
There was a problem hiding this comment.
For me running on Python 3.6.9 (installed via pyenv, test suite is OK), the deletion is not working:
Traceback (most recent call last):
File "/home/carsten/.PyCharmCE2019.2/config/scratches/scratch.py", line 7, in <module>
tpl.replace_picture('*:logo', '/home/carsten/Downloads/beer.jpg')
File "/home/carsten/Documents/Python/pptx-blueprint/pptx_blueprint/__init__.py", line 75, in replace_picture
del slide_shapes[slide_shapes.index(old_shape)]
TypeError: 'SlideShapes' object doesn't support item deletion
| if img_shape.height <= old_shape.height and img_shape.width <= old_shape.width and scale_up: | ||
| old_aspect_ratio = old_shape.width / old_shape.height | ||
| new_aspect_ratio = img_shape.width / img_shape.height | ||
| if old_aspect_ratio >= new_aspect_ratio: |
There was a problem hiding this comment.
Using an image in portrait mode, the width of the image is set to the width of old_shape, which results in an overflow in y direction. This logic seems broken?
There was a problem hiding this comment.
Vice versa, an image in landscape mode overflows in x direction. Replacing >= with < hence fixes this issue.
5 participants
Large images are always scaled.
The aspect ratio is kept.
The new kwarg
do_not_scale_upcan disable upscaling.Centering is always active.