Skip to content

Conversation

@amartya4256
Copy link
Contributor

@amartya4256 amartya4256 commented Jul 14, 2025

This PR introduces Rectangle.OfFloat & Point.OfFloat while using the getter / setter of a win32 widget to have better precision while scaling them up.

contributes to #62

@amartya4256 amartya4256 force-pushed the amartya4256/add_float_setter_getter branch from c1712f2 to f4b62c6 Compare July 16, 2025 12:29
@github-actions
Copy link
Contributor

github-actions bot commented Jul 16, 2025

Test Results

   539 files   -  7     539 suites   - 7   34m 35s ⏱️ + 7m 38s
 4 358 tests  - 54   4 343 ✅  - 52   14 💤  - 3  0 ❌ ±0  1 🔥 +1 
16 664 runs   - 54  16 539 ✅  - 52  124 💤  - 3  0 ❌ ±0  1 🔥 +1 

For more details on these errors, see this check.

Results for commit 7c98ed4. ± Comparison against base commit 34a4d76.

This pull request removes 54 tests.
AllWin32Tests ImageWin32Tests ‑ testDisposeDrawnImageBeforeRequestingTargetForOtherZoom
AllWin32Tests ImageWin32Tests ‑ testDrawImageAtDifferentZooms(boolean)[1] true
AllWin32Tests ImageWin32Tests ‑ testDrawImageAtDifferentZooms(boolean)[2] false
AllWin32Tests ImageWin32Tests ‑ testImageDataForDifferentFractionalZoomsShouldBeDifferent
AllWin32Tests ImageWin32Tests ‑ testImageShouldHaveDimesionAsPerZoomLevel
AllWin32Tests ImageWin32Tests ‑ testRetrieveImageDataAtDifferentZooms(boolean)[1] true
AllWin32Tests ImageWin32Tests ‑ testRetrieveImageDataAtDifferentZooms(boolean)[2] false
AllWin32Tests TestTreeColumn ‑ test_ColumnOrder
AllWin32Tests Test_org_eclipse_swt_dnd_DND ‑ testByteArrayTransfer
AllWin32Tests Test_org_eclipse_swt_dnd_DND ‑ testFileTransfer
…

♻️ This comment has been updated with latest results.

@arunjose696
Copy link
Contributor

I notice many methods are being updated to return Point.OfFloat or Rectangle.OfFloat, should this be also changed in function definitions?

@amartya4256
Copy link
Contributor Author

amartya4256 commented Jul 21, 2025

I notice many methods are being updated to return Point.OfFloat or Rectangle.OfFloat, should this be also changed in function definitions?

Changing the return type changes the API, which is not allowed. Moreover, Rectangle.OfFloat and Point.OfFloat are children of Rectangle and Point respectively. So we don't need to do so also allowing better abstraction.

@amartya4256 amartya4256 force-pushed the amartya4256/add_float_setter_getter branch from f4b62c6 to f3120a1 Compare July 21, 2025 09:49
@amartya4256 amartya4256 force-pushed the amartya4256/add_float_setter_getter branch from f3120a1 to 8540ca5 Compare July 21, 2025 10:46
This commit introduces the use of Rectangle.OfFloat and Point.OfFloat in win32 Widgets'
getters and setter for a better precision.

contributes to eclipse-platform#62
@HeikoKlare HeikoKlare force-pushed the amartya4256/add_float_setter_getter branch from 8540ca5 to 7c98ed4 Compare July 22, 2025 13:22
Copy link
Member

@fedejeanne fedejeanne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the proposal, @amartya4256 . Is there any visual effect/issue that is solved by this PR? Put otherwise: where exactly do we need more precision when working with rectangles/points?

I ask because the PR is quite big and exposes some (still) internal details like the fact that one needs to instantiate internal classes (OfFloat) to achieve the desired precision.

@amartya4256 amartya4256 marked this pull request as draft July 23, 2025 09:04
@amartya4256
Copy link
Contributor Author

Thank you for the proposal, @amartya4256 . Is there any visual effect/issue that is solved by this PR? Put otherwise: where exactly do we need more precision when working with rectangles/points?

I ask because the PR is quite big and exposes some (still) internal details like the fact that one needs to instantiate internal classes (OfFloat) to achieve the desired precision.

As discussed, we will not merge it now and rather only introduce OfFloat in places where necessary. I'll keep this PR as a draft till then.

@amartya4256
Copy link
Contributor Author

Since Win32DPIUtils already returns OfFloat variants of Point and Rectangle now, this PR doesn't make any difference anymore. I had a run through with the Widgets' getBounds and getSize calls and the behaviour is identical with master hence we should discard this PR now.

@HeikoKlare
Copy link
Contributor

An additional remark on this: what this PR would (currently) change implicitly is the rounding behavior used in Win32DPIUtils in case a rectangle returned by one of the modified methods it directly passed to another method that uses a pointToPixel calculation from Win32DPIUtils. This is actually bad design, as the rounding behavior should depend on the use case and should thus be explicitly defined by the caller (e.g., via calling an according method) and not by accident by the type an object has, depending on where and how it was created and whether it was passed through (maybe with modification) or if its data have been wrapped into another object. That is an additional argument for my why this is not the right thing to do in general but may, if at all, only be done after cleaning up the Win32DPIUtils implementation to not alter method behaviors depending on the type that is passed to those methods.

@HeikoKlare HeikoKlare closed this Nov 17, 2025
@HeikoKlare HeikoKlare deleted the amartya4256/add_float_setter_getter branch November 17, 2025 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

4 participants