Skip to content

Conversation

@amartya4256
Copy link
Contributor

@amartya4256 amartya4256 commented Mar 18, 2025

On Win32, the OS is responsible for painting the Image of a MenuItem and it expects images to be in metrics specific size specified by SM_CYMENUCHECK or SM_CXMENUCHECK. If the images are not provided within these sizes, Windows tries to rescale them, leading to unexpected sizes and masking.
This PR contributes to scaling of the MenuItem::hBitmap with the DPI Aware System Metrics for the size of the SM_CYMENUCHECK since the expected size of the Context menu images can differ from the size specific to the zoom level of the monitor.
This PR also addresses the internal rounding error in the size obtained by the systemMetrics while scaling across different zooms (in case of fractional scaling factor) - The reasoning to that has been added as a comment block in the code.

contributes to
#62 and
#127

Depends on: #1933

Bug Description

  • Start the IDE at a 125% zoom monitor (175, 225, 275, etc also work)
  • Create a context 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 of the metrics specific size.
  • There should not be any black background on disabled images and the image must be scaled to exepected sizes.
    image

contributes to
#62 and #127

@github-actions
Copy link
Contributor

github-actions bot commented Mar 18, 2025

Test Results

   545 files  + 6     545 suites  +6   35m 33s ⏱️ + 5m 34s
 4 367 tests +37   4 355 ✅ +35   12 💤 +3  0 ❌  - 1 
16 616 runs  +37  16 512 ✅ +35  104 💤 +3  0 ❌  - 1 

Results for commit 4e9c96e. ± Comparison against base commit 22e8829.

♻️ This comment has been updated with latest results.

Copy link
Member

@fedejeanne fedejeanne left a comment

Choose a reason for hiding this comment

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

The changes in the 2nd commit make sense to me, I would just like to wait until I get clarification on the 1st commit (#1913) to proceed with a deeper review.

@amartya4256 amartya4256 marked this pull request as draft March 21, 2025 11:38
@amartya4256
Copy link
Contributor Author

Converted to draft because for some inconsistencies in the Windows behaviour.

@amartya4256 amartya4256 force-pushed the amartya4256/menuItem_image_scaling_with_metrics branch 6 times, most recently from 4b49151 to 142c502 Compare March 24, 2025 15:59
@amartya4256 amartya4256 marked this pull request as ready for review March 24, 2025 16:02
@amartya4256 amartya4256 force-pushed the amartya4256/menuItem_image_scaling_with_metrics branch 2 times, most recently from 0d71b69 to 8a07b4c Compare March 26, 2025 10:31
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.

Unfortunately, this does not work for me. Even in a single-monitor scenario, this breaks. Take a monitor at 175% zoom, start an application (now the context menu is fine). Then change monitor zoom to 125%, then the context menu looks like this:
image

@amartya4256
Copy link
Contributor Author

@HeikoKlare Yes you are right. It is my bad - I can as well reproduce it. I did not test the scenario for quarter primary monitor zoom at startup with quarter zoom at scaling. Of course there can also be a rounding error because of fractional scaling factor. Let me readapt the algorithm. Would be nice to test primary monitor startup zooms of all type (full, half and quarter) with that of all type of zoom while scaling.

@amartya4256 amartya4256 force-pushed the amartya4256/menuItem_image_scaling_with_metrics branch from 8a07b4c to 84564cf Compare March 26, 2025 13:21
@amartya4256
Copy link
Contributor Author

@HeikoKlare There was a missing case for quarter to quarter rounding correction. I have added it and tested for 175 -> 125, 125 -> 175, 125 -> 225, 225 -> 125. Can you please test it out on your monitors as well? and please test out the other scenarios as well to make sure if every condition holds for your monitor setup ad well.

@HeikoKlare
Copy link
Contributor

Thank you for the quick adaptation! I will have a look and test.

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.

Sorry, but this is still not working. Simple two monitor setup: primary 100%, secondary 200%. On 100% everything is fine, on 200% it looks like this:
image

@amartya4256 amartya4256 force-pushed the amartya4256/menuItem_image_scaling_with_metrics branch from 84564cf to 80e0ccc Compare March 27, 2025 14:26
This commit contributes to scaling of the MenuItem::hBitmap with the DPI
-aware System Metrics for the size of the SM_CYMENUCHECK or the size
obtained by menuItem zoom. It also corrects the zoom in specific cases
where the images are not consistently drawn on the context menu.

Contributes to
eclipse-platform#62 and
eclipse-platform#127
@HeikoKlare HeikoKlare force-pushed the amartya4256/menuItem_image_scaling_with_metrics branch from 80e0ccc to 4e9c96e Compare March 27, 2025 14:58
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.

I heavily tested around with the current proposal in all kinds of different configurations and changes to it. I did not find any issues with the scaling of menus anymore.

So to get even better feedback by continuous testing, I will merge this now to also have it in M1. In case we find remaining issues, we can even improve the algorithm.

@HeikoKlare HeikoKlare merged commit 4325d7b into eclipse-platform:master Mar 27, 2025
15 checks passed
@HeikoKlare HeikoKlare deleted the amartya4256/menuItem_image_scaling_with_metrics branch March 27, 2025 18:19
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.

Menu items improperly sized when zoom changes Inactive menu items are sometimes black

3 participants