Skip to content

Conversation

akoch-yatta
Copy link
Contributor

This PR replaces the usage of the zoom attribute in methods in the SingleZoomCoordinateMapper that translate Point and Rectangle from and to display coordinates to use DPIUtil.getDeviceZoom() instead. As each widget inherits its zoom from DPIUtil.getDeviceZoom() when the SingleZoomCoordinateMapper is involved, the behavior is not changed in most scenarios. but this PR fixes the scenarios, if a widget is scaled to a different zoom, e.g. by utilizing widget.setData("AUTOSCALE_DISABLED", true) the current implementation will break all conversions. One important factor in the CoordinateMapper implementation is to have a consistent translation from and to display coordinates. In the MultiZoomCoordinateSystemMapper this is achieved by extracting the Monitor (zoom) out of the coordinate to translate. In SingleZoomCoordinateMapper, the implementation was not consistent yet, as e.g. in SingleZoomCoordinateMapper#getCursorLocation or SingleZoomCoordinateMapper#setCursorLocation DPIUtil.getDeviceZoom() is already used.

@akoch-yatta akoch-yatta linked an issue Aug 5, 2025 that may be closed by this pull request
Copy link
Contributor

github-actions bot commented Aug 5, 2025

Test Results

   546 files  ±0     546 suites  ±0   29m 57s ⏱️ + 2m 40s
 4 425 tests ±0   4 408 ✅ ±0   17 💤 ±0  0 ❌ ±0 
16 746 runs  ±0  16 619 ✅ ±0  127 💤 ±0  0 ❌ ±0 

Results for commit 1b3ae75. ± Comparison against base commit 9baef58.

♻️ This comment has been updated with latest results.

This commit replaces the usage of the zoom attribute in methods in the
SingleZoomCoordinateMapper that translate Point and Rectangle from and
to display coordinates to use DPIUtil.getDeviceZoom() instead. The
implementation was not consistent yet, as e.g. in
SingleZoomCoordinateMapper#getCursorLocation or
SingleZoomCoordinateMapper#setCursorLocation DPIUtil.getDeviceZoom() is
already used.
@HeikoKlare HeikoKlare force-pushed the win32-single-zoom-coordinate-mapper-zoom branch from 0bac779 to 1b3ae75 Compare August 5, 2025 13:08
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.

The change definitely makes sense. As a follow up, we should remove the obsolete zoom parameter of those methods.

I wonder if the mapMonitorBounds() method is not affected in the same way?
One consumer is the Display#getMonitor() method:

monitor.setBounds(coordinateSystemMapper.mapMonitorBounds(boundsInPixels, autoscaleZoom));

Shouldn't those bounds also be scaled according to the device zoom instead of the zoom of that specific monitor in case of a single zoom mapper? Otherwise with a 100% primary monitor and a 200% secondary monitor, I would expect the bounds of the secondary monitor, scaled with 200% instead of 100%, to be wrong.

@akoch-yatta
Copy link
Contributor Author

akoch-yatta commented Aug 5, 2025

The change definitely makes sense. As a follow up, we should remove the obsolete zoom parameter of those methods.

I wonder if the mapMonitorBounds() method is not affected in the same way? One consumer is the Display#getMonitor() method:

monitor.setBounds(coordinateSystemMapper.mapMonitorBounds(boundsInPixels, autoscaleZoom));

Shouldn't those bounds also be scaled according to the device zoom instead of the zoom of that specific monitor in case of a single zoom mapper? Otherwise with a 100% primary monitor and a 200% secondary monitor, I would expect the bounds of the secondary monitor, scaled with 200% instead of 100%, to be wrong.

Yes, I thought about that, but wanted to come up with a test for that. But I think we will need to adapt those as well.

@HeikoKlare
Copy link
Contributor

Yes, I thought about that, but wanted to come up with a test for that. But I think we will need to adapt those as well.

Alright, we can also keep that for a separate PR and merge this one as is.

@akoch-yatta
Copy link
Contributor Author

I would propose to do that separately and merge this PR as is. I will test the original behavior to add a proper test set for mapMonitorBounds to ensure the correct behavior.

@HeikoKlare HeikoKlare merged commit 7e6f2aa into eclipse-platform:master Aug 6, 2025
17 checks passed
@HeikoKlare HeikoKlare deleted the win32-single-zoom-coordinate-mapper-zoom branch August 6, 2025 15:44
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.

Use deviceZoom in single zoom mapper
2 participants