search

eclipse-platform / eclipse.platform.swt

Eclipse SWT
https://www.eclipse.org/swt/
Eclipse Public License 2.0
101 stars 123 forks source link

[win32] Consider monitor zoom when calculating bounds if autoScaleOnRuntime is active #1312

Open akoch-yatta opened 1 week ago

akoch-yatta commented 1 week ago

Currently the monitor bounds are calculated using the static DPIUtil.deviceZoom attribute as zoom. In multi monitor setup with different zooms, this will result in wrong bounds for the non primary monitor.

There are two scenarios:

  1. isAutoScaleOnRuntimeActive == false: This should behave as before, thefore DPIUtil.getDeviceZoom() is used
  2. isAutoScaleOnRuntimeActive == true: As the zoom of shell is now tied to the zoom of its monitor, it is important that each monitor bounds (in points) are based on there own zoom as well.

One example where you can construct an effect of it is with the CompletionProposalPopup. There, the clientArea of the monitor is used to make sure, maximum of 25% of the monitor is used by the popup. If the monitor is calculated with the wrong zoom, the popup will get bigger as expected, see Screenshot 2024-06-26 132107 vs. Screenshot 2024-06-26 132134

if the Monitor e.g. has 2160 pixels on a zoom of 200%, the primary monitor has a zoom of 100%. If the primary monitor zoom is used, the 2160 pixels will used as 2160 points. 25% of it is 540. As the popup will be initialized (correctly) with the zoom of 200% it will convert the 540 points back to 1080 pixels => that is bigger than it should be.

Conclusion: monitor zoom must be treated the same of the zoom of widgets.

github-actions[bot] commented 1 week ago

Test Results

   470 files  ±0     470 suites  ±0   7m 42s :stopwatch: -31s  4 135 tests ±0   4 127 :white_check_mark: ±0   8 :zzz: ±0  0 :x: ±0  16 336 runs  ±0  16 244 :white_check_mark: ±0  92 :zzz: ±0  0 :x: ±0 

Results for commit a5fd2540. ± Comparison against base commit a29aa312.

:recycle: This comment has been updated with latest results.

akoch-yatta commented 6 days ago

From a functional perspective, the changes look good. I only have a nitpicky comment regarding readability: I see (at least) two responsibilities in this method:

  1. Determine and set the correct zoom value for the monitor
  2. Determine and set the bounds and clientArea of the monitor

Their calculations seem to be completely independent, but the control flow mixes them up. Would is be possible to first do everything related to the zoom value (the OS call for retrieving result, validating the result and assigning it to monitor.zoom) and then do everything related to the bounds and the clientArea (calculate the values, retrieve the correct autoscaleZoom and set the according values on monitor)?

@HeikoKlare I moved the code in the method a bit. Can you have a look again? Do you mean something like that?

HeikoKlare commented 6 days ago

I moved the code in the method a bit. Can you have a look again? Do you mean something like that?

Yes, thank you! I think this improves comprehensibility as the blocks in the method can be read kind of independently now.