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

#1797 Add checkbox to ribbon for user preference to compress images #1800

Merged
merged 6 commits into from Mar 14, 2019
Merged

#1797 Add checkbox to ribbon for user preference to compress images #1800

merged 6 commits into from Mar 14, 2019

Conversation

blewjy
Copy link
Contributor

@blewjy blewjy commented Mar 2, 2019

Fixes #1797

Outline of Solution
Previously, when using Crop to Shape, high resolution images will be compressed after the crop. This is because we were exporting the images at 96ppi. This PR bumps this up to 330ppi to maintain a certain amount of quality on the pictures.

Before:
croptoshape-before
After:
croptoshape-after

There is also a checkbox added to the main ribbon to allow users to decide if they would like images to be compressed. If this is checked, the image exporting resolution will be set back down to the original 96ppi.

Checkbox:
compress-images-checkbox

Note: this PR also affects other labs that use the same exporting and import method, including EffectsLab, thus the FT slides for EffectsLab had to be updated.

@blewjy blewjy added the ok-2013 label Mar 2, 2019
@ChesterSng
Copy link
Contributor

ChesterSng commented Mar 4, 2019

Hi @blewjy, good job on solving the issue! Tested on PowerPoint 2016
Image now retain its resolution after Crop functions.

There is an issue when checking Compress Images:
Compress Image becomes unchecked after switching tabs

  1. Check Compress Images
  2. Go to any other ribbon tab i.e. Home
  3. Go back to PowerPointLabs tab.
  4. Compress Images is now unchecked.
    However, if you do Crop functions, the images are now lower resolution, which means even though it is unchecked, Compress Images is still active.

I think you need to add a getPressed parameter and a function to use ShouldCompressImages like in Maintain Tab Focus.

I also have a suggestion for the tooltip of Compress Images:

People might not understand the what functionalities it affects even though it states image-related functions, not sure if it is important to include a list of features that it will affect for better clarity.

Alternatively, maybe a ? icon can be added to beside the Compress Images option so people who click it can get a more detailed explanation of the option:

image

@blewjy
Copy link
Contributor Author

blewjy commented Mar 7, 2019

Hey @ChesterSng, thanks for your review!

I think you need to add a getPressed parameter and a function to use ShouldCompressImages like in Maintain Tab Focus.

You're right about this, adding the function solved that issue.

People might not understand the what functionalities it affects even though it states image-related functions, not sure if it is important to include a list of features that it will affect for better clarity.
Alternatively, maybe a ? icon can be added to beside the Compress Images option so people
who click it can get a more detailed explanation of the option:

To keep it consistent with the other buttons, I decided to add more detail to the tooltip instead. I listed out the features that were affected by this compress image setting.

ChesterSng
ChesterSng previously approved these changes Mar 8, 2019
Copy link
Contributor

@ChesterSng ChesterSng left a comment

Choose a reason for hiding this comment

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

Hi @blewjy, previous bug is fixed! Code LGTM

@leeyh20
Copy link
Contributor

leeyh20 commented Mar 9, 2019

Was it tested on 2016?

@ChesterSng
Copy link
Contributor

@leeyh20 Yup tested!

Copy link
Contributor

@leeyh20 leeyh20 left a comment

Choose a reason for hiding this comment

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

LGTM except for one part!


if (ShouldCompressImages)
{
GraphicsUtil.PictureExportingRatio = 96.0f / 72.0f;
Copy link
Contributor

Choose a reason for hiding this comment

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

it will be better if these ratio values will be stored in GraphicsUtil as a constant so that if anyone wants to find these values then they will directly go to GraphicsUtil, not Ribbon1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! moved them into GraphicsUtil

YuPeiHenry
YuPeiHenry previously approved these changes Mar 14, 2019
Copy link
Contributor

@YuPeiHenry YuPeiHenry left a comment

Choose a reason for hiding this comment

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

Tested and working on PPT2010.

Copy link
Contributor

@leeyh20 leeyh20 left a comment

Choose a reason for hiding this comment

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

Sorry just one last thing!

// Picture exporting ratios
private const float PictureExportingRatioHigh = 330.0f / 72.0f;
private const float PictureExportingRatioCompressed = 96.0f / 72.0f;

// Heuristics for image compression obtained through testing
private const long targetCompression = 75L;
Copy link
Contributor

Choose a reason for hiding this comment

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

I realised that these variables are lower-case while the above const float is upper-case for the first letter, could you standardise them according to https://github.com/oss-generic/process/blob/master/codingStandards/CodingStandard-CSharp.adoc ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! Changed to camel case for private variabels.

@leeyh20 leeyh20 merged commit 93c4762 into PowerPointLabs:dev-release Mar 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants