Use monitor-aware coordinates in multi-zoom coordinate system

[amartya4256]

This PR contributes to the use of MonitorAware Points and Rectangles for the translation between points and pixels coordinates in the Display Coordinate System. Since the Display Coordinate System can have different scales (zoom) in different area, it is designed to be not continuous in the points coordinates. Hence when we manipulate the coordinates of a Point or a Rectangle object, it might end up in a region which is between the 2 monitors in the point coordinate system, which we consider a gap. So, we need the context of the monitor on which those points and rectangles were created in the first place to evaluate the scaling factor. If the context is not available or the coordinates were updated to an irrelevant value, a fallback method tries to evaluate the right monitor for the coordinates and evaluates the scaled value with that.

Contributes to #62 and #127

[github-actions]

Test Results

   494 files  ± 0     494 suites  ±0   9m 17s ⏱️ -34s
 4 330 tests + 4   4 317 ✅ + 9   13 💤  -  5  0 ❌ ±0 
16 568 runs  +32  16 460 ✅ +72  108 💤  - 40  0 ❌ ±0 

Results for commit 9a1c825. ± Comparison against base commit d441a95.

This pull request removes 6 and adds 10 tests. Note that renamed tests count towards both.

org.eclipse.swt.widgets.CoordinateSystemMapperTests ‑ translatePointInGapBackAndForthShouldBeTheSame(CoordinateSystemMapper)
org.eclipse.swt.widgets.CoordinateSystemMapperTests ‑ translateRectangleInGapBackAndForthShouldBeTheSame(CoordinateSystemMapper)
org.eclipse.swt.widgets.CoordinateSystemMapperTests ‑ translateRectangleInGapPartiallyInRightBackAndForthShouldBeTheSame(CoordinateSystemMapper)
org.eclipse.swt.widgets.CoordinateSystemMapperTests ‑ translateRectangleInPixelsOutisdeMonitorsBackAndForthShouldBeTheSame(CoordinateSystemMapper)
org.eclipse.swt.widgets.CoordinateSystemMapperTests ‑ translateRectangleInPointsInBothMonitorsPartiallyBackAndForthShouldBeTheSame(CoordinateSystemMapper)[1]
org.eclipse.swt.widgets.CoordinateSystemMapperTests ‑ translateRectangleInPointsInBothMonitorsPartiallyBackAndForthShouldBeTheSame(CoordinateSystemMapper)[2]

org.eclipse.swt.widgets.CoordinateSystemMapperTests ‑ translatePointInGapBackAndForthInMultiZoomShouldEndInsideTheSameMonitor
org.eclipse.swt.widgets.CoordinateSystemMapperTests ‑ translatePointInGapBackAndForthInSingleZoomShouldBeTheSame
org.eclipse.swt.widgets.CoordinateSystemMapperTests ‑ translateRectangleInGapBackAndForthInMultiZoomShouldBeInMonitorBounds
org.eclipse.swt.widgets.CoordinateSystemMapperTests ‑ translateRectangleInGapBackAndForthInSingleZoomShouldBeTheSame
org.eclipse.swt.widgets.CoordinateSystemMapperTests ‑ translateRectangleInGapPartiallyInRightBackAndForthInMultiZoomShouldBeInside
org.eclipse.swt.widgets.CoordinateSystemMapperTests ‑ translateRectangleInGapPartiallyInRightBackAndForthInSingleZoomShouldBeTheSame
org.eclipse.swt.widgets.CoordinateSystemMapperTests ‑ translateRectangleInPixelsOutisdeMonitorsBackAndForthShouldBeTheSame(CoordinateSystemMapper)[1]
org.eclipse.swt.widgets.CoordinateSystemMapperTests ‑ translateRectangleInPixelsOutisdeMonitorsBackAndForthShouldBeTheSame(CoordinateSystemMapper)[2]
org.eclipse.swt.widgets.CoordinateSystemMapperTests ‑ translateRectangleInPointsInBothMonitorsPartiallyBackAndForthInMultiZoomShouldNotEndUpInGap
org.eclipse.swt.widgets.CoordinateSystemMapperTests ‑ translateRectangleInPointsInBothMonitorsPartiallyBackAndForthInSingleZoomShouldBeTheSame

♻️ This comment has been updated with latest results.

[fedejeanne]

@amartya4256 I see this PR is a WiP (based on the commit text), which is fine for me, but please add some description to the PR (the issue that is fixed, intended result, how to test it, etc).

For the moment I can only do an initial review of the coding standards and so on.

[fedejeanne]

I played around a bit with it and this is what I found

When rescaling at runtime is deactivated

