Skip to content
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

IllegalArgumentException: Argument cannot be null when creating SharedCursor #464

Open
Phillipus opened this issue Jul 4, 2024 · 4 comments

Comments

@Phillipus
Copy link
Contributor

Phillipus commented Jul 4, 2024

Cursors for the Palette are created in the SharedCursors class. The ImageData is returned from the ImageDescriptor at the current device zoom level. But if the zoom level is not 100% or 200% then ImageData will be null and an exception is thrown.

  1. On Windows launch a GEF-based app with a Palette with the VM argument -Dswt.autoScale=150
  2. Select a tool from the palette

This is caused by the ImageDescriptor not finding an image with a @1.5x suffix.

Perhaps a fallback is required so that if ImageData is null 100% zoom is used instead?

@Phillipus
Copy link
Contributor Author

Phillipus commented Jul 4, 2024

Or another possibility is to get the ImageData from an Image rather than an ImageDescriptor as that will not be null:

    private static Cursor createCursor(String sourceName) {
        // ImageDescriptor will load @2x image
        ImageDescriptor src = ImageDescriptor.createFromFile(Internal.class, sourceName);
        Image img = src.createImage();        
        ImageData id = img.getImageData(getDeviceZoom());
        img.dispose();
        return new Cursor(null, id, 0, 0);
    }

@Phillipus
Copy link
Contributor Author

There are other cursors created in FlyoutPaletteComposite here -

Because these are created with ImageDescriptor.getImageData() with no zoom level they are too small on Windows at hi-dpi 200% scale. These would need to be changed as well.

@ptziegler
Copy link
Contributor

I agree that throwing an exception is definitely not the way to go. My initial approach would've been to fall back to the 100% zoom level, as you suggested. Alternatively, one could also make it so that it goes to the "closest" zoom level instead. e.g. from 125% to 100% and from 175% to 200%.

private static Cursor createCursor(String sourceName) {
	ImageDescriptor src = ImageDescriptor.createFromFile(Internal.class, sourceName);
	ImageData data = src.getImageData(getDeviceZoom());
	if (data == null) {
		// if no image for that zoom level exists use the one for the default zoom level
		data = src.getImageData(100);
	}
	return new Cursor(null, data, 0, 0);
}

Or another possibility is to get the ImageData from an Image rather than an ImageDescriptor as that will not be null:

    private static Cursor createCursor(String sourceName) {
        // ImageDescriptor will load @2x image
        ImageDescriptor src = ImageDescriptor.createFromFile(Internal.class, sourceName);
        Image img = src.createImage();        
        ImageData id = img.getImageData(getDeviceZoom());
        img.dispose();
        return new Cursor(null, id, 0, 0);
    }

Wouldn't that cause the cursor to be scaled to the current zoom level? That might also work, though I'm a little concerned how good those scaled cursors will look...

Because these are created with ImageDescriptor.getImageData() with no zoom level they are too small on Windows at hi-dpi 200% scale. These would need to be changed as well.

Apparently, those icons come from here and haven't been updated in... 21 years. Yikes.
https://github.com/eclipse-platform/eclipse.platform.ui/blob/master/bundles/org.eclipse.ui/icons/full/pointer/

There are no @2x variants for those icons, so we would have to replace them with something else entirely.

@Phillipus
Copy link
Contributor Author

Wouldn't that cause the cursor to be scaled to the current zoom level? That might also work, though I'm a little concerned how good those scaled cursors will look...

Yep, and they look a bit scrunched. But then so do the icons in the palette at that setting so they complement each other ;-).

I experimented with things like "if displayZoom <= 150 then use 100" and "if displayZoom > 150 then use 200" but if the setting is -Dswt.autoScale=300 then even the double size cursors are too small relative to everything else in the UI. Icons are scaled by SWT's DPIUtil magic to match the scale setting, so I thought maybe it should be the same for cursors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants