Fix errors in drag cursors when using high-DPI screens #479

ptziegler · 2024-07-16T04:33:11Z

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

ptziegler · 2024-07-16T04:35:02Z

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.

fedejeanne · 2024-07-16T09:11:08Z

org.eclipse.gef/src/org/eclipse/gef/internal/InternalGEFPlugin.java

@@ -58,6 +60,26 @@ public static AbstractUIPlugin getDefault() {
 		return singleton;
 	}

+	public static int getDeviceZoom() {


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.

How was this solved in SWT?

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).

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...

With the latest state, the device zoom is no longer stored in a static variable, but always read directly from the system property.

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 and ImageData don't really know anything about zoom changes which means once you create a Cursor out of an Image/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.

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.

[...] 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.

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.

Phillipus · 2024-07-17T19:14:24Z

If PNG images are used for the cursors then separate source and mask are not needed as the PNG format has transparency.

ptziegler · 2024-07-17T19:23:17Z

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 · 2024-07-17T19:57:10Z

At 100% zoom:

At 200% zoom:

At 300% zoom:

azoitl · 2024-07-18T18:52:35Z

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.

ptziegler · 2024-07-18T19:25:01Z

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

azoitl · 2024-07-21T20:11:31Z

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

ptziegler · 2024-07-21T20:14:36Z

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:

createCursor(ISharedImages.IMG_OBJS_DND_INVALID_SOURCE, ISharedImages.IMG_OBJS_DND_INVALID_MASK);

One can simply do:

createCursor(ISharedImages.IMG_OBJS_DND_INVALID);

ptziegler · 2024-07-21T20:17:25Z

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.

ptziegler · 2024-07-21T21:27:47Z

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.

fedejeanne

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?

fedejeanne · 2024-07-22T07:16:17Z

org.eclipse.gef/src/org/eclipse/gef/internal/InternalGEFPlugin.java

+	 * 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")


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.

I've updated the PR to include the three things you've pointed out.

fedejeanne · 2024-07-22T07:20:52Z

org.eclipse.gef/src/org/eclipse/gef/internal/InternalGEFPlugin.java

+	/**
+	 * 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.
+	 */


Suggested change

/**

* 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.

*/

fedejeanne · 2024-07-22T07:23:28Z

org.eclipse.gef/src/org/eclipse/gef/ui/palette/FlyoutPaletteComposite.java

+		/**
+		 * 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.
+		 */


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

fedejeanne · 2024-07-25T13:05:21Z

LGTM (I can't merge though since I'm not a committer in GEF)

ptziegler added this to the 3.21.0 milestone Jul 16, 2024

ptziegler force-pushed the drag-cursors branch from 8963cef to 25968f0 Compare July 16, 2024 05:49

fedejeanne reviewed Jul 16, 2024

View reviewed changes

ptziegler force-pushed the drag-cursors branch from 25968f0 to 5171232 Compare July 17, 2024 19:53

ptziegler marked this pull request as ready for review July 17, 2024 19:57

ptziegler force-pushed the drag-cursors branch from 5171232 to 1bef440 Compare July 17, 2024 19:58

HeikoKlare mentioned this pull request Jul 18, 2024

Convert shared DnD icons from bmp to png and add 2x zoom variants eclipse-platform/eclipse.platform.ui#2084

Merged

ptziegler mentioned this pull request Jul 19, 2024

Mark Draw2D Cursors class as deprecated and for-removal #481

Closed

ptziegler force-pushed the drag-cursors branch 2 times, most recently from 9a396f9 to c152f71 Compare July 21, 2024 21:17

ptziegler linked an issue Jul 21, 2024 that may be closed by this pull request

IllegalArgumentException: Argument cannot be null when creating SharedCursor #464

Closed

fedejeanne reviewed Jul 22, 2024

View reviewed changes

ptziegler force-pushed the drag-cursors branch from c152f71 to 9d6e39c Compare July 22, 2024 13:27

fedejeanne approved these changes Jul 25, 2024

View reviewed changes

azoitl approved these changes Jul 26, 2024

View reviewed changes

azoitl merged commit a9f29fd into eclipse:master Jul 26, 2024
9 checks passed

HeikoKlare mentioned this pull request Oct 18, 2024

Non system cursors are not adapted to DPI scale changes vi-eclipse/Eclipse-Platform#104

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix errors in drag cursors when using high-DPI screens #479

Fix errors in drag cursors when using high-DPI screens #479

ptziegler commented Jul 16, 2024 •

edited

Loading

ptziegler commented Jul 16, 2024

fedejeanne Jul 16, 2024

ptziegler Jul 16, 2024

fedejeanne Jul 17, 2024

ptziegler Jul 17, 2024

ptziegler Jul 17, 2024

fedejeanne Jul 18, 2024

ptziegler Jul 18, 2024

fedejeanne Jul 22, 2024

ptziegler Jul 22, 2024

Phillipus commented Jul 17, 2024

ptziegler commented Jul 17, 2024 •

edited

Loading

ptziegler commented Jul 17, 2024

azoitl commented Jul 18, 2024

ptziegler commented Jul 18, 2024

azoitl commented Jul 21, 2024

ptziegler commented Jul 21, 2024 •

edited

Loading

ptziegler commented Jul 21, 2024

ptziegler commented Jul 21, 2024

fedejeanne left a comment

fedejeanne Jul 22, 2024

ptziegler Jul 22, 2024

fedejeanne Jul 22, 2024

fedejeanne Jul 22, 2024

fedejeanne commented Jul 25, 2024

Fix errors in drag cursors when using high-DPI screens #479

Fix errors in drag cursors when using high-DPI screens #479

Conversation

ptziegler commented Jul 16, 2024 • edited Loading

ptziegler commented Jul 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Phillipus commented Jul 17, 2024

ptziegler commented Jul 17, 2024 • edited Loading

ptziegler commented Jul 17, 2024

azoitl commented Jul 18, 2024

ptziegler commented Jul 18, 2024

azoitl commented Jul 21, 2024

ptziegler commented Jul 21, 2024 • edited Loading

ptziegler commented Jul 21, 2024

ptziegler commented Jul 21, 2024

fedejeanne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fedejeanne commented Jul 25, 2024

ptziegler commented Jul 16, 2024 •

edited

Loading

ptziegler commented Jul 17, 2024 •

edited

Loading

ptziegler commented Jul 21, 2024 •

edited

Loading