Skip to content
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.

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

feature: add _BaseShapes.iter_leaf_shapes() #435

Open
scanny opened this issue Sep 15, 2018 · 0 comments
Open

feature: add _BaseShapes.iter_leaf_shapes() #435

scanny opened this issue Sep 15, 2018 · 0 comments

Comments

@scanny
Copy link
Owner

scanny commented Sep 15, 2018

See eQualityTime/TheOpenVoiceFactory#101 for context.


Hi Joe, I've had a serious think about this and I'm inclined to conclude that the present behavior is the best compromise between competing design interests.

What I do think would improve matters is to add a _BaseShapes.iter_leaf_shapes() method, which would be inherted by SlideShapes, the object present on Slide.shapes. There's an implementation just below you can use without waiting for me to get around to adding it in.

Let me know what you think.

Here's my rationale in more detail:
The unfortunate aspect of the situation is that code that deals with shape click actions (hyperlinked shapes basically) may need to know that group shapes can't have a click action and have code to guard against attempting access to shape.click_action on group shapes.

Here's why I think trying to remove that inconvenience would degrade the design more than improve it:

If you happen to know there are no group shapes, perhaps because you just wrote the slide, then you can remain blissfully ignorant of this situation. I expect this accounts for 99.9% of use cases when folks who don't do anything with shape hyperlinks are factored in.

If you are dealing with "from the wild" presentations, you need to be dealing with group shapes explicitly anyway, lest you only consider top-level shapes and miss those otherwise "hidden" inside groups.

In this case, to iterate over all hyperlinkable shapes on a slide, you need something like this:

from pptx.enum.shapes import MSO_SHAPE_TYPE

def iter_leaf_shapes(shapes):
for shape in shapes:
if shape.shape_type == MSO_SHAPE_TYPE.GROUP:
for s in iter_leaf_shapes(shape.shapes):
yield s
else:
yield shape

for shape in iter_leaf_shapes(slide.shapes):
process_shape_I_know_for_sure_isnt_a_group_shape()
Using a shape-tree iterator like this, you naturally don't get group shapes presenting themselves to be processed.

The alternative would make the get case more streamlined, but would just kick the can down the road for the set case. I've outlined that option at the end here just for the record.

Option I don't like but might be worth preserving for the record
Ease iteration of shape sequence click actions by having shape.click_action always return an ActionSettings-like object, so click_action = shape.click_action would never raise or return None, regardless of the shape.

This would be accomplished by a new NullActionSettings object (maybe with a better name), that has the same interface as today's ActionSettings object, but with the following behaviors:

NullActionSettings.action unconditionally returns PP_ACTION.NONE
NullActionSettings.hyperlink returns a NullHyperlink object.
NullActionSettings.target_slide unconditionally returns None.
Assignment to NullActionSettings.target_slide raises something like the same error you're seeing now, e.g. "Group Shape cannot have click action".
NullHyperlink.address unconditionally returns None.
Assignment to NullHyperlink.address raises something like "shape of type GroupShape cannot have a hyperlink".
This would support a protocol something like this:

for shape in shapes:
if shape.click_action.action == PP_ACTION_NONE:
continue
# ... process click action ...
While this NullActionSettings approach might ease the get side of the equation, you would still face the same "need to treat group shapes as special case" challenge when attempting to set a click-action on an unknown shape.

Regarding the option of returning None for GroupShape.click_action, I would consider that only slightly less onerous than the current behavior for get.

Instead of this:

for shape in shapes:
try:
shape.click_action
except TypeError:
continue
if shape.click_action.action == PP_ACTION_NONE:
continue
# ... process click action ...
You still have two steps, one to skip over group shapes (the only
type that would return None) and one to skip over shapes that can but don't
have a click action defined:

for shape in shapes:
if shape.click_action is None:
continue
if shape.click_action.action == PP_ACTION_NONE:
continue
# ... process click action ...
And it would do nothing for the set side, which would result in None object has no attribute 'action' on attempts to set an action on a group shape.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant