Take MonitorAware out of the geometry hierarchy with as few other changes as possible

[nedtwigg]

This PR follows a very simple recipe

• make Point and Rectangle how they were before Use monitor-aware coordinates in multi-zoom coordinate system #1711
• make the MonitorAware classes not extend anything, and have no concept of equality
• change types to the internal APIs until the compile error go away

CoordinateSystemMapperTests are still passing.

[HeikoKlare]

I understand that there seems to be an issue regarding equality. But please first read the explanations given in #1711 (comment) and #1711 (comment). Removing the hierarchy between point/rectangle and their monitor-aware specialization breaks the monitor-specific UI scaling. Not everything is covered by tests, so probably we need to add some tests for the coordinate system at shell level (as it's currently limited to the mappers themselves) to better cover the necessary behavior at shell level.
Can you give examples (in the best case, reproducing tests) that demonstrate the actual issue we need to discuss about?

[nedtwigg]

Point topLeft = shell.getLocation();
// one way to get an offset
Point offset = new Point(topLeft.x + 5, topLeft.x + 5);
// another way to get the exact same point
topLeft.x += 5;
topLeft.y += 5;
Point otherOffset = topLeft;
// the two points are not equal, but only on Windows
assertThat(offset).isEqualTo(otherOffset) //fails

That would be easy to fix by changing the .equals, it's a contrived example that I'm not worried about. The bigger deal is that other methods are going to behave differently based on whether I made a Point from scratch in an event, or whether I grabbed a magic Point from the correct getter, and then translated it.

You're maintaining this project and have brought it back to life, the onus is on me not on you. Feel free to CLOSE WONTFIX. But as an experienced user, I no longer understand:

• what is the SWT coordinate system
• when is it okay to create a Point, or when do I need to create a MonitorAwarePoint
• how can I know which methods are going to give me which, and which methods are going to respond to the monitor value if present

If there is any possible way to resolve the scaling issue (which fwiw has not been a problem for me or my users who are heavily in multimonitor windows setups) in either a more encapsulated way (what I tried to do here) or a more explicit way (create public API which demand a MonitorAwarePoint in the places where it affects behavior), I think that would be a big win. Nullability is famously the "$1 billion mistake", the PR I'm objecting to is quite a clever trick which solves a real problem, I get it, but I am so worried about the kinds of debugging that having magic points which may or may not have more than just an x and y is going to require.

[HeikoKlare]

As said, I am totally fine with fixing issues regaring equality. Maybe we could just consider, e.g., a Point and MonitorAwarePoint symetrically equal if their x/y values are equal, as that's what is expected everywhere else than in the coordinate system (as no consumer should know or at least care about monitor-aware implementations).

• when is it okay to create a Point, or when do I need to create a MonitorAwarePoint
• how can I know which methods are going to give me which, and which methods are going to respond to the monitor value if present

The monitor-aware implementations are not part of the API, so no one is supposed to instantiate or should need to otherwise care about them.

If there is any possible way to resolve the scaling issue (which fwiw has not been a problem for me or my users who are heavily in multimonitor windows setups)

Out of curiousity: since when are you using monitor-specific scaling without facing an issue with the coordinate system across monitors? First chance to test the experimental feature was just recently with the 2024-12 release.

or a more explicit way (create public API which demand a MonitorAwarePoint in the places where it affects behavior)

Yes, it would of course be best to carry everything properly via the API. Actually, it is of course possible to develop a better designed solution. The problem is that such a solution would require significant changes of the API and of every consumer (inside Eclipse and outside). That's why we chose the current approach, which, as far as we are aware of, works in a sufficient way without changes of the API and consumers. Only thing that seem to be an issue is the implementation of quality of the affected classes.

[nedtwigg]

a Point and MonitorAwarePoint symetrically equal if their x/y values are equal

If MonitorAwarePoint remains within the inheritance hierarchy, I think it would be a good idea for equality and hashCode to be based only on x, y, and the MonitorAware part can be invisible from an equality perspective.

That would address all my concerns except the fact that sometimes some other SWT method will behave differently depending on whether it is a Point or a MonitorAwarePoint. My intuition is that

• if this is rare, then this feature is not needed
• if it's common, then MonitorAwarePoint is not going to work because too many clients will be broken

since when are you using monitor-specific scaling without facing an issue with the coordinate system across monitors? First chance to test the experimental feature was just recently...

This means I haven't used it at all, but I also haven't had a problem (nor has a customer made a request) which drove me to desire this feature. My lack of experience or need doesn't invalidate the experience and needs of other users in the ecosystem, of course! But every user uses the coordinate system, monitor-specific scaling on Windows seems a niche interest compared to that.

