introduce static helper method GC#create #1063

tobiasmelcher · 2024-02-26T07:44:12Z

which takes care of creation and disposal of the GC object

Fixes: #955

tobiasmelcher · 2024-02-26T08:05:11Z

@HannesWell : could you please take a look at this change? I had to introduce a new Runnable interface which passes the GC as argument. Is there already an existing interface I could re-use? Do we have to introduce the static method GC#drawOn in all platform specific GC implementations?

github-actions · 2024-02-26T08:22:23Z

Test Results

176 files ±0 176 suites ±0 28m 53s ⏱️ + 1m 24s
4 693 tests ±0 4 671 ✅ ±0 22 💤 ±0 0 ❌ ±0
489 runs ±0 483 ✅ ±0 6 💤 ±0 0 ❌ ±0

Results for commit 4ab1b13. ± Comparison against base commit 9c79cf6.

♻️ This comment has been updated with latest results.

HannesWell · 2024-03-23T10:49:05Z

Hello @tobiasmelcher, thank you for this PR. Sorry for my late reply but I just started my almost three week of vacation when you created this PR and when I came back a full TODO list was ready to be worked off.

Do we have to introduce the static method GC#drawOn in all platform specific GC implementations?

Yes we do, but you can just import/open the fragments for the other platforms and they will compile in your workspace. For you it is probably sufficient to only have one arch per OS since they only differ in the embedded native binaries, but the code is the same for all archs on one OS.

I had to introduce a new Runnable interface which passes the GC as argument. Is there already an existing interface I could re-use?

Yes there is already SWTRunnable. But as I have just written in #955 (comment), I think this is not the optimal solution, which I only noticed when reviewing your changes to apply the new method (applying it immediately can therefore be very helpful 👍🏽 )

HannesWell

Thanks for the update. In general it looks good, but I added some remarks below.

But the most important missing part is the implementation for the linux- and mac-platform as written in my previous comment.

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/GC.java

...g.eclipse.swt/Eclipse SWT Custom Widgets/common/org/eclipse/swt/custom/AnimatedProgress.java

...les/org.eclipse.swt/Eclipse SWT Custom Widgets/common/org/eclipse/swt/custom/CTabFolder.java

tobiasmelcher · 2024-03-28T22:10:25Z

Thanks a lot @HannesWell for all the feedback. Could you please take a look and review my latest change eb1dc57 ? As already mentioned, I had some troubles with the formatting; I executed then the format action only on the changed coding parts. Is that ok?

HannesWell

As already mentioned, I had some troubles with the formatting; I executed then the format action only on the changed coding parts. Is that ok?

Yes, that's IMO the perfect decision.

Could you please also update the doc of the GC class to mention the new GC.create() factory (for all platforms).

