-
Notifications
You must be signed in to change notification settings - Fork 49
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
Fix errors in drag cursors when using high-DPI screens #479
Conversation
I've created eclipse-platform/eclipse.platform.ui#2084 to contribute 2x variants for those drag icons. Everything seems to work for 100% zoom already, but I haven't had the chance to test this on a high-DPI screen. |
@@ -58,6 +60,26 @@ public static AbstractUIPlugin getDefault() { | |||
return singleton; | |||
} | |||
|
|||
public static int getDeviceZoom() { |
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 implementation will probably suffer from the same problem that DPIUtil
was suffering which is: the value will be obtained once and never updated again i.e. if the zoom of the monitor changes then this value will be outdated.
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.
How was this solved in SWT?
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.
DPIUtil.setDeviceZoom
is called on zoom changes, which keeps the deviceZoom
always up-to-date i.e. calling DPIUtil.getDeviceZoom()
should always give you the current zoom. But those methods are not accessible in this case, so this is just fyi.
In this specific case, you could simply avoid storing the zoom in the field deviceZoom
and query the system property org.eclipse.swt.internal.deviceZoom
every time. Since DPIUtil.setDeviceZoom(...)
is also updating that property then the result should be that querying it always gives you the right zoom :-)
Long story short: remove the field deviceZoom
and remove the condition if (deviceZoom == -1)
.
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.
That you for the deeper insight in how this is handled on the SWT side of things. This is pretty similar to what I had in mind.
Though there is a separate issue which makes me concerned about how much we can get out of it in the first place. If the zoom changes, we normally would also need to refresh the cursors, as they are still using the old image data.
But those cursors are API, meaning we can't simply dispose them and create new objects...
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.
With the latest state, the device zoom is no longer stored in a static variable, but always read directly from the system property.
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.
That you for the deeper insight in how this is handled on the SWT side of things. This is pretty similar to what I had in mind.
Though there is a separate issue which makes me concerned about how much we can get out of it in the first place. If the zoom changes, we normally would also need to refresh the cursors, as they are still using the old image data.
But those cursors are API, meaning we can't simply dispose them and create new objects...
Yes, that is true. The problems are that:
- The cursors are constants in
SharedCursors
i.e. one can not simply recreate them when the zoom changes Image
andImageData
don't really know anything about zoom changes which means once you create aCursor
out of anImage
/ImageData
, you're stuck with the zoom level at which they were created.
Point nr. 2 means that one has to manually react to zoom level changes and recreate the cursors with an image/image data at the appropriate zoom level, but this breaks the contract established in point nr. 1.
Sadly, I don't see an easy way out of this problem, at least not at this level of abstraction (GEF is a consumer of SWT). Best case scenario, this kind of issues would be handled transparently by SWT and not by the consumer (GEF), but I know this is not trivial since our team has been working on the HighDPI subject for quite a while now and this issue has also surfaced somewhere else.
I will take this to the team and we'll try to address it in vi-eclipse/Eclipse-Platform#65 too.
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.
Point nr. 2 means that one has to manually react to zoom level changes and recreate the cursors with an image/image data at the appropriate zoom level, but this breaks the contract established in point nr. 1.
That's what I'm afraid of. And it's not just affecting the SharedCursors class, but also the Draw2D Cursors class containing static instances of the native SWT cursors. In the long term, I believe the most sensible thing to do would be to deprecate those static fields and have something similar to the ImageDataProvider, except for cursors.
GEF can then listen to SWT.ZoomChanged events and reload the cursors accordingly. But that is going way beyond the scope of this PR and something we have to consider at a later date. Thank you again for your input.
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.
[...] it's not just affecting the SharedCursors class, but also the Draw2D Cursors class containing static instances of the native SWT cursors
Which class is that? If it's using the native cursors (i.e. the constructor that takes a style
as parameter) then they shouldn't be affected by this issue. AFAICT the issue only affects cursors created with the constructor that takes ImageData
as a parameter. You can test the natives cursor with the org.eclipse.swt.snippets.Snippet44
, that's what I did.
GEF can then listen to SWT.ZoomChanged events and reload the cursors accordingly. But that is going way beyond the scope of this PR and something we have to consider at a later date. Thank you again for your input.
Agreed, that's beyond the scope of this PR. Let's put a pin on it.
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.
Which class is that? If it's using the native cursors (i.e. the constructor that takes a style as parameter) then they shouldn't be affected by this issue.
I'm talking about this Cursors class, where we create static instances of the native cursors. Looking at the actual implementation, it looks like they don't actually work with ImageData instances, so you are indeed correct and they should be updated automatically.
If PNG images are used for the cursors then separate source and mask are not needed as the PNG format has transparency. |
The new images still have a white background, to make sure that they look exactly the same. Edit: Clients may also still run GEF with an older Eclipse version. By omitting the mask, we would break backwards compatibility. |
@ptziegler we already have some cursors without mask since the last release. I hope I didn't break anything there. But the mask always made problems at least for these cursors. So I'm happy when it is gone. |
@azoitl I talked to Heiko over at the Eclipse Platform PR and my current proposal is that for the new images, I drop the white background for transparency. In order to remain backwards compatible, we still need to use the mask. But for future cursors, it would then no longer be necessary. |
Thx for checking and the feedback. |
As a quick update: With the 2024-09, the mask & source images are going to be marked as "for removal", being replaced by a new constants that combine both into one. So instead of:
One can simply do:
|
So in principle, we could delay this PR until the M2 and then refactor the code or merge it now and keep the "old" images. The first would break backwards compatibility and require clients to also update to the latest Eclipse version, when updating GEF. For my perspective, I would prefer to keep using the old icons and wait out the 2-year deprecation period. |
9a396f9
to
c152f71
Compare
I've updated the PR to also scale the shared images. While it can't handle dynamic DPI changes, it at least won't throw an IllegalArgumentException anymore. |
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.
Just some nitpicks, the rest LGTM.
Regarding breaking backwards compatibility or not (#479 (comment)) I'd say you make it clear that something will break and give time for the clients to adapt. If the appropriate methods are being deprecated for removal anyway then there's no chance this change is going to be forgotten in 2 years anyway (one can live with the technical debt as long as there's an end in sight).
One last thing: I'd like to test this too. Can you please add a short "how to test" section to the description?
* on MacOS or Linux (x11 window system) or if the device zoom couldn't | ||
* otherwise be determined, this method returns {@code 100} as default value. | ||
*/ | ||
@SuppressWarnings("nls") |
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.
Better add the proper //$NON-NLS-X$
and remove this, otherwise you'll be possibly silencing valid warnings that may be introduced in future changes.
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've updated the PR to include the three things you've pointed out.
/** | ||
* Convenience class to get the image data for a given zoom level. If no image | ||
* for the requested zoom level exists, the image data may be an auto-scaled | ||
* version of the native image and may look more blurred or mangled than | ||
* expected. | ||
*/ |
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.
/** | |
* Convenience class to get the image data for a given zoom level. If no image | |
* for the requested zoom level exists, the image data may be an auto-scaled | |
* version of the native image and may look more blurred or mangled than | |
* expected. | |
*/ | |
/** | |
* Convenience method to get the image data for a given zoom level. If no image | |
* for the requested zoom level exists, the image data may be an auto-scaled | |
* version of the native image and may look blurred or mangled. | |
*/ |
/** | ||
* Creates a new cursor using the shared images as source and mask, | ||
* respectively. The cursors are created with respect to the current device | ||
* zoom, with the hotspot being the center of the image. | ||
*/ |
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.
Add the parameters to the javadoc and mention that they need to be names of shared cursors.
The cursors used in the flyout palette only exist for the default 100%. If a system is using a higher zoom level, the creation of those cursors fails with an IllegalArgumentException. Instead, we should scale the cursor image to match the current level. As opposed to reading the zoom level only once, it is now calculated dynamically, as the zoom may change depending on the display or because it has been changed. The methods for handling this case have also been moved from the SharedCursors class to the InternalGEFPlugin, so that it becomes available to the DragCursors class. From there, we call scaledImageData() to convert the image descriptor to image data matching the requested zoom. The scaling is done internally by SWT. See eclipse#464
LGTM (I can't merge though since I'm not a committer in GEF) |
The cursors used in the flyout palette only exist for the default 100%. If a system is using a higher zoom level, the creation of those cursors fails with an IllegalArgumentException. Instead, we should scale the cursor image to match the current level.
Instead of reading the zoom level only once, it is now calculated dynamically, as the zoom may change depending on the display or because it has been changed. The methods for handling this case have also been moved from the SharedCursors class to the InternalGEFPlugin, so that it becomes available to the DragCursors class.
From there, we call asImageDataProvider() to convert the image descriptor to image data which internally scales the image via DPIUtil magic, if necessary.
See #464