[win32] different coordinate system strategies #1630
Conversation
There was a problem hiding this comment.
The changes look sound to me. I've not tested yet, but just looked at static code correctness. I only have minor comments so far.
Since I did quite some manual comparison with the state before 0e7f799 (which introduced specific coordinate system behavior for rescaling at runtime), I was wondering whether we may add a revert commit for 0e7f799 and reapply this on top of it, to make it easier to review and identify the changes compared to existing behavior.
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Display.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Display.java
Show resolved
Hide resolved
akoch-yatta
force-pushed
the
extract-display-coordinate-system
branch
from
f642464 to
137fbd8
Compare
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Display.java
Outdated
Show resolved
Hide resolved
akoch-yatta
force-pushed
the
extract-display-coordinate-system
branch
2 times, most recently
from
c2d8158 to
34aab7f
Compare
akoch-yatta
force-pushed
the
extract-display-coordinate-system
branch
2 times, most recently
from
7d0ba65 to
dde6558
Compare
There was a problem hiding this comment.
Thank you for splitting up the commit to a revert and the separation of different mapping strategies. The changes look good to me now. I have taken a particularly thorough look at the SingleZoomCoordinateSystemMapper for the existing HiDPI behavior.
I have some minor points left:
- If I understand correctly, the regression reported in Context menus do not appear where mouse click occurred, often context menu appears on a different display eclipse.platform.ui#2595 was introduced by set Location of menu wrt Display coordinate system #1536 and will be fixed by this PR. Can you please add that information to the commit message and the PR description, so that the issue will be auto-linked and auto-closed once this is merged?
- The commit message contains some typos ("strategie" instead of "strategies", "This contributes" instead of "This contribution"). Could you please correct them?
- One proposal for simplification directly in the code.
| private int getApplicableMonitorZoom(Monitor monitor) { | ||
| return DPIUtil.getZoomForAutoscaleProperty(isRescalingAtRuntime() ? monitor.zoom : getDeviceZoom()); | ||
| } |
There was a problem hiding this comment.
This method only seems to be used within the MultiZoomCoordinateSystemMapper, so maybe we could move it there and remove the case distinction whether rescaling at runtime is active (as that mapper is only used when that mode is active).
There was a problem hiding this comment.
Good point, I moved and adapted the method accordingly
akoch-yatta
force-pushed
the
extract-display-coordinate-system
branch
from
dde6558 to
4ef8e11
Compare
This reverts commit 0e7f799.
This contribution extracts two different strategies to provide a consistent coordinate system in the win32 implemenentation for a single-zoom and a multi-zoom environment. The existing logic remains unchanged in this commit. It is only moved and consolidated in the new inner classes in Display. Contributes to eclipse-platform#62 and eclipse-platform#131 Fixes eclipse-platform/eclipse.platform.ui#2595
HeikoKlare
force-pushed
the
extract-display-coordinate-system
branch
from
4ef8e11 to
fbfb9c0
Compare
This contributes extracts two different strategies to provide a consistent coordinate system in the win32 implemenentation for a single-zoom and a multi-zoom environment. The existing logic remains unchanged in this commit. It is only moved and consolidated in the new inner classes in Display.
This PR fixes eclipse-platform/eclipse.platform.ui#2595 as well