Skip to content

GC with Image in unsupported use cases #1979

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.

Sign up for GitHub

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

GC with Image in unsupported use cases #1979

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1743,6 +1743,14 @@ private ImageHandle init(ImageData i, int zoom) {
@Override
public long internal_new_GC (GCData data) {
if (isDisposed()) SWT.error(SWT.ERROR_GRAPHIC_DISPOSED);

if(imageProvider != null && (imageProvider instanceof ImageDataProviderWrapper || imageProvider instanceof ImageFileNameProviderWrapper)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instanceof already implicitly does a null check, so you can remove that one.
And change if( to if (

System.err.println("***WARNING: Image initialized with ImageDataProvider or ImageFileNameProvider is not supposed to be modified");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is really wrong, then an exception should be thrown.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Throwing an exception is kind of a breaking change. Yes, it's kind of wrong to do this, but making erroneous consumers (that just had incorrect images so far) aware of this by potentially breaking the complete application through an exception is probably nothing we should do. Throwing an exception would be something that needs to be guarded by an explicit strict mode (we have one already).

Copy link
Contributor

@laeubi laeubi Apr 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how consumers will be "made aware" other than forbid bad calls. E.g. system outs will go completely unnoticed in regular Eclipse installs as they never show up in the error log.

Also this will not show nay wrong assumptions e.g. in unit tests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't do anything about this situation as a (product) consumer anyway. This information is relevant for developers / framework consumers. What would be your proposal?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR title says "GC with Image in unsupported use cases", so it either is supported the nothing has to be done or it is not supported then an Exception must be thrown as otherwise people will simply use it.

As this is also only done for Windows, I wonder what developers / framework consumer should benefit from that, and it more feels like a temporary debugging facility as the message is completely unclear what it is an error / unsupported at all and what should be the alternative. Bot provided examples look perfectly valid to me as one might want to draw over an image and then write it to a file or whatever.

In any case we have org.eclipse.swt.widgets.Display.getRuntimeExceptionHandler() and org.eclipse.swt.widgets.Display.getErrorHandler() facility so application devlopers at least have control over how the issue is reported/handled.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it either is supported the nothing has to be done or it is not supported then an Exception must be thrown as otherwise people will simply use it.

It was always a really bad idea to do that but it became unsupported with the introduction of HiDPI support to SWT. With recent changes supporting multiple handles for different zooms on Windows, this became even more problematic. But you cannot simply start throwing exception when consumers have (for bad reasons) used this for years. It was not explicitly marked as unsupported, so there are likely consumers that will fail with such a change. What is obivously missing here is some documentation of when you can reasonably draw onto an image (conforming with when the message is not printed).

As this is also only done for Windows, I wonder what developers / framework consumer should benefit from that, and it more feels like a temporary debugging facility as the message is completely unclear what it is an error / unsupported at all and what should be the alternative.

Yes, that's exactly what you may use it for. Improving the message is of course fine. And and may of course make sense to also introduce this to the other OS implementations.

Bot provided examples look perfectly valid to me as one might want to draw over an image and then write it to a file or whatever.

The examples may not be perfect, but whenever you have an image where you retrieve the data for different zooms from different sources, drawing onto these images is a really bad idea (as it will not give you the results that you expect, at latest when changing the zoom).

In any case we have org.eclipse.swt.widgets.Display.getRuntimeExceptionHandler() and org.eclipse.swt.widgets.Display.getErrorHandler() facility so application devlopers at least have control over how the issue is reported/handled.

How may these handlers be used here? Those are for exceptions/errors in listeners and callbacks, so how would they fit here?

Anyway, we should not overcomplicate this. Either there is a proposal for a better solution, or we can agree on an improved version of the proposal, or we simply leave everything as is and leave it up to consumers to find issues and drop any chance of support for them to find issues. Unfortunately, that will also complicate the search for errors for ourselves as well, but we can at least locally at such a logging for a testing session and try to find issues by that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as it will not give you the results that you expect, at latest when changing the zoom

I don't think that the primary use-case of images is changing the zoom dynamically. Images are used mostly in application code as images, and I don't see that the API forbid (or discourage) getting a "zoomed" image and paint on it. As in the first example I might then retrieve the ImageData and do whatever I want with that. Or I paint the image on a component (in wich case I might need to handle zoom changes anyways). All the examples even dispose the image right away and these are common examples so I can't see how this is unsupported or qualifies for a warning.

For the second example I'm completely lost... How is one supposed to create a (user drawn) image if not by creating one with a GC and paint on it? Would we expect that users are creating images pixel-by-pixel?

How may these handlers be used here?

At least platform registers these handlers and perform certain actions on it (at least log them) so for me there are two ways:

  1. We reuse these and e.g. handle it like a RuntimeException in a handler (e.g with passing an AssertionException), this might be arguable if this is really "handler code" but I don't think it is really that strict. e.g. org.eclipse.swt.widgets.Synchronizer.syncExec(Runnable) use it and this is obviously not always a "handler" so any code running in the UI thread can be seen as "handler code" anyways.
  2. We add a new pair of get/setAssertionHandler() that explicitly accepts an AssertionException so we can pass any failed assertions there so that application code is able to handle it in whatever fashion seems suitable

With that approach there is a stack trace to trace down the actual call (this is what concerns me most at the moment that there is literally no way to find out where the call is coming from (except attaching a debugger) and there is a little chance people complain and things get fixed.

}

if(zoomLevelToImageHandle != null && !zoomLevelToImageHandle.isEmpty()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

space after if
I think the check is not complete, we should exclude the ImageGCDrawerWrapper. This one would be fine no matter the amount of handles.
And shouldn't we check for zoomLevelToImageHandle.size() > 1 instead? Isn't there always already one handle at this point or will it be created later?
I would extend the message and add "should not be modified with a GC. Changes will not be applied on existing handles"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point there should be no handle. That's why I added the empty check. We don't create handle on initialization anymore.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes you are right. It will implicitly create the handle, when it is first used inside a GC call 👍

System.err.println("**WARNING: Images with handles created for a different zoom level should not be modified.");
}
/*
* Create a new GC that can draw into the image.
* Only supported for bitmaps.
Expand Down
Loading