The problem is that such a solution would require significant changes of the API and of every consumer

My goal with this PR was to try to trace where these magic points were created to where they were consumed, to see if there was a way to connect these points using internal API, and thus avoid all these issues. And based on this, I still worry that this API is going to cause bugs because new Point(e.x, e.y) in event handlers is going to get scaled/positioned differently / incorrectly.

And this where I want to emphasize that you don't owe me an explanation! But if you have more time to give, am I correct about the following?

• if every shell is contained completely within a single monitor (no single shell across multiple monitors) then this monitor-aware scaling is not neccesary
• if a shell is spread across multiple monitors, and those monitors have different DPI, then a specific point on that shell can be ambiguous. Measuring from the top-left of the shell onto the second monitor within the Shell's coordinate system can give a different result than measuring from the top-left of the total Display in the Display's coordinate system

If I am correct about this, then I would consider shrinking the scope, and taking an approach something like this:

• our coordinate system has a limitation, things only work if you stay within this limitation
• here is a new coordinate system which is needed to operate past this limitation, things will only work correctly if you adopt this new coordinate system. we have adapted the built-in layouts to use it

If a new coordinate system is required to do this correctly, but it's too much work to add a new coordinate system, then maybe the feature is just too much work 🤷

[HeikoKlare]

If MonitorAwarePoint remains within the inheritance hierarchy, I think it would be a good idea for equality and hashCode to be based only on x, y, and the MonitorAware part can be invisible from an equality perspective.

I completely agree with this point.

And this where I want to emphasize that you don't owe me an explanation! But if you have more time to give, am I correct about the following?

• if every shell is contained completely within a single monitor (no single shell across multiple monitors) then this monitor-aware scaling is not neccesary
• if a shell is spread across multiple monitors, and those monitors have different DPI, then a specific point on that shell can be ambiguous. Measuring from the top-left of the shell onto the second monitor within the Shell's coordinate system can give a different result than measuring from the top-left of the total Display in the Display's coordinate system

No, this is not correct. I think the news on the feature sufficiently describes what it does: https://eclipse.dev/eclipse/news/4.34/platform.html#rescale-on-runtime-preference

But every user uses the coordinate system, monitor-specific scaling on Windows seems a niche interest compared to that.

I don't get this point. There is no "coordinate system vs. monitor-specific scaling". If you do not enable monitor-specific scaling, there won't be any of those monitor-aware points/rectangles at all and everything remains as it was. If you think monitor-specific scaling is not important or relevant, feel free to not use it. But you can we sure that we don't invest person years for a feature that is "a niche" :-)

[nedtwigg]

If you think monitor-specific scaling is not important or relevant, feel free to not use it.
You're maintaining this project and have brought it back to life, the onus is on me not on you

The main thing I want to say is thanks for bringing SWT back to life! For now I'm going to stay on SWT 4.34, and hope that by the time the Skija stuff lands maybe Point is back to being regular old final, but if it isn't then I'll do the small amount of workaround effort needed to adapt a Kotlin codebase to the sealed change in 4.35.

There is no "coordinate system vs. monitor-specific scaling"

So far, org.eclipse.swt.graphics.Point did not imply any coordinate system. You would look at the docs of the method, and it would tell you if it was relative to the top-left of a given Control, Shell, or the entire Display. The point didn't store that reference point.

If a Point has a Monitor attached to it, I don't understand what it changes, conceptually, in the ideal case. I don't think it means that the point is relative to the top-left of that particular monitor. When the Monitor is not passed in, the MultiZoomCoordinateSystemMapper happily calculates it from scratch. Which is why I tried these experiments to keep the WithMonitor thing internal. I haven't been able to figure out in which cases my event handlers would need to stop creating new Point() and start creating new MonitorAwarePoint() if I wanted the benefits of per-shell scaling. If a Shell is spanning multiple monitors, I'm not sure if I should pass in the monitor at the top-left of the shell, or the monitor where the point is. This is what I mean by "I have not been able to figure out what the coordinate system is". I think it should be an exact concept which is exactly defined

eclipse.platform.swt/bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/graphics/MonitorAwarePoint.java

Lines 21 to 24 in de99b2d

	* Instances of this class represent {@link org.eclipse.swt.graphics.Point}
	* objects along with the context of the monitor in relation to which they are
	* placed on the display. The monitor awareness makes it easy to scale and
	* translate the points between pixels and points.

I understand the difference between pixels and points, but I don't understand which thing MonitorAwarePoint is. You're not obligated to explain it to me! But I don't think the javadoc or code make it clear which is which.