Skip to content

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
@github-actions
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
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.

LGTM.

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

@fedejeanne
Copy link
Member

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
Member

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