MenuItem Image Scaling with DPI aware metrics #62 #1914
Conversation
fedejeanne
left a comment
fedejeanne
left a comment
There was a problem hiding this comment.
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.
|
Converted to draft because for some inconsistencies in the Windows behaviour. |
amartya4256
force-pushed
the
amartya4256/menuItem_image_scaling_with_metrics
branch
6 times, most recently
from
4b49151 to
142c502
Compare
amartya4256
force-pushed
the
amartya4256/menuItem_image_scaling_with_metrics
branch
2 times, most recently
from
0d71b69 to
8a07b4c
Compare
HeikoKlare
left a comment
HeikoKlare
left a comment
There was a problem hiding this comment.
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:
|
@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
force-pushed
the
amartya4256/menuItem_image_scaling_with_metrics
branch
from
8a07b4c to
84564cf
Compare
|
@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. |
|
Thank you for the quick adaptation! I will have a look and test. |
HeikoKlare
left a comment
HeikoKlare
left a comment
There was a problem hiding this comment.
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:
amartya4256
force-pushed
the
amartya4256/menuItem_image_scaling_with_metrics
branch
from
84564cf to
80e0ccc
Compare
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
force-pushed
the
amartya4256/menuItem_image_scaling_with_metrics
branch
from
80e0ccc to
4e9c96e
Compare
HeikoKlare
left a comment
HeikoKlare
left a comment
There was a problem hiding this comment.
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.
3 participants
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
How to test
contributes to
#62 and #127