Use Float Aware Geometry for Scaling #62

[amartya4256]

This PR introduces Rectangle.OfFloat and Point.OfFloat as an extension of Rectangle and Point respectively to improve scaling precision in the DPIUtil by using residuals obtained from rounding.

Contributes to
#62 and #128

[github-actions]

Test Results

104 files   -    441  104 suites   - 441   4s ⏱️ - 31m 45s
 68 tests  -  4 335   68 ✅  -  4 317  0 💤  -  18  0 ❌ ±0 
215 runs   - 16 518  215 ✅  - 16 378  0 💤  - 140  0 ❌ ±0 

Results for commit a9f3fc7. ± Comparison against base commit 5389894.

This pull request removes 4335 tests.

AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicASCII_dollarSign
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicASCII_emptyString
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicASCII_letterA
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicASCII_letters
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicUTF16LE_null
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicUTF16_AsciiLetters
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicUTF16_Asciiletter
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicUTF16_LotsOfLetters
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicUTF16_letter
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicUTF16_letters
…

♻️ This comment has been updated with latest results.

[laeubi]

I must confess that this XyzAware things sound strange from an object oriented point of view. one would always question e.g. if the fields are synchronized with the integer variants.

Is there any reason to not have proper getter/setter instead of public modifiable fields? The e.g. a setX(double) can always update the integer fields as well.

Also one might look at how AWT is doing it here: https://docs.oracle.com/javase/8/docs/api/java/awt/geom/Rectangle2D.html

So the have

• https://docs.oracle.com/javase/8/docs/api/java/awt/Rectangle.html (integer coordinates)
• https://docs.oracle.com/javase/8/docs/api/java/awt/geom/Rectangle2D.Float.html (float coordinates)
• https://docs.oracle.com/javase/8/docs/api/java/awt/geom/Rectangle2D.Double.html (double coordinates)

[HeikoKlare]

I must confess that this XyzAware things sound strange from an object oriented point of view. one would always question e.g. if the fields are synchronized with the integer variants.

Is there any reason to not have proper getter/setter instead of public modifiable fields? The e.g. a setX(double) can always update the integer fields as well.

Also one might look at how AWT is doing it here: https://docs.oracle.com/javase/8/docs/api/java/awt/geom/Rectangle2D.html

I don't see a good reason to not have proper getter/setters for the newly added fields. One issue from OO perspective is that we have the public fields in the existing Rectangle/Points classes which are accessed directly everywhere and makes it difficult to properly extend the classes. But for the additional classes with higher precision, which are only supposed to be used inside SWT at least for now anyway, it definitely makes sense to apply proper encapsulation.

I also agree that that names sound strange, at least the float version (for MonitorAware* it makes more sense to me).
Thank you for the reference to the AWT classes. I was not aware of them. Having the specializations realized as inner classes, so that you can have a Rectangle.Float (and maybe then also a Rectangle.WithMonitor or the like) sounds like a reasonable approach that we could also apply here.

[laeubi]

But for the additional classes with higher precision, which are only supposed to be used inside SWT at least for now anyway,

I'm not sure if we should design such "supposed to be internal" API here. Actually one big drawback of SWT compared to eg. Graphics2D is its inability to use floating point arithmetic in most places. One can prevent this by constructing a Path (that supports at least float), so I see an opportunity enable such advanced use cases with using "Double/Float" variants of rectangle, point and so on ... especially if transforms / scaling comes into play now it seems to limited to only allow integer / pixel drawing.

[HeikoKlare]

I'm not sure if we should design such "supposed to be internal" API here.

I think this is a misunderstanding. I did not want to propose to design an API that is supposed to only be internal. I just wanted to say that these classes are only supposed to be used internally for now, so still having the chance for redesign before they are eventually made public API. We should first adapt the code inside SWT to properly handle higher-precision coordinates (for the reasons you also mentioned) and see where/whether we run into problems there and what needs to be done to adapt all consumers of Point/Rectangle to properly handle the higher-precision versions of them. Afterwards, once we know how to properly do it and once everything is implemented, we can make it API public. If we would do it now, strange things would happen as a consumer could pass a point with floating-point precision to consumers inside SWT without them being adapted to properly consider the higher-precision value. So there is quite some need to adaptation before we might do that.

[amartya4256]

As per our discussion, I would move the *Aware classes inside Rectangle and Point and would rather call them Rectangle.Float and Rectangle.withMonitor (Same for Point) which would then have getters and setters implemented. The inner classes will be @noreference classes as they must only be used internally in SWT. I will then adapt the intended consumers accordingly, inside SWT.

[fedejeanne]

I like the idea, I have some concerns regarding loss of precision when cloning and some nitpicky comments about coding practices.

[laeubi]

One note about API, I think it would be good to have getter in Point/Rectangle by default

e.g. doube Point.getX() and double Point.getY() these can return the int field by default and the overridden by the concrete subclasses.

This has the advantage that code can move away from the int fields to proper getters/setters without needing to know the actual type of storage, this is similar to using Number from the java API. One can then have getXAsInt() and getXAsFloat() if desired (and maybe overridden again) and deemed useful.

As an alternative, a Point might has asDouble() / asFloat() that maybe just return this if the type already matches.

