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.

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

Jump to bottom

#1209 improve text sync #1766

Merged
merged 10 commits into from Feb 24, 2019
Merged

#1209 improve text sync #1766

merged 10 commits into from Feb 24, 2019

Conversation

YuPeiHenry
Copy link
Contributor

@YuPeiHenry YuPeiHenry commented Feb 2, 2019 •
edited

Fixes #1209

Ready for review (with tests). Tested and working on PPT2016. Does not support strike through as it is suggested as low value.

Outline of Solution
Text alignment: Used TextFrame's Horizontal and Vertical Anchor.
Text orientation: Used TextFrame's orientation.
Line spacing: Used TextFrame.TextRange.ParagraphFormat's SpaceAfter, SpaceBefore and SpaceWithin.

ChesterSng
ChesterSng reviewed Feb 12, 2019
View reviewed changes
Copy link
Contributor

@ChesterSng ChesterSng left a comment •
edited

Choose a reason for hiding this comment

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

Hi @YuPeiHenry your new unit tests work on powerpoint 2013.

However there are some problems on manual testing.

Horizontal text alignment does not sync:

  1. Copy and select horizontal text alignment
  2. Create new textbox and apply format

Result: Nothing happens

On debug mode, the formatShape.TextFrame2.HorizontalAnchor returns msoAnchorNone.

The attached screenshots show every other format being synced but not horizontal alignment.
before syncing
after syncing

@YuPeiHenry
Copy link
Contributor Author

@ChesterSng turns out there was a bug in horizontal text alignment. I have fixed it, please look through when convenient.

@ChesterSng
Copy link
Contributor

@YuPeiHenry Tested on PowerPoint 2016, working as expected now

@alexfjw
Copy link
Member

alexfjw commented Feb 24, 2019 •
edited

Could someone test this on 2013 as well?
Unfortunately, SyncLab is one of labs which exhibits different behavior on different PPT versions.
2013 in particular, is a pretty consistent culprit.

@ChesterSng
Copy link
Contributor

No problem on 2013 👍

alexfjw
alexfjw requested changes Feb 24, 2019
View reviewed changes
Copy link
Member

@alexfjw alexfjw left a comment

Choose a reason for hiding this comment

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

Code LGTM. Just need that merge conflict fixed.

@YuPeiHenry
Merge branch 'dev-release' of https://github.com/PowerPointLabs/Power…
5751b3a 
…PointLabs into 1209-improve-text-sync

# Conflicts:
#	PowerPointLabs/PowerPointLabs/PowerPointLabs.csproj
@YuPeiHenry
Copy link
Contributor Author

@alexfjw I have resolved the merge conflict.

@alexfjw alexfjw merged commit 4a4f324 into PowerPointLabs:dev-release Feb 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChesterSng ChesterSng ChesterSng left review comments

@alexfjw alexfjw alexfjw approved these changes

Assignees
No one assigned
Labels
ok-2010 ok-2013 ok-2016 s.ToReview
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@YuPeiHenry @ChesterSng @alexfjw