Skip to content

implementing replace_picture() #6

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

Closed
m-entrup wants to merge 16 commits into from
Closed

implementing replace_picture() #6

Changes from 3 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
31ea782
Implementing replace_picture():
m-entrup Oct 12, 2019
8ac31cd
Adding playground code for adding an image shape:
m-entrup Oct 12, 2019
4d8f199
Merge remote-tracking branch 'upstream/master'
m-entrup Oct 12, 2019
37e9b1b
Implementing replace_picture():
m-entrup Oct 12, 2019
ef48d76
Merge remote-tracking branch 'upstream/master'
m-entrup Oct 12, 2019
032e65a
Ignoring pipenv and PyCharm related files:
m-entrup Oct 12, 2019
ffda58f
Merge remote-tracking branch 'upstream/master'
m-entrup Oct 12, 2019
e8e043e
Fix type specification
timhoffm Oct 12, 2019
7abbeb2
First test
Oct 12, 2019
6576e08
Ignoring pipenv and PyCharm related files:
m-entrup Oct 12, 2019
9ec849e
Merge remote-tracking branch 'origin/master'
m-entrup Oct 12, 2019
a8a17e4
Enhancing testing_1.py:
m-entrup Oct 12, 2019
e629891
Merge remote-tracking branch 'upstream/master'
m-entrup Oct 12, 2019
e8cdc97
Adding a test for image scaling.
m-entrup Oct 12, 2019
d316fd3
Extending `replace_picture()` to scale images:
m-entrup Oct 12, 2019
d0a5208
Merge remote-tracking branch 'upstream/master'
m-entrup Oct 12, 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
22 changes: 21 additions & 1 deletion pptx_blueprint/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,27 @@ def replace_picture(self, label: str, filename: _Pathlike) -> None:
label (str): label of the placeholder (without curly braces)
filename (path-like): path to an image file
"""
pass
shapes_to_replace = self._find_shapes(label)
if not shapes_to_replace:
Copy link

@cmahr cmahr Oct 12, 2019

Choose a reason for hiding this comment

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

This should be an Exception. Don't let errors pass silently!

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.

It is a good idea to handle the problem on a higher level. I will replace return by

raise ValueError(f"The label '{label}' can't be found in the template.")

return
if isinstance(filename, str):
filename = pathlib.Path(filename)
if not filename.is_file():
raise FileNotFoundError(f"The file does not exist: {filename}")
img_file = open(filename, "rb")
Copy link
Collaborator

@lysnikolaou lysnikolaou Oct 12, 2019

Choose a reason for hiding this comment

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

Suggested change
img_file = open(filename, "rb")
with open(filename, "rb") as img_file:

old_shape: BaseShape
Copy link
Collaborator

@lysnikolaou lysnikolaou Oct 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I would maybe type shapes_to_replace, instead of old_shape.

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.

I already use shapes_to_replace for the iterator that is returned by _find_shapes(). In my opinion the singular version shape_to_replace is to close to shapes_to_replace and therefor old_shape makes it easier to distinguish between the iterator and the single shape.

for old_shape in shapes_to_replace:
slide_shapes = old_shape._parent
Copy link

@cmahr cmahr Oct 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Not a big fan of using the internals of the pptx library. But if there is really no better way...

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.

It seems there is currently no other solution:
scanny/python-pptx#246

Could be mentionend in a code comment.

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.

Thx for linking the resource.

slide_shapes.add_picture(
image_file=img_file,
left=old_shape.left,
top=old_shape.top,
width=old_shape.width,
height=old_shape.height
)
Copy link
Contributor

@michaelosthege michaelosthege Oct 12, 2019

Choose a reason for hiding this comment

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

Should this be a with open() …?
Or is that not required when only read access is performed?

# 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)
Copy link

@cmahr cmahr Oct 12, 2019

Choose a reason for hiding this comment

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

Yikes! We should really not deal with xml ourselves. We should open a RP in pptx to get this feature implemented. For the time being, we should either 1) isolate this in a private method marked deprecated to prevent excessive use, or 2) decorate the pptx presentation object.


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