[fedejeanne]

Thank you for your suggestions @laeubi , I like them, but I would like to move that discussion to another PR. As Heiko mentioned already, we'd like to keep this PR about using float internally and see how it works. The design of the public API should be postponed since it's not trivial.

Would that be ok?

[laeubi]

It just seem to complicate things a lot, in any case I would love to see using higher precision values in SWT anyways :-D

[fedejeanne]

The day will come when we can all enjoy better precision in SWT (I'm also looking forward to it!) .

I'll take your comment as a 👍 on postponing the discussion about the public API, hope that's ok for you :-)

[fedejeanne]

The API Tools check is failing because internal classes were removed from Windows

But these classes were marked with @noreference, they were in Windows (no idea why the check complains about Linux) and the versions for Linux have been bumped already anyway (fe64f69).

Pull-Request Checks / check-versions / Check and increment service versions (pull_request) fails with this error, which is also unrelated:

Error:  Failed to execute goal org.eclipse.tycho.extras:tycho-p2-extras-plugin:5.0.0-SNAPSHOT:compare-version-with-baselines (compare-attached-artifacts-with-release) on project org.eclipse.swt.svg: Baseline and reactor have the same fully qualified version, but different content
Error:  different
Error:     META-INF/ECLIPSE_.RSA: not present in baseline
Error:     META-INF/ECLIPSE_.SF: not present in baseline

I am ignoring these check failures.

[akurtakov]

MonitorAwarePoint is not internal API, why have you thought it is? This caused api warnings on Linux too.

[akurtakov]

The noferencence is smth that majority of people will not see. Anyway as this has been in only couple of release I am fine with dropping it now but api filters have to be added ASAP to fix errors in workspaces.

[akurtakov]

The reason why api tools fail on all platform is because this code is in "Eclipse SWT/common" directory which is shared between all swt ws/os combos and thus api filters are needed for all of them.

[iloveeclipse]

Now all SWT PR's (like #2223) are failing with API errors and they are also visible in IDE on Linux!

List of breaking changes:
- The type org.eclipse.swt.graphics.MonitorAwareRectangle has 
been removed from org.eclipse.swt.gtk.linux.x86_64_3.130.0
- The type org.eclipse.swt.graphics.MonitorAwarePoint has been 
removed from org.eclipse.swt.gtk.linux.x86_64_3.130.0

[iloveeclipse]

Why anyone would merge this state:

I can't find words!

[iloveeclipse]

I plan to merge the reverted commits once the build will be green via #2264.

[iloveeclipse]

Please next time double check PR build results. All the checks we have are not there to be ignored, except it is known problem that is documented via a ticket.

[fedejeanne]

@iloveeclipse would you kindly tell me how to properly get rid of the API validation errors instead? I tried increasing the version of the affected plugins in my IDE before merging the PR but the error persisted so I assumed it was a bogus error.

How would you get rid of the errors?

[iloveeclipse]

I tried increasing the version of the affected plugins in my IDE before merging the PR but the error persisted so I assumed it was a bogus error.

The SWT bundle minor version segment was already increased for 4.37 release. We don't update minor segment multiple times during release cycle. Whatever error you saw, it was wrong solution anyway.

How would you get rid of the errors?

Like this but for all platforms:

diff --git a/binaries/org.eclipse.swt.gtk.linux.x86_64/.settings/.api_filters b/binaries/org.eclipse.swt.gtk.linux.x86_64/.settings/.api_filters
index 3a7b1ab..80df19f 100644
--- a/binaries/org.eclipse.swt.gtk.linux.x86_64/.settings/.api_filters
+++ b/binaries/org.eclipse.swt.gtk.linux.x86_64/.settings/.api_filters
@@ -10,2 +10,26 @@
     </resource>
+    <resource path="META-INF/MANIFEST.MF">
+        <filter comment="See https://github.com/eclipse-platform/eclipse.platform.swt/issues/62" id="923795461">
+            <message_arguments>
+                <message_argument value="3.131.0"/>
+                <message_argument value="3.130.0"/>
+            </message_arguments>
+        </filter>
+    </resource>
+    <resource path="META-INF/MANIFEST.MF" type="org.eclipse.swt.graphics.MonitorAwarePoint">
+        <filter comment="See https://github.com/eclipse-platform/eclipse.platform.swt/issues/62" id="305324134">
+            <message_arguments>
+                <message_argument value="org.eclipse.swt.graphics.MonitorAwarePoint"/>
+                <message_argument value="org.eclipse.swt_3.131.0"/>
+            </message_arguments>
+        </filter>
+    </resource>
+    <resource path="META-INF/MANIFEST.MF" type="org.eclipse.swt.graphics.MonitorAwareRectangle">
+        <filter comment="See https://github.com/eclipse-platform/eclipse.platform.swt/issues/62" id="305324134">
+            <message_arguments>
+                <message_argument value="org.eclipse.swt.graphics.MonitorAwareRectangle"/>
+                <message_argument value="org.eclipse.swt_3.131.0"/>
+            </message_arguments>
+        </filter>
+    </resource>
 </component>

Note, usually we do not delete non deprecated API, I assume this is safe now only because it was introduced in the last release and it was marked as noreference, so there shouldn't be many clients affected, if any.