-
Notifications
You must be signed in to change notification settings - Fork 182
Use OS.CreateIconIndirect for colored cursors with source and mask #2517
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
base: master
Are you sure you want to change the base?
Conversation
491e76f
to
de5bba9
Compare
Test Results 118 files ±0 118 suites ±0 9m 54s ⏱️ -28s For more details on these failures, see this check. Results for commit 433b5ba. ± Comparison against base commit 43f54cc. ♻️ This comment has been updated with latest results. |
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Cursor.java
Outdated
Show resolved
Hide resolved
de5bba9
to
7b36487
Compare
7b36487
to
f271319
Compare
Failing test are unrelated, see #2516 |
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 have some concerns about this change:
- It introduces unnecessary complexity to
setupCursorFromIamgeData
as that internal utility method is made capable of handling cases that can not occur (or at least that were also not supported) by the existing API. - It change the return type of
getPointerSizeScaleFactor()
without any obvious need, increasing the complexity of consumer code. - The code is not properly formatted.
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Cursor.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Cursor.java
Outdated
Show resolved
Hide resolved
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.
My comment was addressed 👍
See Heiko's comments
de48225
to
43cf74d
Compare
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Cursor.java
Outdated
Show resolved
Hide resolved
if (this.mask != null) { | ||
int scaledMaskWidth = Math.round(this.mask.width * zoomFactor); | ||
int scaledMaskHeight = Math.round(this.mask.height * zoomFactor); | ||
scaledMask = this.mask.scaledTo(scaledMaskWidth, scaledMaskHeight); |
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.
This is a behavior change. Previously DPIUtil.scaleImageData()
was used for the mask and now ImageData::scaledTo
is used producing a different kind of result. This either needs to be explained or reverted.
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.
We talked about that and in your words, "probably the issue is that when using "proper" scaling for the source it creates an image with alpha data, such that the mask is not properly taken into account. Using scaledTo will of course lead to worse scaling results, but maybe it's the best solution for the source+mask use case then"
43cf74d
to
c9148df
Compare
Previously, the Cursor constructor that accepted both source and mask used OS.CreateCursor, which only supports monochrome cursors. As a result, any color information in the source was ignored and the cursor always appeared black. This change updates the constructor to use OS.CreateIconIndirect, allowing full-color cursors while still respecting the mask for transparency. DPI scaling of the source and mask now works correctly with colored cursors.
c9148df
to
433b5ba
Compare
Use OS.CreateIconIndirect for colored cursors with source and mask
Previously, the Cursor constructor that accepted both source and mask used OS.CreateCursor, which only supports monochrome cursors. As a result, any color information in the source was ignored and the cursor always appeared black.
This change updates the constructor to use OS.CreateIconIndirect, allowing full-color cursors while still respecting the mask for transparency. DPI scaling of the source and mask now works correctly with colored cursors.
How to Test
Run the following snippet:
Snippet386
Cursor(Device, ImageData, ImageData, int, int)
value from drop downResult
Before:

After:

Requires