With that I think this change is complete (I'll mark it as ready for review), but I would like give others some time to review.

HannesWell · 2024-03-29T13:02:14Z

bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/graphics/GC.java

 * @see <a href="http://www.eclipse.org/swt/">Sample code and further information</a>
 */
-public final class GC extends Resource {
+public sealed class GC extends Resource {


It would be good to also update the doc of this class (where it mentions GC.dispose()) and recommend to use the new factory, ideally with the little example also used in the Image class.
And I think Windows95 and Windows98 are not the most relevant target OS anymore. Maybe that just should be generalized.

HannesWell · 2024-03-29T13:05:06Z

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java

+ *    try (var gc = GC.create(i)) {
+ *      gc.drawRectangle(0, 0, 50, 50));
+ *    }


Please apply this doc update also to the Image implementations for Linux and Mac.

HannesWell · 2024-03-30T11:28:11Z

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/GC.java

+		}
+	}
+
+	public static GC.Closeable create(Drawable drawable) {


The new method needs some java-doc on all platforms. The existing single argument constructor of GC is probably a good source of inspiration.
The existing GC constructor could also be adjusted to recommend this method for safety.

What's important is to add the tag @since 3.126 in the Java-doc in order to make the API-tools aware this method was just added.

Thanks for the hint. I hope it is fine with 6443eca

HannesWell · 2024-03-30T11:29:45Z

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/GC.java

-public final class GC extends Resource {
+public sealed class GC extends Resource {
+
+	public static final class Closeable extends GC implements AutoCloseable {


This new class needs some java-doc on all platforms. I think a short text that mentions that this is an auto-closable extension of the GC class is sufficient. The create method could be linked with @see.
What's important is to add the tag @since 3.126 in the Java-doc in order to make the API-tools aware this method was just added.

I just stumbled upon this again and thought if it would be better to name this
AutoDisposable? Since it's an extension of AutoClosable that just calls dispose().

Nit-pick this does not extends AutoClosable (what is an interface) but implements it.

It also feels like AutoDisposable actually should be an interface so others can implement it as well.

And then it probabbly should become AutoDisposableGC here...

renamed to AutoDisposableGC with [326fd48]

tobiasmelcher · 2026-02-09T08:47:08Z

Can anyone give guidance how to solve the build error:

Error:  Failed to execute goal org.eclipse.tycho:tycho-apitools-plugin:5.0.2:verify (verify) on project org.eclipse.swt.gtk.linux.ppc64le: There are API errors:
Error:  The field org.eclipse.swt.graphics.GC.handle has been added to a class
Error:  META-INF/MANIFEST.MF:1 The major version should be incremented in version 3.133.0, since API breakage occurred since version 3.132.0

I assume that changing the final class GC to a sealed class introduces the API breakage? So, the current idea and approach will not work?

merks · 2026-02-09T08:55:20Z

The message says The field org.eclipse.swt.graphics.GC.handle has been added to a class which seems bogus, so I kind of doubt the issue is related to sealed. For starters, I would rebase this on master before anything else.

merks · 2026-02-09T09:04:00Z

These are pretty clear and indicate you have the wrong values, so change that as well.

tobiasmelcher · 2026-02-09T09:35:54Z

Should I really change to 4.0.0 ? I haven't changed GC.handle. The message irritates me.

org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal org.eclipse.tycho:tycho-apitools-plugin:5.0.2:verify (verify) on project org.eclipse.swt.cocoa.macosx.aarch64: There are API errors:
The field org.eclipse.swt.graphics.GC.handle has been added to a class
META-INF/MANIFEST.MF:1 The major version should be incremented in version 3.133.0, since API breakage occurred since version 3.132.0
org.eclipse.swt.graphics.GC.handle declared as non-API type org.eclipse.swt.internal.cocoa.NSGraphicsContext
`´`

HeikoKlare · 2026-02-09T10:33:59Z

Should I really change to 4.0.0 ? I haven't changed GC.handle. The message irritates me.

There is definitely no need for bumping to 4.0.0 here. I also don't think it related to making the class sealed, as we did something similar with the Point/Rectangle classes several releases ago and did not have this kind of error: #1711
It might be fine to just add an API filter to silence the error.

One thing I would like mention is that making a final class sealed will probably somehow break compatibility with consumers using Kotlin (see #1711 (comment)). In my opinion, that's a flaw of the Kotlin language, still we might see issues being raised for SWT from Kotlin developers using SWT and instantiating a GC.

merks · 2026-02-09T10:47:25Z

It looks more like an API tools bug. I can't reproduce the error in my IDE so I'm not sure what we need to add to suppress the message. I can reproduce an error in the IDE only if I add a new public field:

@laeubi @HannesWell

Any ideas?

merks · 2026-02-09T10:59:38Z

It might/probably would work to add a filter like this:

Like this:

    <resource path="Eclipse SWT/cocoa/org/eclipse/swt/graphics/GC.java" type="org.eclipse.swt.graphics.GC">
        <filter comment="Bogus problem" id="336658481">
            <message_arguments>
                <message_argument value="org.eclipse.swt.graphics.GC"/>
                <message_argument value="handle"/>
            </message_arguments>
        </filter>
    </resource>
```

tobiasmelcher · 2026-02-09T12:40:38Z

Could we let the GC class implement AutoCloseable? So, there would be no need to change from final to sealed? Do you see any issues by doing it this way?

merks · 2026-02-09T12:49:40Z

Could we let the GC class implement AutoCloseable?

I wondered that as well. That seems simpler to me...

HeikoKlare · 2026-02-09T13:23:39Z

Could we let the GC class implement AutoCloseable? So, there would be no need to change from final to sealed? Do you see any issues by doing it this way?

I would expect a bunch of resource leak warnings because of all existing creators of GC instances not closing them.

I am not sure if that was discussed already, but an alternative to an AutoClosable might be a kind of execute method that just takes a Drawable and a lambda that is executed on a temporary GC for that drawable. Then you could wrap everything inside that method instead of adapting the type hierarchy.
The AutoClosable solution is in my opinion definitely more elegant, so just consider it an alternative in exploring the design space which might avoid some of the drawbacks with the AutoClosable solution.

tobiasmelcher · 2026-02-09T13:54:54Z

I am not sure if that was discussed already, but an alternative to an AutoClosable might be a kind of execute method that >>just takes a Drawable and a lambda that is executed on a temporary GC for that drawable. Then you could wrap >>everything inside that method instead of adapting the type hierarchy.

The very first proposal was implemented in this way.
https://github.com/eclipse-platform/eclipse.platform.swt/compare/d4fb76fdc63bf1d38b2b834dad679f7dc6f04e29..37519e2ade4acb878e50c8a78a9931524707854f

Heiko, I get from your comment that implementing AutoClosable in GC would lead to a big list of "resource leak warnings" which is not what we want?

HeikoKlare · 2026-02-09T14:10:44Z

The very first proposal was implemented in this way. https://github.com/eclipse-platform/eclipse.platform.swt/compare/d4fb76fdc63bf1d38b2b834dad679f7dc6f04e29..37519e2ade4acb878e50c8a78a9931524707854f

I see. Then I don't want to open up that discussion again.

Heiko, I get from your comment that implementing AutoClosable in GC would lead to a big list of "resource leak warnings" which is not what we want?

Yes, every call to new GC(...) will give you this:

In the Eclipse platform repos, this would already lead to around 130 of such warnings.

which takes care of creation and disposal of the GC object Fixes: eclipse-platform#955

tobiasmelcher requested review from iloveeclipse and niraj-modi as code owners February 26, 2024 07:44

tobiasmelcher marked this pull request as draft February 26, 2024 07:47

BeckerWdf requested a review from HannesWell February 26, 2024 08:03

BeckerWdf force-pushed the GC.drawOn branch from b27a84d to d4fb76f Compare February 26, 2024 08:16

HannesWell mentioned this pull request Mar 23, 2024

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

Open

tobiasmelcher force-pushed the GC.drawOn branch from d4fb76f to 37519e2 Compare March 28, 2024 17:31

HannesWell requested changes Mar 28, 2024

View reviewed changes

HannesWell reviewed Mar 29, 2024

View reviewed changes

HannesWell marked this pull request as ready for review March 29, 2024 13:38

HannesWell requested review from lshanmug and sravanlakkimsetti as code owners March 29, 2024 13:38

HannesWell force-pushed the GC.drawOn branch from eb1dc57 to 573159d Compare March 29, 2024 17:46

HannesWell reviewed Mar 30, 2024

View reviewed changes

tobiasmelcher force-pushed the GC.drawOn branch 2 times, most recently from eb1dc57 to 6443eca Compare April 2, 2024 07:01

fedejeanne changed the title ~~introduce static helper method GC#drawOn~~ introduce static helper method GC#create Jan 22, 2025

fedejeanne mentioned this pull request Jan 22, 2025

Identification of Long-Living (Potential Not-DPI-Updated) GCs vi-eclipse/Eclipse-Platform#89

Closed

HeikoKlare mentioned this pull request Feb 6, 2025

Flicker in ControlExample because of Button swt-initiative31/prototype-skija#60

Closed

tobiasmelcher force-pushed the GC.drawOn branch from 6443eca to 326fd48 Compare February 6, 2026 17:40

tobiasmelcher force-pushed the GC.drawOn branch 2 times, most recently from 782fcbc to ed45377 Compare February 9, 2026 09:14

tobiasmelcher force-pushed the GC.drawOn branch 2 times, most recently from 66d72ba to bc0ad9a Compare February 10, 2026 08:51

introduce AutoDisposableGC

4ab1b13

which takes care of creation and disposal of the GC object Fixes: eclipse-platform#955

tobiasmelcher force-pushed the GC.drawOn branch from bc0ad9a to 4ab1b13 Compare February 10, 2026 13:13

introduce static helper method GC#create #1063

Are you sure you want to change the base?

introduce static helper method GC#create #1063

Conversation

tobiasmelcher commented Feb 26, 2024

Uh oh!

tobiasmelcher commented Feb 26, 2024

Uh oh!

github-actions bot commented Feb 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

HannesWell commented Mar 23, 2024

Uh oh!

HannesWell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tobiasmelcher commented Mar 28, 2024

Uh oh!

HannesWell left a comment

Choose a reason for hiding this comment

Uh oh!

HannesWell Mar 29, 2024

Choose a reason for hiding this comment

Uh oh!

HannesWell Mar 29, 2024

Choose a reason for hiding this comment

Uh oh!

HannesWell Mar 30, 2024

Choose a reason for hiding this comment

Uh oh!

tobiasmelcher Apr 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HannesWell Mar 30, 2024

Choose a reason for hiding this comment

Uh oh!

HannesWell Feb 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laeubi Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

tobiasmelcher Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

tobiasmelcher commented Feb 9, 2026

Uh oh!

merks commented Feb 9, 2026

Uh oh!

merks commented Feb 9, 2026

Uh oh!

tobiasmelcher commented Feb 9, 2026

Uh oh!

HeikoKlare commented Feb 9, 2026

Uh oh!

merks commented Feb 9, 2026

Uh oh!

merks commented Feb 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tobiasmelcher commented Feb 9, 2026

Uh oh!

merks commented Feb 9, 2026

Uh oh!

HeikoKlare commented Feb 9, 2026

Uh oh!

tobiasmelcher commented Feb 9, 2026

Uh oh!

HeikoKlare commented Feb 9, 2026

Uh oh!

Reviewers

Assignees

Labels

github-actions bot commented Feb 26, 2024 •

edited

Loading

tobiasmelcher Apr 2, 2024 •

edited

Loading

HannesWell Feb 6, 2026 •

edited

Loading

merks commented Feb 9, 2026 •

edited

Loading