-
Notifications
You must be signed in to change notification settings - Fork 186
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.
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
Add drawImage API that takes only a destination rectangle #2548
Conversation
Test Results 111 files - 7 111 suites - 7 12m 59s ⏱️ +59s 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.♻️ This comment has been updated with latest results. |
1010784 to
1fe62ac
Compare
1fe62ac to
58b826a
Compare
fedejeanne
left a comment
There was a problem hiding this 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?
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/GC.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/GC.java
Outdated
Show resolved
Hide resolved
Answer to myself: it's in our internal issue (vi-eclipse/Eclipse-Platform#459)
|
3ff5643 to
932a013
Compare
fedejeanne
left a comment
There was a problem hiding this 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 👍
932a013 to
882b8fc
Compare
I have now added a usage in the CTabFolder renderer. To test it, you can run an Eclipse runtime workspace:
For adapting the remaining usages I would be creating an issue |
fedejeanne
left a comment
There was a problem hiding this 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.
bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/graphics/GC.java
Outdated
Show resolved
Hide resolved
ba7ee78 to
acf8ccb
Compare
acf8ccb to
e832de5
Compare
e107c10 to
fe093a2
Compare
|
I have also added the tests for this new API similar to other drawImage() calls |
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.
fe093a2 to
3e42bcc
Compare
|
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
left a comment
There was a problem hiding this 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.
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.