• Moving the workbench window between 2 monitors with different zoom rescales the menu bar according to the zoom of the monitor.

IIRC this is expected so I do not consider it a bug.

When rescaling at runtime is activated

No findings

[HeikoKlare]

No testing and in-depth review from my side so far, but at least one important point from my side: please mark the new classes as "not API" (i.e., "noreference"), as we should not make them public API (at least for now).

Please also improve the commit message to shortly explain the reasons for the change and what it does, so that people can later on generally understand what the commit does. Please also add that commit description to the PR description, in the based case with some how-to-test information.

[fedejeanne]

Looks better, I only found some unnecessary empty lines.

Let me know when this PR is ready to be tested again. If you (plan to) make more changes please don't squash the commits so I can review better. You (or I) can squash the commits once the PR has been approved by me and @HeikoKlare

[fedejeanne]

I tested and found this:

When rescaling at runtime is deactivated

• The size of the checkboxes and radio buttons in the Preferences reacts to zoom changes.

This is on the 100% monitor:

To reproduce:

• Start Eclipse in 200% monitor
• Open Preferences (it should open in the 200% monitor)
• Move Preferences dialog to the 100% monitor

When rescaling at runtime is activated

• It is still difficult to (re)attach views when the workbench window spans across 2 monitors (Corner case!)

In the video you can see that:

• I wasn't able to detach the editor in my 1st attempt
• Once detached, I could only reattach it on the 200% monitor (on the left). For your reference, the border between both monitors is right in the middle of the "Navigate back" (arrow)
  

My setup

• 100% Monitor on the right
• 200% Primary monitor on the left.

Prioritization

I can live with both issues so I suggest we merge this PR and leave the findings for another release. @HeikoKlare WDYT?

[HeikoKlare]

Is the issue with rescaling at runtime being deactivated really an issue introduced by this PR? I don't see how this could be related, so would be good to check that. Otherwise it might be an independent regression for which we need to open an issue and address separately as soon as possible.

[amartya4256]

When the rescaling at runtime is deactivated it still uses the old singlezoom mapper. So I don't think that this PR could have caused this issue.

[fedejeanne]

It's not --> vi-eclipse/Eclipse-Platform#191

[HeikoKlare]

@amartya4256 can you please update the commit message as requested in #1711 (review)?

Please also improve the commit message to shortly explain the reasons for the change and what it does, so that people can later on generally understand what the commit does. Please also add that commit description to the PR description, in the based case with some how-to-test information.

[HeikoKlare]

Some minor comments regarding documentation of new API.

[HeikoKlare]

Thank you, @amartya4256!
The changes now look fine to me. I have also tested the changes and found the behavior of the multi-zoom coordinate mapping much better now. It's only corner cases (right at the border between two monitors with different scalings) that sometimes behave a bit "unexpected", but at least it behaves rather "consistently" (e.g., regarding a preview of a detach operation and the final result). And those corner cases are not easy to resolve anyway, so it's fine like it is. In particular, dragging things to another monitor now behaves eben better than without monitor-specific scaling, where, for example, a detach operation moved to another monitor with different zoom value leads to unexpected positioning.

In addition, this change is safe in the sense that it only affected the multi-zoom coordinate system mapper, which is used by the experimental monitor-specific scaling mode on Windows. So no effects regarding existing behavior of "ordinary" functionality are to be expected.

With recent change 7a58782, I adapted the equals/hashcode implementations for Point/Rectangle and MonitorAwarePoint/MonitorAwareRectangle, as otherwise they would not take the monitor into account.

[nedtwigg]

First off, I love this new pace of innovation! I don't mind regressions or breaking changes when the code is moving this fast, so great to see SWT come back to life!

This change makes it impossible to instantiate a Point or Rectangle from Kotlin, due to a Sealed types cannot be instantiated error. It is possible to work around this by writing a factory method in java and replacing all new Point(x, y) with PointFactory.create(x, y).

But if the code is only used in the MultiZoomCoordinateSystemMapper, it seems like it would be better to contain this complexity there. Dunno if there's been any benchmarking, but I would expect keeping Point and Rectangle as monomorphic would help a fair bit.

Would you be open to making this geometric base classes back how they were, or are these MonitorAware versions here to stay?

[HeikoKlare]

Thank you for your feedback!

First of all, the MonitorAware versions are supposed to stay. They are only accessed in MultiZoomCoordinateSysteMapper but are also "used" outside. More precisely, a shell's methods return monitor-aware points/rectangles that may be processed by clients (changing their values) and then passed to any other method accepting points/rectangles in a shell again. It's is necessary to carry the monitor the original point/rectangle was created for belongs to, as otherwise it is not possible to identify the monitor it belongs to. The underlying issue is that some years ago a decision was made to use logical coordinates in SWT also at the display level (i.e., across all monitors), which is conceptually incompatible with monitor-specific scaling of the UI as it relies on a linear relation between logical points in pixels but with different scaling for every monitor, this is not univerally given anymore. The introduction of the monitoir-aware points/rectangles was the best way of dealing with the limitatiations without unacceptable glitched and without requiring to rewrite consumers that we found so far.

Regarding the issue with Kotlin, I have to admit that I do not understand the actual issue. The structure of, e.g., Point and MonitorAwarePoint is nothing special, just a sealed hierarchy of two classes. If Kotlin is not able to instantiate those types, my first impression is that Kotlinm must be incapable of properly processing sealed types at all (but I gues that's not the case). Do you know more about the reasons for Kotlin complaining here?

[ptziegler]

Regarding the issue with Kotlin, I have to admit that I do not understand the actual issue. The structure of, e.g., Point and MonitorAwarePoint is nothing special, just a sealed hierarchy of two classes. If Kotlin is not able to instantiate those types, my first impression is that Kotlinm must be incapable of properly processing sealed types at all (but I gues that's not the case).

From what I understand, Kotlin treats sealed types as abstract classes:
https://kotlinlang.org/docs/sealed-classes.html#constructors

A sealed class itself is always an abstract class, and as a result, can't be instantiated directly.

So my guess is that Kotlin does a 1-to-1 mapping from sealed Java classes to sealed Kotlin classes, while ignoring the fact that sealed Java classes don't need to be abstract.

[HeikoKlare]

Thank you for those insights, @ptziegler! I wasn't aware of that behavior/decision in Kotlin. If I do not miss anything, that means changing a final type to a sealed type is source- and binary-compatible for Java consumers, but for Kotlin consumers it might break. I don't think this is anything you need to or should considers when evolving Java APIs but rather consider it a limitation in Kotlin, in accordance with:

So my guess is that Kotlin does a 1-to-1 mapping from sealed Java classes to sealed Kotlin classes, while ignoring the fact that sealed Java classes don't need to be abstract.

So maybe this is something that should be reported to JetBrains?

[nedtwigg]

The problem is equality. If I have an event and I create a new Point(e.x, e.y), and then I map it to a control and then map it back, the result will be the exact same coordinates I started with. But they will not be equal, because one will be a Point and the other will be a MonitorAwarePoint. This ambiguous equality is the reason that Kotlin intentionally disallows this feature which Java allows.

The bug I mentioned above will only affect Windows, since it's the only place that these are being created right now, IIUC. It's also a problem for math. If I create a Point, map it, and then do intersections and such with it, what do I get? Does the monitor come along?

Compatibility with Kotlin isn't a big deal, there is an easy workaround, it's moreso that equality is now a bit unreliable for the fundamental geometry classes. I did a quick search with GPT and couldn't find any other widget kit whose geometry primitives optionally track monitor instances.

I opened two PRs to explore some alternatives:

• Make MonitorAwarePoint a windows-specific implementation detail #1903
• Remove MonitorAwarePoint and restore simple equality for geometry #1904

[HeikoKlare]

I have to admit that I do not understand that proposal. It conflicts with what I tired to explain in #1711 (comment)
With the proposed change, points and rectangles returned by the methods of a shell, modified outside of it, and then passed to that (or another) shell again, will not carry information about the monitor this point or rectangle belongs to anymore. That leads to positioning issues when using multiple monitors of different zoom, which have been addressed by this PR. Performing the proposed change would significantly degrade the behavior of the monitor-specific scaling support on Windows again.

And in my opinion, this is still without any proper need. We have a correct usage of sealed types and a source- and binary-compatible change made to use it. I am quite surprised that Kotlin seems to be incompatible with Java code regarding sealed types. But from my point of view, this pushes the responsibility for fixing this at Kotline side. Even if we found a solution here, this situation can occur again if Kotlin is not adapted. Maybe there is a misunderstanding on my side, but this is what I understood from the explanations so far.

[nedtwigg]

Kotlin incompatibility doesn't bother me at all, I'd like to drop that completely. My problem is that geometric equality is broken.

• Two points with the same coordinates might not be equal to each other, depending on whether one has a monitor implicitly attached.
• The way that a monitor should be passed along for arithmetic is not clear
• To me this is a big smell that there is some more internal approach which is possible. I tried again in Take MonitorAware out of the geometry hierarchy with as few other changes as possible #1905. If this approach is okay I think it could be cleaned up, but I tried to demonstrate that the tests can pass without changing the inheritance hierarchy of geometry.