Skip to content

Add drawImage API that takes only a destination rectangle #2548

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
Merged
merged 1 commit into from
Sep 29, 2025
Merged

Add drawImage API that takes only a destination rectangle #2548

merged 1 commit into from
Sep 29, 2025

Conversation

@arunjose696
Copy link
Contributor

@arunjose696 arunjose696 commented Sep 25, 2025

Introduces a new method that draws the full image into a specified destination rectangle without requiring source bounds. Consumers no longer need to call image.getBounds() to determine image dimensions, unlike the existing drawImage method (Image image, int srcX, int srcY, int srcWidth, int srcHeight, int destX, int destY, int destWidth, int destHeight), which required knowing the image size in advance.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 25, 2025 •
edited
Loading

Test Results

  111 files   -  7    111 suites   - 7   12m 59s ⏱️ +59s
4 527 tests  - 54  4 511 ✅  - 51  14 💤  - 3  1 ❌  - 1  1 🔥 +1 
  274 runs   - 56    273 ✅  - 51   1 💤  - 3  0 ❌  - 2 

For more details on these failures and errors, see this check.

Results for commit 3e42bcc. ± Comparison against base commit f51bb3b.

This pull request removes 56 and adds 2 tests. Note that renamed tests count towards both. 
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 ImageWin32Tests ‑ test_getImageData_fromCopiedImage
AllWin32Tests ImageWin32Tests ‑ test_getImageData_fromImageForImageDataFromImage
AllWin32Tests TestTreeColumn ‑ test_ColumnOrder
…

org.eclipse.swt.tests.junit.Test_org_eclipse_swt_dnd_Clipboard ‑ Unknown test
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_graphics_GC ‑ test_drawImageLorg_eclipse_swt_graphics_ImageIIII

♻️ This comment has been updated with latest results.

@arunjose696 arunjose696 force-pushed the arunjose696/newAPIDrawImage branch 2 times, most recently from 1010784 to 1fe62ac Compare September 26, 2025 07:28
@arunjose696 arunjose696 mentioned this pull request Sep 26, 2025
Closed
@arunjose696 arunjose696 linked an issue Sep 26, 2025 that may be closed by this pull request
Add drawImage API that takes only a destination rectangle vi-eclipse/Eclipse-Platform#459
Closed
@arunjose696 arunjose696 force-pushed the arunjose696/newAPIDrawImage branch from 1fe62ac to 58b826a Compare September 26, 2025 07:36
fedejeanne
fedejeanne reviewed Sep 26, 2025
View reviewed changes
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.

Initial review: please apply formatting in the affected code.

Question: why add a new API method, who needs it? Maybe in a follow-up PR?

@fedejeanne
Copy link
Member

fedejeanne commented Sep 26, 2025

Question: why add a new API method, who needs it?

Answer to myself: it's in our internal issue (vi-eclipse/Eclipse-Platform#459)

A new API in GC is required to draw images at specified size. This is required for vi-eclipse/Eclipse-Platform#326 .

@arunjose696 arunjose696 force-pushed the arunjose696/newAPIDrawImage branch 2 times, most recently from 3ff5643 to 932a013 Compare September 26, 2025 12:34
fedejeanne
fedejeanne reviewed Sep 29, 2025
View reviewed changes
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.

Changes make sense.

Since there are no usages of this API, I can't test it and therefore I can't really approve it. I don't oppose it though 👍

@arunjose696 arunjose696 force-pushed the arunjose696/newAPIDrawImage branch from 932a013 to 882b8fc Compare September 29, 2025 09:22
@arunjose696
Copy link
Contributor Author

arunjose696 commented Sep 29, 2025 •
edited
Loading

Changes make sense.

Since there are no usages of this API, I can't test it and therefore I can't really approve it. I don't oppose it though 👍

I have now added a usage in the CTabFolder renderer. To test it, you can run an Eclipse runtime workspace:

  • In open files, the unselected ones are drawn using the updated drawImage call in drawUnselected(). This now uses the new API, and you can verify that it has no visual impact on the image.
  • The drawBackground method was also updated to use the new API, which made it possible to remove the getBounds() call.

For adapting the remaining usages I would be creating an issue

fedejeanne
fedejeanne reviewed Sep 29, 2025
View reviewed changes
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 adding the usage, @arunjose696 . I see no visual defects on Windows when using this PR 👍

I haven't tested on Mac / Linux.

@arunjose696 arunjose696 force-pushed the arunjose696/newAPIDrawImage branch 3 times, most recently from ba7ee78 to acf8ccb Compare September 29, 2025 12:53
@akoch-yatta akoch-yatta force-pushed the arunjose696/newAPIDrawImage branch from acf8ccb to e832de5 Compare September 29, 2025 14:08
@arunjose696 arunjose696 force-pushed the arunjose696/newAPIDrawImage branch 2 times, most recently from e107c10 to fe093a2 Compare September 29, 2025 14:44
@arunjose696
Copy link
Contributor Author

arunjose696 commented Sep 29, 2025

I have also added the tests for this new API similar to other drawImage() calls

@arunjose696 @akoch-yatta
Add drawImage API that takes only a destination rectangle
3e42bcc 
Introduces a new method that draws the full image into a specified destination rectangle without requiring source bounds.
Consumers no longer need to call image.getBounds() to determine image dimensions, unlike the existing drawImage method
(Image image, int srcX, int srcY, int srcWidth, int srcHeight, int destX, int destY, int destWidth, int destHeight), which
required knowing the image size in advance.
@akoch-yatta akoch-yatta force-pushed the arunjose696/newAPIDrawImage branch from fe093a2 to 3e42bcc Compare September 29, 2025 16:51
@akoch-yatta
Copy link
Contributor

akoch-yatta commented Sep 29, 2025

Main purpose for this additional API is to get rid of any Image#getBounds calls in advance as this could result in creating unnecessary handles in windows. Additionally, most calls to this API are the exact use case of the new API: draw an Image scaled - so the original method should mainly be used to draw a dropped & scaled Image.

akoch-yatta
akoch-yatta approved these changes Sep 29, 2025
View reviewed changes
Copy link
Contributor

@akoch-yatta akoch-yatta left a comment

Choose a reason for hiding this comment

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

I tested on windows and shortly on GTK, both worked as expected to me. Reasons for the new API are mentioned.

@akoch-yatta akoch-yatta merged commit 9a317a3 into eclipse-platform:master Sep 29, 2025
21 of 22 checks passed
@akoch-yatta akoch-yatta deleted the arunjose696/newAPIDrawImage branch September 29, 2025 17:22
@arunjose696 arunjose696 mentioned this pull request Sep 30, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@HeikoKlare HeikoKlare HeikoKlare left review comments

@fedejeanne fedejeanne fedejeanne left review comments

@akoch-yatta akoch-yatta akoch-yatta approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add drawImage API that takes only a destination rectangle

4 participants

@arunjose696 @fedejeanne @akoch-yatta @HeikoKlare