Skip to content

Use Rectangle.OfFloat in Widgets getter/setter #2315

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Use Rectangle.OfFloat in Widgets getter/setter #2315

wants to merge 1 commit into from
+143 −138

Conversation

amartya4256
Copy link
Contributor

@amartya4256 amartya4256 commented Jul 14, 2025 •
edited
Loading

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 GitHub Actions
Copy link
Contributor

github-actions bot commented Jul 16, 2025 •
edited
Loading

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.

@amartya4256 amartya4256 linked an issue Jul 16, 2025 that may be closed by this pull request
Analyze: Create Rectangle.OfFloat inside setters and getters of bounds, size, location, etc (points and pixels) for better precision vi-eclipse/Eclipse-Platform#356
Open
@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 •
edited
Loading

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 linked an issue Jul 21, 2025 that may be closed by this pull request
Inconsistency between Control#getBounds and Control#getSize vi-eclipse/Eclipse-Platform#303
Open
@amartya4256 amartya4256 force-pushed the amartya4256/add_float_setter_getter branch from f3120a1 to 8540ca5 Compare July 21, 2025 10:46
@amartya4256 @HeikoKlare
Use OfFloat Geometries in Widgets getter/setter
7c98ed4 
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
fedejeanne
fedejeanne reviewed Jul 23, 2025
View reviewed changes
Copy link
Contributor

@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 amartya4256 mentioned this pull request Jul 31, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fedejeanne fedejeanne fedejeanne left review comments

+1 more reviewer

@arunjose696 arunjose696 arunjose696 left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@amartya4256 @arunjose696 @fedejeanne