Skip to content

Extending replace_picture() to scale images: #9

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
m-entrup wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Extending replace_picture() to scale images: #9

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e995f82
Extending `replace_picture()` to scale images:
m-entrup Oct 12, 2019
948ab46
Changes according to the review of PR #6:
m-entrup Oct 13, 2019
1a3aca6
Change usage of _find_shapes() at replace_image():
m-entrup Oct 13, 2019
badd26f
Changing the kwarg `do_not_scale_up`:
m-entrup Oct 13, 2019
2fe3714
Using the functionality build into `open()`:
m-entrup Oct 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 35 additions & 2 deletions pptx_blueprint/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,47 @@ def replace_text(self, label: str, new_text: str) -> None:
for shape in shapes:
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:
Copy link
Owner

@timhoffm timhoffm Oct 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@m-entrup m-entrup Oct 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

@timhoffm timhoffm Oct 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"""Replaces rectangle placeholders on one or many slides.
Copy link

@cmahr cmahr Oct 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor rewording please: ... on one or many slides with an image.


Args:
label (str): label of the placeholder (without curly braces)
filename (path-like): path to an image file
do_not_scale_up (bool): deactivates that the image is enlarged (default: False)
"""
pass
shapes_to_replace = self._find_shapes(label)
if not shapes_to_replace:
raise ValueError(f"The label '{label}' can't be found in the template.")
if isinstance(filename, str):
filename = pathlib.Path(filename)
if not filename.is_file():
raise FileNotFoundError(f"The file does not exist: {filename}")
with open(filename, "rb") as img_file:
old_shape: BaseShape
for old_shape in shapes_to_replace:
slide_shapes = old_shape._parent
img_shape = slide_shapes.add_picture(
image_file=img_file,
left=old_shape.left,
top=old_shape.top,
)
# Scaling the image if `do_not_scale == False`:
if img_shape.height <= old_shape.height and img_shape.width <= old_shape.width and not do_not_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:
Copy link

@cmahr cmahr Oct 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

@cmahr cmahr Oct 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vice versa, an image in landscape mode overflows in x direction. Replacing >= with < hence fixes this issue.

img_shape.width = old_shape.width
img_shape.height = int(img_shape.width / new_aspect_ratio)
else:
img_shape.height = old_shape.height
img_shape.width = int(img_shape.height * new_aspect_ratio)
# 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)]
Copy link

@cmahr cmahr Oct 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

# Removing shapes is performed at the lxml level.
# The `element` attribute contains an instance of `lxml.etree._Element`.
slide_shapes.element.remove(old_shape.element)

def replace_table(self, label: str, data) -> None:
"""Replaces rectangle placeholders on one or many slides.
Expand Down