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

Add layout support via GridLayoutBase #58

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

DrChainsaw
Copy link

This adds support for layouting slides using GridLayoutBase as suggested in #48 with the big disclaimer that I don't think I'm even close to groking everything the package is capable of.

Fwiw, I asked if using GridLayoutBase outside of Makie here and got a reply from the maintainer.

I have tried to make as few changes as possible to existing codebase to make reviewing easier for now.

The API is basically identical to the suggestion in #48 (comment).

Example
 sl = ShapeLayout(Slide(;title="Layout Test"))

 sl[1,1] = TextBox("Upper left")
 sl[2,2] = TextBox("Lower right")
 sl12 = sl[1,2] = ShapeLayout(sl)
 sl12[1,1] = TextBox("Upper right => Upper left"; size_x=80)
 sl12[1,2] = TextBox("Upper right => Upper right"; size_x=80)
 sl12[2,1] = TextBox("Upper right => Lower left"; size_x=80)
 sl12[2,2] = TextBox("Upper right => Lower right"; size_x=80)

 sl21 = sl[2,1] = ShapeLayout(sl)
 sl21[1,1] = TextBox("Lower left => Upper left"; size_x=80)
 sl21[1,2] = TextBox("Lower left => Upper right"; size_x=80)
 sl21[2,1] = TextBox("Lower left => Middle left"; size_x=80)
 sl21[2,2] = TextBox("Lower left => Middle right"; size_x=80)
 sl21[3,1] = TextBox("To be replaced and not seen in ppt"; size_x=80)
 sl21[3,1] = TextBox("Lower left => Lower left"; size_x=80)
 sl21[3,2] = TextBox("Lower left => Lower right"; size_x=80)

 pp = Presentation([sl.slide])
 PPTX.write("c:/temp/layouttest.pptx", pp, overwrite = true, open_ppt=true)

Produces the following slide:
image.png

I would have preferred and API where the ShapeLayout does not implicitly modify the Slide and instead was added as just another shape.

The reason this does not fly here is that make_slide assumes that each element of shape is one single unique shape. Rewriting make_xml so it returns a next shape id and an array of arrays with xml would allow for the shapes in Slide.shapes to be a ShapeLayout (which may contain other ShapeLayouts).

I'm also not sure how much users are expected to interact with the GridLayout. The current implementation has some gottchas for such users, mainly that all units are in EMU (just to make things consistent with the rest of PPTX) and that top and bottom use negative values since the y-axis is pointing from top of the slide to the bottom in PPTX while GridLayoutBase assumes the opposite. This is somewhat protected against by wrapping the GridLayout in ShapeLayout so that accessing it becomes accessing package internals.

I realized when I was implementing that PPTX.jl does not seem to have functionality for importing/interpreting the slide layout integer given to power point. If it did, it would be much easier to give reasonable defaults for the ShapeLayout bounding box.

Add layout support for TextBox
@matthijscox-asml
Copy link
Collaborator

Okay this is a nice start, let me try to find time to review this properly so I understand what's going on.

First quick thoughts:

  • We should definitely add some unit tests
  • I see this still needs to be extended to every shape, not just TextBox
  • Main question: based on this small prototype, do you think using GridLayoutBase is the right way to go, or not?

I would have preferred and API where the ShapeLayout does not implicitly modify the Slide and instead was added as just another shape.

I agree this would be nicer from the user perspective. Do we currently have to choose whether we have an unstructured Slide or a ShapeLayout Slide? So something fails if we try to directly push a shape into the Slide object of your example?

I realized when I was implementing that PPTX.jl does not seem to have functionality for importing/interpreting the slide layout integer given to power point. If it did, it would be much easier to give reasonable defaults for the ShapeLayout bounding box.

Hmm, I see. Right now the user needs to set the SlideLayout size for different slide sizes and/or layouts. I guess this might be complicated to automate for every scenario, for example we don't know where the title/subtitle box and footnotes are located. But let's leave all this complexity for a future PR.

@DrChainsaw
Copy link
Author

Quick answers:

We should definitely add some unit tests

Absolutely. I just wanted to get an early picture of what the needed changes where and what the API should look like first.

I see this still needs to be extended to every shape, not just TextBox

At the moment yes, since each shape struct has its own members for size and offsets and those are what is being used to determine the size in the PPT. This could be moved to some common component for better reuse. One could also just create a new instance and replace it whenever the size changes, but I guess that could cause some confusion for users who want to hold on to the Shape objects they create for whatever reason.

Main question: based on this small prototype, do you think using GridLayoutBase is the right way to go, or not?

A little bit too early to say for me since I still don't understand the full scope of GridLayoutBase. In particular I don't understand if a) there is one "right" way to connect all the observables (e.g. in the LayoutObservables) or b) you are supposed to connect them differently depending on the behaviour you want.

One somewhat concerning aspect of the current implementation is that I can't seem to get GridLayoutBase to rescale the sizes of the items in the layout so that they fit the outer (top level) bounding box. I don't know if this is just because I haven't found the right way to connect things or if this is just not something it does. I guess images rendered by Makie will always just be confined to the size of the window in which they are drawn, so perhaps Makie does not need to care about this aspect (although it would be strange if the size argument provided to the Figure constructor is not respected).

I think writing a layout system from scratch seems like a pretty daunting task however and I think the similarity to Makie is a nice bonus.

Do we currently have to choose whether we have an unstructured Slide or a ShapeLayout Slide? So something fails if we try to directly push a shape into the Slide object of your example?

It shouldn't fail, so it should be possible to combine them. It will fail if something added by the ShapeLayout is removed by someone else since it remembers the indexes in the Shape vector.

Agree that handling powerpoints layouts should be easy to plug in later.

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

Successfully merging this pull request may close these issues.

2 participants