GC with Image in unsupported use cases

[ShahzaibIbrahim]

Logging a warning for GC initialized with unsupported use cases.

1. all dynamic images (retrieved via any of the existing providers).
2. all images for which handles in other zoom values have already been created.

Examples

import org.eclipse.swt.*;
import org.eclipse.swt.graphics.*;
import org.eclipse.swt.widgets.*;

public class SWTGCImageExample {
	static final ImageFileNameProvider filenameProvider = zoom -> {
		String path = null;
		switch (zoom) {
		case 150:
			path = "bin/org/eclipse/swt/snippets/eclipse.png";
			break;
		case 200:
			path = "bin/org/eclipse/swt/snippets/eclipse.png";
			break;
		default:
			path = "bin/org/eclipse/swt/snippets/eclipse.png";
		}
		return path;
	};

    public static void main(String[] args) {
    	 Display display = new Display();
         Shell shell = new Shell(display);

         Image image = new Image(display, filenameProvider);
         GC gc = new GC(image);

         // Draw something on the image
         gc.setForeground(display.getSystemColor(SWT.COLOR_RED));
         gc.drawRectangle(50, 50, 100, 100);
         gc.drawText("Hello SWT!", 60, 90);

         // Dispose GC after drawing
         gc.dispose();

         // Dispose image
         image.dispose();

         shell.dispose();
         display.dispose();
    }
}

import org.eclipse.swt.*;
import org.eclipse.swt.graphics.*;
import org.eclipse.swt.widgets.*;

public class SWTImageHandleExample {
    public static void main(String[] args) {
        Display display = new Display();

        // Create an image using the display
        Image image = new Image(display, 200, 200);

        // Obtain the handle from the Image using win32_getHandle
        long handle = image.win32_getHandle(image, 100);
        System.out.println("Created image handle: " + handle);

        // Initialize GC with the image
        GC gc = new GC(image);

        // Modify the image using GC
        gc.setBackground(display.getSystemColor(SWT.COLOR_BLUE));
        gc.fillRectangle(50, 50, 100, 100);

        // Dispose GC and Image
        gc.dispose();
        image.dispose();

        display.dispose();
    }
}

[github-actions]

Test Results

   539 files  + 6     539 suites  +6   25m 44s ⏱️ + 2m 20s
 4 330 tests +37   4 320 ✅ +35  10 💤 +3  0 ❌  - 1 
16 407 runs  +37  16 309 ✅ +35  98 💤 +3  0 ❌  - 1 

Results for commit 84840bc. ± Comparison against base commit 74c39bf.

[akoch-yatta]

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

[akoch-yatta]

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"

[ShahzaibIbrahim]

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

[akoch-yatta]

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

[laeubi]

I don't think that printing to system out is appropriate here.

Either it is an error, then we should throw an exception or it should simply be ignored.

[laeubi]

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

[HeikoKlare]

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

[laeubi]

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.

[HeikoKlare]

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?

[laeubi]

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.

[HeikoKlare]

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.

[laeubi]

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.