Implement java.lang.AutoCloseable in class org.eclipse.swt.graphics.GC

[tobiasmelcher]

I suggest to implement the java.lang.AutoCloseable interface in org.eclipse.swt.graphics.GC so that the try-with-resources Java feature can be used.

Here a concrete example in org.eclipse.jface.dialogs.Dialog where the GC class is used.

protected void initializeDialogUnits(Control control) {
	// Compute and store a font metric
	GC gc = new GC(control);
	gc.setFont(JFaceResources.getDialogFont());
	fontMetrics = gc.getFontMetrics();
	gc.dispose();
}

gc.dispose() is not called when JFaceResources.getDialogFont() throws an exception.

Wouldn't it be much nicer if the try-with-resource approach would be used here:

protected void initializeDialogUnits(Control control) {
	// Compute and store a font metric
	try (GC gc = new GC(control)) {
	  gc.setFont(JFaceResources.getDialogFont());
	  fontMetrics = gc.getFontMetrics();
	}
}

Did you already discuss this topic? Are there any drawbacks of implementing the AutoCloseable interface in GC?

[vogella]

+1

[BeckerWdf]

Does anybody see unwanted side-effects when we would implement this proposal?

[merks]

I tried it locally a few minutes ago and checked if existing uses would get a warning about missing the close. New warnings that suggest close must be called would be very annoying...

[BeckerWdf]

I tried it locally a few minutes ago and checked if existing uses would get a warning about missing the close. New warnings that suggest close must be called would be very annoying...

And did new warnings happen?

[merks]

No, but I wasn't sure if that's because of some preferences. E.g.,

I'll test some more...

[merks]

Here is a self-contained example:

So yes, an auto closeable that isn't closed is considered a resource leak and depending on how a project configures the compiler preferences, this can be an error.

I don't think that's acceptable.

[tobiasmelcher]

Understood. We would introduce warnings or errors for existing code which does call dispose, but not using try-with-resources. We want to find all the locations which do not call dispose and introduce a SWT handle leak. Or make it easier for consumers so that dispose() must no longer be called explicitly.

[merks]

I fully understand the common/general use case of a GC needing to be disposed within the block of code in which it is created and see significant value in the suggestion from that point of view. It's just that when we talk about what "we" want, we ought to take into consideration everyone. Definitely not everyone will want new errors, warnings, or infos. Also, a great many of the "we" out there will want to provide libraries that work with older versions of SWT as well as with the latest version of SWT. This makes retrofitting good new patterns into the existing code a significant tradeoff for the huge downstream code base out there in the ecosystem.

[Phillipus]

Agree that it's a great idea but, like @merks, I implemented AutoCloseable in GC locally as a test and my workspace had several warnings about resource leaks. I could fix those in my own code but not in third-party code, so I would have to re-configure my workspace to turn the warnings/errors off which is not ideal.

[laeubi]

There are several ways to accomplish something similar for example one can have a static method in GC class like this:

public static void localGC(Drawable drawable, SWTRunnable runnable) {
	GC gc = new GC(drawable);
	try {
		runnable.run();
	} finally {
		gc.dispose();
	}
}

[HannesWell]

In general I like to leverage java language to increase the safety of the program and

I could fix those in my own code but not in third-party code, so I would have to re-configure my workspace to turn the warnings/errors off which is not ideal.

Do you have thrid-party code as sources in your workspace? Because for compiled jars such warnings are not shown. Maybe it would then be good to contribute to those projects (if they are open source).

There are several ways to accomplish something similar for example one can have a static method in GC class like this:

public static void localGC(Drawable drawable, SWTRunnable runnable) {
	GC gc = new GC(drawable);
	try {
		runnable.run();
	} finally {
		gc.dispose();
	}
}

I would name it GC.drawOn(drawable, ...) and I think this should be a consumer that accepts a gc, but in general this looks like a good suggestion. But then we should discourage from using the current constructor.

Definitely not everyone will want new errors, warnings, or infos.

I think nobody wants that, but applying a new method/class/etc. to the Eclipse SDK code base is often a good measure to see if it is really as good as hoped and to find potential flaws early. So ideally the designer also takes up that burden. :)

Also, a great many of the "we" out there will want to provide libraries that work with older versions of SWT as well as with the latest version of SWT. This makes retrofitting good new patterns into the existing code a significant tradeoff for the huge downstream code base out there in the ecosystem.

This is indeed a real difficulty and I understand that it is annoying for those that can't use the new things. But how do we then tell those that can use the new methods that there is something better to use? I doubt that that many scan the docs/classes for differences and only a fraction reads the N&N especially if there are tutorials out in the internet that are decades old. But a (deprecation, resource-leak, what ever) warning is often a good way to make people aware of a better alternative.

[jukzi]

you would need to

• org.eclipse.swt.graphics.Resource implements AutoCloseable,
• deprecated dispose() for removal
• refactor all usages of dispose() with .close() - easy
• wait 2 years

but i don't think its worth the effort, as SWT already offers checks to find non disposed Resources - but only few people care about.

[HannesWell]

There are several ways to accomplish something similar for example one can have a static method in GC class like this:

public static void localGC(Drawable drawable, SWTRunnable runnable) {
	GC gc = new GC(drawable);
	try {
		runnable.run();
	} finally {
		gc.dispose();
	}
}

I would name it GC.drawOn(drawable, ...) and I think this should be a consumer that accepts a gc, but in general this looks like a good suggestion. But then we should discourage from using the current constructor.

@tobiasmelcher would you be interested in implementing a solution like this?
In order to support arbitrary exceptions being thrown by the lambda, you could consider using the existing SWTCallable and SWTRunnable.

[HannesWell]

Thank you @tobiasmelcher for providing #1063.

While reviewing your PR and looking at the applications of the new method I noticed two major drawbacks of the GC#drawOn approach that accepts the action as runnable.

1. We need a custom runnable to propagate potential checked exceptions thrown by the drawing code
2. We cannot assign variables from the drawing-action which is often done at the moment, since a GC can be referenced without restrictions

To address the first point we already have SwtRunnable. But in order to solve the second point cumbersome workarounds like mutable-references (however implemented) are required. We could mitigated that by using a Supplier that can return at least one value, but then we again need a custom implementation that can throw checked exceptions, but a SwtSupplier would have to be added. But then all drawing actions must return a value and if they don't one often has to add an actually needless return null. And if multiple variables are to be assigned, this also doesn't really help. So overall the GC#drawOn approach can be quite inconvenient.

On the other hand, when using an auto-closable in a try-with-resources block, the handling of checked and unchecked exceptions can just be left unchanged. And also assigning one, multiple or no variable can be done as it is done now. Many use-cases of GC in the Eclipse SDK even already use a try-finally block to ensure the GC is disposed.
So from a (future) usability point of view the AutoClosable approach looks much more convenient to me.

But at the same time I agree that implementing AutoClosable in GC would cause a lot of friction not in the Eclipse SDK but also in other code. Searching the entire Eclipse SDK source code I see almost 200 hits of new GC in java files. That's a lot of code to change.

To make a long story short, to get the best of both worlds I propose to created a Closable subclass of GC that implements AutoClosable (since we are on Java-17 🙏🏽 we can just make GC sealed so nobody else can subclass it) and just use that class as replacement:

public sealed class GC extends Resource {

	public static final class Closeable extends GC implements AutoCloseable {
		Closeable(Drawable drawable, int style) {
			super(drawable, style);
		}
		@Override
		public void close() {
			dispose();
		}
	}

	public static GC.Closeable create(Drawable drawable) {
		return new Closeable(drawable, SWT.NONE);
	}
...

Users can then change their code from

GC gc = new GC(drawable);
// draw on gc
gc.dispose();

to

try (var gc = GC.create(drawable) {
  // draw on gc
}

without being disturbed at any existing call to new GC().

Instead of GC#create() we could just make the constructor of GC.Closeable public, but I didn't like it that much:

try (var gc = new GC.Closeable(drawable) {
  // draw on gc
}

What do you think?

If in the future we have the impression that GC#create() is used enough, we could then still deprecate its public constructor and/or just implement AutoClosable directly. But that's a future discussion.

[laeubi]

There is also SwtCallable look for example BusyIndicator what simply offers one method for runnable and callable code.

[HannesWell]

There is also SwtCallable

I actually looked for it, but somehow oversaw it.

look for example BusyIndicator what simply offers one method for runnable and callable code.

Yes that would be an option, but while both solutions of their pro's and con's and are similar complicated depending on the exact use-case, the Runable+Callable approach still is inconvenient if multiple variables should be assigned, which is already necessary in the applications in the PR.

[tobiasmelcher]

Thank you @tobiasmelcher for providing #1063.

While reviewing your PR and looking at the applications of the new method I noticed two major drawbacks of the GC#drawOn approach that accepts the action as runnable.

1. We need a custom runnable to propagate potential checked exceptions thrown by the drawing code
2. We cannot assign variables from the drawing-action which is often done at the moment, since a GC can be referenced without restrictions

To address the first point we already have SwtRunnable. But in order to solve the second point cumbersome workarounds like mutable-references (however implemented) are required. We could mitigated that by using a Supplier that can return at least one value, but then we again need a custom implementation that can throw checked exceptions, but a SwtSupplier would have to be added. But then all drawing actions must return a value and if they don't one often has to add an actually needless return null. And if multiple variables are to be assigned, this also doesn't really help. So overall the GC#drawOn approach can be quite inconvenient.

On the other hand, when using an auto-closable in a try-with-resources block, the handling of checked and unchecked exceptions can just be left unchanged. And also assigning one, multiple or no variable can be done as it is done now. Many use-cases of GC in the Eclipse SDK even already use a try-finally block to ensure the GC is disposed. So from a (future) usability point of view the AutoClosable approach looks much more convenient to me.

But at the same time I agree that implementing AutoClosable in GC would cause a lot of friction not in the Eclipse SDK but also in other code. Searching the entire Eclipse SDK source code I see almost 200 hits of new GC in java files. That's a lot of code to change.

To make a long story short, to get the best of both worlds I propose to created a Closable subclass of GC that implements AutoClosable (since we are on Java-17 🙏🏽 we can just make GC sealed so nobody else can subclass it) and just use that class as replacement:

public sealed class GC extends Resource {

	public static final class Closeable extends GC implements AutoCloseable {
		Closeable(Drawable drawable, int style) {
			super(drawable, style);
		}
		@Override
		public void close() {
			dispose();
		}
	}

	public static GC.Closeable create(Drawable drawable) {
		return new Closeable(drawable, SWT.NONE);
	}
...

Users can then change their code from

GC gc = new GC(drawable);
// draw on gc
gc.dispose();

to

try (var gc = GC.create(drawable) {
  // draw on gc
}

without being disturbed at any existing call to new GC().

Instead of GC#create() we could just make the constructor of GC.Closeable public, but I didn't like it that much:

try (var gc = new GC.Closeable(drawable) {
  // draw on gc
}

What do you think?

If in the future we have the impression that GC#create() is used enough, we could then still deprecate its public constructor and/or just implement AutoClosable directly. But that's a future discussion.

Please take a look at 37519e2 . What do you think? I like your proposal. Is this change downward compatible?

[HannesWell]

Please take a look at 37519e2 . What do you think? I like your proposal. Is this change downward compatible?

Thanks, added my review in the PR. And yes since we only add a new method and class that change is fully compatible.