Skip to content

MenuItem Image Half Scaling to make sure there's no adverse effect #1872

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
Closed
wants to merge 1 commit into from
Closed

MenuItem Image Half Scaling to make sure there's no adverse effect #1872

wants to merge 1 commit into from

Conversation

amartya4256
Copy link
Contributor

@amartya4256 amartya4256 commented Mar 3, 2025 •
edited
Loading

This commit contributes to enforcing half scaling on the MenuItem Image. On Win32, the OS is responsible for painting the Image of a MenuItem and it expects images to be in standard sizes i.e. 16px, 24 px, 32 px, etc. If the images are not provided within these sizes, Windows tries to rescale them, leading to unexpected sizes and masking. Since, half scaling yields the images in standard sizes, MenuItems are scaled accordingly.

Bug Description

  • Start the IDE at a 125% zoom monitor (175, 225, 275, etc also work)
  • Create a contect menu with a right-click
  • The disabled images have a dark gray/black background
  • The images might appear too small or too big.
    image

How to test

  • Follow the same steps as described above
  • The images should be scaled with half scaling strategy, i.e. 125% -> 100%, 175% -> 200%, 150% -> 150%
  • There should not be any black background on disabled images.
    image

contributes to
#62 and #127

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Mar 3, 2025 •
edited
Loading

Test Results

   506 files  ±0     506 suites  ±0   7m 45s ⏱️ -24s
 4 340 tests ±0   4 326 ✅ ±0   14 💤 ±0  0 ❌ ±0 
16 596 runs  ±0  16 487 ✅ ±0  109 💤 ±0  0 ❌ ±0 

Results for commit 8c1409a. ± Comparison against base commit 5a22598.

♻️ This comment has been updated with latest results.

akoch-yatta
akoch-yatta reviewed Mar 6, 2025
View reviewed changes
Copy link
Contributor

@akoch-yatta akoch-yatta left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Changes look fine to me and solves the issue.

I'm just not sure about the behavior with monitor-specific scaling not active. With this PR the image size is updated in all cases, so on a 150% primary monitor:

old state
image
new state
image
This is obviously a difference before and after. I personally prefer the better sizing, but the scaling leads to slight blurryness.

This was linked to issues Mar 6, 2025
Inactive menu items are sometimes black vi-eclipse/Eclipse-Platform#242
Closed
Menu items improperly sized when zoom changes vi-eclipse/Eclipse-Platform#253
Closed
@HeikoKlare
Copy link
Contributor

This is obviously a difference before and after. I personally prefer the better sizing, but the scaling leads to slight blurryness.

I would be in favor of keeping the existing behavior as is.
Proposal: as said in my other comment, encapsulate the functionality into a specific method in DPIUtil. Then only perform a "correction" of the autoscale mode in case a problematic mode (like quarter or exact) is set but keep it for other modes like the default integer.

@akoch-yatta
Copy link
Contributor

This is obviously a difference before and after. I personally prefer the better sizing, but the scaling leads to slight blurryness.

I would be in favor of keeping the existing behavior as is. Proposal: as said in my other comment, encapsulate the functionality into a specific method in DPIUtil. Then only perform a "correction" of the autoscale mode in case a problematic mode (like quarter or exact) is set but keep it for other modes like the default integer.

Sounds okay to me to shift that logic to DPIUtil

@amartya4256 amartya4256 force-pushed the amartya4256/menu_item_image_quarter_scaling branch from 1d54936 to 644469f Compare March 7, 2025 15:14
@amartya4256
Copy link
Contributor Author

Made the changes as requested.

@amartya4256 amartya4256 force-pushed the amartya4256/menu_item_image_quarter_scaling branch from 644469f to 8c1409a Compare March 7, 2025 15:19
fedejeanne
fedejeanne reviewed Mar 10, 2025
View reviewed changes
bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/internal/DPIUtil.java
@@ -631,7 +631,19 @@ public static boolean useCairoAutoScale() {
return useCairoAutoScale;
}

public static int getZoomForMenuItemImage(int nativeDeviceZoom) {
String autoScaleValueForMenuItemImage = DPIUtil.autoScaleValue;
if(autoScaleValueForMenuItemImage.equals("quarter") || autoScaleValueForMenuItemImage.equals("exact")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this can be specified by the user, you should either use equalsIgnoreCase(...) or transform it using toLowerCase() like it is done in other places

HeikoKlare
HeikoKlare reviewed Mar 10, 2025
View reviewed changes
Copy link
Contributor

@HeikoKlare HeikoKlare left a comment

Choose a reason for hiding this comment

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

This does not work for me. It might fix some background issues but it does not seem to fix icon sizing issues in menus in general.

Setup:

  • 100% primary, 150% secondary monitor
  • Move to 150% monitor and open context menu there

Result: broken with wrong sizes
image

@amartya4256 amartya4256 force-pushed the amartya4256/menu_item_image_quarter_scaling branch from 8c1409a to 1ffa8a7 Compare March 12, 2025 15:46
@HeikoKlare HeikoKlare marked this pull request as draft March 13, 2025 09:57
@amartya4256
Image Rounding Error with smooth scaling eclipse-platform#62
af9dd04 
This commit contributes to fixing the logic for scaling ImageData with
Smooth scaling strategy. The logic of
DPIUtil::autoScaleImageData is replicated in the Image class and
modified to have properly defined current and target zoom. The previous
implementation scales the bounds of image up and down several times
which can lead to rounding error in case of scaling factor being a
fractional value. With this implementation, the obtained imageData has
no rounding error after ruling out those scale ups and downs and hence
improves the menu item icons.

contributes to
eclipse-platform#62 and
eclipse-platform#127
@amartya4256 amartya4256 force-pushed the amartya4256/menu_item_image_quarter_scaling branch from 1ffa8a7 to af9dd04 Compare March 18, 2025 12:25
@amartya4256
Copy link
Contributor Author

Closing this PR as #1913 and #1914 makes it obsolete.

@amartya4256 amartya4256 deleted the amartya4256/menu_item_image_quarter_scaling branch March 18, 2025 15:53
This was unlinked from issues Mar 25, 2025
Inactive menu items are sometimes black vi-eclipse/Eclipse-Platform#242
Closed
Menu items improperly sized when zoom changes vi-eclipse/Eclipse-Platform#253
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HeikoKlare HeikoKlare HeikoKlare left review comments

@fedejeanne fedejeanne fedejeanne left review comments

@akoch-yatta akoch-yatta akoch-yatta left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@amartya4256 @HeikoKlare @akoch-yatta @fedejeanne