Add OfFloat.from methods in Point/Rectangle

[amartya4256]

This PR adds Rectangle.OfFloat.from and Point.OfFloat.from methods and removes the Win32DPIUtils.FloatAwareGeometryFactory class to make these methods OS-independent.

[github-actions]

Test Results

   546 files  ±0     546 suites  ±0   28m 2s ⏱️ - 4m 14s
 4 431 tests ±0   4 414 ✅ ±0   17 💤 ±0  0 ❌ ±0 
16 764 runs  ±0  16 637 ✅ ±0  127 💤 ±0  0 ❌ ±0 

Results for commit 59ed121. ± Comparison against base commit 508ad4a.

♻️ This comment has been updated with latest results.

[HeikoKlare]

Some questions on this proposal:

• Are the methods supposed to be public API or shall they only be used internal for now? In the latter case, they need to be marked @noreference. Otherwise, why do they need to be static at all and cannot just be toFloat(), as they become public API anyway? But since the classes themselves are currently @noreference, this method should probably be as well. In any case, they need a documentation.
• Since the methods are supposed to create OfFloat versions out of any other, wouldn't it make sense to place them inside Point.OfFloat, so that you write Point.OfFloat.From(something) instead of Point.from(something) but giving you an OfFloat instead of a pure Point.
• Is it correct and expected that a Point.WithMonitor loses the monitor information when passed to that method?

[amartya4256]

• Are the methods supposed to be public API or shall they only be used internal for now? In the latter case, they need to be marked @noreference. Otherwise, why do they need to be static at all and cannot just be toFloat(), as they become public API anyway? But since the classes themselves are currently @noreference, this method should probably be as well. In any case, they need a documentation.

We do not need @noreference since the classes are already marked with it. Adding the documentation to the method.

• Since the methods are supposed to create OfFloat versions out of any other, wouldn't it make sense to place them inside Point.OfFloat, so that you write Point.OfFloat.From(something) instead of Point.from(something) but giving you an OfFloat instead of a pure Point.

They are already inside Point.OfFloat.

• Is it correct and expected that a Point.WithMonitor loses the monitor information when passed to that method?

Adapting the method to call clone internally.

[HeikoKlare]

We do not need @noreference since the classes are already marked with it. Adding the documentation to the method.

They are already inside Point.OfFloat.

I see. I expected them to be in Point as obviously a minor version increment was necessary which I would not have expected for something effectively not being API.

You need to add an @since tag to the clone method implementation now.

[laeubi]

@HeikoKlare @fedejeanne if we move it here, would that not mean they should become public API now? And if not why not move them into the internal packages?

[fedejeanne]

@HeikoKlare @fedejeanne if we move it here, would that not mean they should become public API now? And if not why not move them into the internal packages?

The OfFloat classes are already internal API, they are marked with @noreference.

[laeubi]

The OfFloat classes are already internal API, they are marked with @noreference.

This is not the same. So if we now move them to a "more public place" and it was promised to make them public API last time, why not make them public API now (or consequently keep them internal as is).

@noreference is a plain PDE API tools thing that no other tooling likely understands nor is it a requirement for any consumers of SWT.

[fedejeanne]

Good point @laeubi , thank you.

@amartya4256 can you draft this one too until you decide how to proceed?

[amartya4256]

@laeubi so is your proposal to make both Point.OfFloat and Rectangle.OfFloat public API?

[laeubi]

Last time it was added "windows only" and @HeikoKlare stated we should make it sometimes public API (but not now).

So if we now move it "up in the chain" to make it available for other OS, it might be it is "the time" to do so ... that's just my feelings about it before someone uses it by accident.

[HeikoKlare]

Making it available for other OSes and making it public API are two different things. Unless it's necessary, we should avoid making things public API as it will make it more difficult to change it afterwards. If there is no specific need to make this public API right now, I would not be in favor of doing so.

[laeubi]

If there is no specific need to make this public API right now, I would not be in favor of doing so.

The point is that once we put it into commons it seems there actually is a specific need for it as it means the thing has escaped from the (internal) win32 impl it was needed for.

[HeikoKlare]

I can only repeat myself:

Making it available for other OSes and making it public API are two different things.

[laeubi]

I can only repeat myself:

Making it available for other OSes and making it public API are two different things.

And I can only repeat myself that the demand was to use it in a LayoutManger what is public API that needs to be used by any SWT user so its likely other (outside SWT) will need it if we make it available for "others"...

So using @norefrefence for "I don't like to maintain its API" is not sufficient. This is usually because one needs to expose a type for technical reasons but it should be a last resort and not the norm (e.g. assume a library requires the type to be public). Also in P2 we have had the case already that these become defacto API because people simply used it (and either did not noticed or suppressed the warning or did not use API tools in their setups).

[amartya4256]

I mean this PR was intended to move the methods to the right class since having a utility in Win32DPIUtils in an inner factory class doesn't make much sense and the classes (Point.OfFloat and Rectangle.OfFloat) themselves seem to be the right candidate for such methods. So from a refactoring POV it seems to be a right thing to do. But what I believe the the discussion here is about if it should be an API or a @noreference. Am I right?

I think since we intend for it to be not used outside by any consumer outside SWT, there's no need for it to be public. We also have a lot of static methods in many Common classes which have noreference methods. For instance we have StyledText#updateAndRefreshCarets, Drawable#internal_new_GC, ImageData#internal_new

The same mistakes can be made with these methods as well. We intend to not encourage usage of OfFloat outside SWT at least for now.