Skip to content

Deprecate Image(Rectangle) constructor and replace usages #2088

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.

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

Merged
merged 1 commit into from
May 8, 2025

Conversation

ShahzaibIbrahim
Copy link
Contributor

@ShahzaibIbrahim ShahzaibIbrahim commented May 2, 2025

Deprecate Image constructor that accepts Rectangle in favor of Image(display, width, height).

@ShahzaibIbrahim ShahzaibIbrahim linked an issue May 2, 2025 that may be closed by this pull request
2 tasks
Copy link
Contributor

github-actions bot commented May 2, 2025

Test Results

   539 files   -  6     539 suites   - 6   30m 8s ⏱️ + 1m 34s
 4 340 tests  - 36   4 324 ✅  - 34   15 💤  - 3  1 ❌ +1 
16 610 runs   - 33  16 471 ✅  - 31  138 💤  - 3  1 ❌ +1 

For more details on these failures, see this check.

Results for commit de604cc. ± Comparison against base commit 5d05f5a.

This pull request removes 37 and adds 1 tests. Note that renamed tests count towards both.
AllWin32Tests org.eclipse.swt.graphics.ImageWin32Tests ‑ testImageDataForDifferentFractionalZoomsShouldBeDifferent
AllWin32Tests org.eclipse.swt.graphics.ImageWin32Tests ‑ testImageShouldHaveDimesionAsPerZoomLevel
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testByteArrayTransfer
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testFileTransfer
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testHtmlTransfer
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testImageTransfer_fromCopiedImage
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testImageTransfer_fromImage
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testImageTransfer_fromImageData
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testImageTransfer_fromImageDataFromImage
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testRtfTransfer
…
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_graphics_Image ‑ test_ConstructorLorg_eclipse_swt_graphics_DeviceLorg_eclipse_swt_graphics_int_int

♻️ This comment has been updated with latest results.

@HannesWell
Copy link
Member

As far as I can tell, this constructor is currently often used to take screenshots from Displays or Controls.
If we introduce a corresponding API for that, as proposed in #2104, the callers would be reduced a lot, when using the new API.

@ShahzaibIbrahim ShahzaibIbrahim force-pushed the master-203 branch 3 times, most recently from fa34257 to b504ee4 Compare May 7, 2025 13:16
Copy link
Contributor

@HeikoKlare HeikoKlare left a comment

Choose a reason for hiding this comment

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

As far as I can tell, this constructor is currently often used to take screenshots from Displays or Controls.

That means it's a convenience constructor for the case where you have bounds (as rectangle) at hand, but the decision was actually questionable as such bounds may have x/y != 0, which has no proper meaning when using this constructor.
So having specific API for the screenshot use cae as proposed in #2104 would be nice.

@ShahzaibIbrahim ShahzaibIbrahim force-pushed the master-203 branch 4 times, most recently from 5170784 to 4cb5414 Compare May 8, 2025 09:52
Deprecate Image constructor that accepts Rectangle in favor of
Image(display, width, height).
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.

LGTM.

I replaced some usages of the deprecated constructor in GTK and added a new test for the preferred constructor

@fedejeanne
Copy link
Contributor

Test failure unrelated: #2113

@fedejeanne fedejeanne merged commit 1d59663 into eclipse-platform:master May 8, 2025
15 of 17 checks passed
@HeikoKlare
Copy link
Contributor

Please process the PRs replacing the consumers as soon as possible (e.g., eclipse-platform/eclipse.platform.ui#2953). In general, it would be best to replace all known consumers before deprecating the method.
Otherwise PR builds for repos with consumers may show errors because of introduced warnings, even though they were not introduced by the PR itself.

@fedejeanne
Copy link
Contributor

Please process the PRs replacing the consumers as soon as possible (e.g., eclipse-platform/eclipse.platform.ui#2953)

... it was always my plan :-)

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dynamize Image constructors with Rectangle
4 participants