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

Fix slide layout out of order #94

Closed

Conversation

kenliau
Copy link
Contributor

@kenliau kenliau commented Mar 8, 2015

This PR fixes the issue where wrong slide layout was being used to generate powerpoint.
The root cause was that, in slideMaster1.xml.rels, it was assuming all the slideLayouts are in the same order as the actual slides in a powerpoint. But in reality, the slideLayouts were not in sorted order causing the generation to use a different layout.

I have narrow down to 2 different solutions and submitting two different PR as there are pros and cons in both.

This PR is Solution#1.

Solution
  1. KSort AbstractLayoutPack $layout so we can still findLayoutIndex and use ($layoutIndex + 1) in Rels.php to generate slides.
    • Pro: 1 line of change.
    • Con: Ksorting the list of slide layouts might affect performance if we have large number of slide layouts.
  2. Do not sort $layout, but refactor findLayoutIndex to findLayoutId in AbstractLayoutPack.php. Then we can replace findLayoutIndex by findLayoutId and use ($layoutId) instead of ($layoutIndex + 1) in Rels.php to generate slides.
    • Pro: Improves readability as we use layoutId instead of layoutIndex.
    • Con: Couple line of changes in 2 different files.

Again, this PR is Solution#1. I have also added unit tests for my changes. Unfortunately, I am not able to completely implement testConstruct for TemplateBasedTest.php

Solution#2 PR is here.
Please critique and provide feedback as this is my very first PR to the open source community.

@Progi1984
Copy link
Member

I prefer the #95, which brings more flexibility.

@Progi1984 Progi1984 closed this May 21, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants