Replace internal SWTEventListener directly with java.util.EventListener

[HannesWell]

SWTEventListener was introduced to provide support for both J2SE and J2ME platforms. But SWT does not support the J2ME for over a decade now and other development artifacts have also been removed for a while already.

Leftover of https://bugs.eclipse.org/bugs/show_bug.cgi?id=483638

See also #1011 (comment) ff.

[HannesWell]

I wonder if we should move this class into the o.e.swt.internal package if it is meant to be internal?
As far as I can tell technically it should be possible.
There are ~5 direct references in the SDK and I'll check if they can be replaced with API.

Does anybody has objections?

[laeubi]

I won't touch this, even though it is marked as such it is visible to client and possibly break things.

[iloveeclipse]

Yes, I have objections. In my workspace I see our internal code using that in tests and also in 3rd party libraries like swtchart & cdt. Of course no one reads javadoc saying "don't use me!" on a public class, but moving it now would be not nice...

[HannesWell]

I'm fine with either way. I wouldn't have a problem with moving, but I also have no strong preference to do it, so I'll keep it as it is if that's your preference.
I will just add the api-filters to suppress the error due to adding the javadoc-tags (without that API-tools complains that a protected field of a API class has changed its type).

[iloveeclipse]

Could we merge this after merging API checks enablement in SWT?
In the IDE we can only see API errors for the current platform.

This should not show any error on any platform.

[github-actions]

Test Results

   299 files     299 suites   6m 19s ⏱️
 4 099 tests  4 091 ✅  8 💤 0 ❌
12 209 runs  12 134 ✅ 75 💤 0 ❌

Results for commit 1dcd8de.

♻️ This comment has been updated with latest results.

[vogella]

Thanks for working on this, I tried the same in the past but never finished it (forgot why).

[HannesWell]

Finally we have a successful build and are down to one API issue left.

Unfortunately I noticed that the API verification is often unstable and fails with:

/home/jenkins/agent/workspace/eclipse.platform.swt_PR-1101/eclipse.platform.swt@tmp/durable-56fcc512/script.sh: line 2: 420 Killed mvn clean verify --batch-mode -V -U -e -DforkCount=0 -Papi-check -Dcompare-version-with-baselines.skip=false -Dorg.eclipse.swt.tests.junit.disable.test_isLocal=true -Dmaven.test.failure.ignore=true -Dmaven.test.error.ignore=true

@laeubi do you have any idea why this happens?

[Phillipus]

This broke our App.

We use the Nebula widget org.eclipse.nebula.jface.gridviewer.GridTableViewer and now we get:

java.lang.NoClassDefFoundError: org/eclipse/swt/internal/SWTEventListener
	at org.eclipse.nebula.jface.gridviewer.GridTableViewer.<init>(GridTableViewer.java:134)

Any suggestions about what to do about this?

[HannesWell]

Any suggestions about what to do about this?

I would suggest to ask Nebula to do the same like in this PR, if it is possible.

[Phillipus]

I would suggest to ask Nebula to do the same like in this PR, if it is possible.

That's impractical.

This change breaks apps and libraries that reference SWTEventListener.

For us it means the end of the road for our RCP app that has been around for 14 years. It means that we will never be able to target the latest version of SWT and Eclipse.

This is a breaking change that marks the end of our product.

I knew this day would come.

[iloveeclipse]

@HannesWell : can we revert the change? I see I saw the reference in swtchart and cdt too before and had objections, I believe my secomd comment was misleading.

[Phillipus]

This is one of those cases where it would be nice to remove old code but the consequences are devastating.

Yes, SWTEventListener is internal and the doc says "This interface is not intended to be referenced by clients", but unfortunately it is the case that it is referenced and we don't know how many more existing apps/libraries reference it directly or indirectly.

We use the Nebula widget in our RCP app. It's unlikely that it will be updated to accommodate this change. So our options are:

• Remove all features of our app that use the Nebula widget (and hope that there are not other libraries that reference SWTEventListener)

or

• Remain on Eclipse 4.31 as target for our RCP app forever

Perhaps it might be better to revert this for now and think about it?

[Phillipus]

Also the Nebula Gallery widget references SWTEventListener somewhere. Sadly, that definitely is the end of the road for our product.

[laeubi]

That's impractical.

Why you can still sue old nebula with old swt, and if one updates also remove the reference?

can we revert the change?

I would instead keep the interface, mark it as deprecated and for removal so it can be deleted some time later.

[laeubi]

Killed mvn clean verify

That means the agent get killed because of to high memory demands.

[Phillipus]

Why you can still sue old nebula with old swt, and if one updates also remove the reference?

That's not how it works.

An RCP application such as ours targets an Eclipse version. We currently target Eclipse 4.31. With this change it means that we cannot target Eclipse 4.32 and greater. It means our product is frozen in time.

[laeubi]

That's not how it works.

That is exactly how it works, SWT / Platform is not responsible for the technical dept of other projects or their misuse of (non) API ... so at some point in time one can simply not use anymore "latest something" with "oldest other"...

With this change it means that we cannot target Eclipse 4.32 and greater. It means our product is frozen in time.

That's exactly why there are releases, so one can stay at an older one, otherwise having releases is quite useless.

[Phillipus]

I've been an Eclipse developer for 20 years. Today marks my final day. Our product Archi has been available for 14 years with 2000 downloads a week. Sadly, this breaking change is the end of the road for us.

[iloveeclipse]

@Phillipus : please don’t panic. I believe the right way is to revert the change and work on active depreciation of this interface. Means - contact cross-project-issues-dev and directly related projects (swtchart, cdt, nebula) and help them to remove the code.

[merks]

Yes, we should avoid knowingly causing damage to our community.

[HannesWell]

I have to say it's a bit frustrating if not even internal classes can be removed, but I guess for everyone involved.

@Phillipus : please don’t panic. I believe the right way is to revert the change and work on active depreciation of this interface. Means - contact cross-project-issues-dev and directly related projects (swtchart, cdt, nebula) and help them to remove the code.

Yes, we'll find a solution for this, no worries.
And thanks for the quick feedback! It's much better to get such feedback immediately than two months later.

It would be great if we can avoid reverting the second commit. If it's done somebody has to add countless api-filters due to the API leakages.
I'll have a look at the mentioned projects tomorrow to see how SWTEventLister is used exactly and report back.

[HannesWell]

Killed mvn clean verify

That means the agent get killed because of to high memory demands.

That story again. Thanks.
Weren't there some work around to permit more memory? I forgot the issues where it has been discussed.

[Phillipus]

It's not always the case that a downstream library class is directly referencing SWTEventLister, so it's not "misuse of API". A compiled library such as Nebula can reference TypedListener and this has internal references to SWTEventLister.

[HannesWell]

It's not always the case that a downstream library class is directly referencing SWTEventLister, so it's not "misuse of API". A compiled library such as Nebula can reference TypedListener and this has internal references to SWTEventLister.

Ok, I see. The problem is that the change in TypedListener is binary incompatible in this case although it is source-compatible. But the latter does not help with already compiled jars.
According to the doc TypedListener is also not part of the SWT API, but I agree that it has not been obvious. It's a mess.
Ideally the usage of TypedListeneris replaced.

[Phillipus]

Ok, I see. The problem is that the change in TypedListener is binary incompatible in this case although it is source-compatible. But the latter does not help with already compiled jars.

Exactly.

According to the doc TypedListener is also not part of the SWT API

I just gave one example. The other listeners also reference SWTEventLister.

[HannesWell]

According to the doc TypedListener is also not part of the SWT API

I just gave one example. The other listeners also reference SWTEventLister.

Yes, but (depending on the exact use-case) I assume that the removal of the extension of SWTEventListener is not that problematic since the using projects can just replace it by the java.util.EventListener (and that would work also with older versions of SWT).

The more difficult part is probably the changed method signatures of new TypedListener (EventListener listener), EventListener getEventListener() and maybe Widget.removeListener (int eventType, EventListener listener) as this is a binary incompatible change so one has to recompile the class files and it can therefore only work for 'old' or 'new' SWT (which makes it difficult in SimRel if projects compile against the latest release and not I-builds).
All these methods are internal but at least TypedListener seems to be used relatively often, even in the Eclipse SDK...
Ideally all usages of TypedListener are replaced by SWT API, but I assume there is a reason why it was not done in the past, so it is probably not trivial. As last resort, to overcome the binary incompatibility one could use reflection to call these methods. Then only 'source' compatibility would be relevant at runtime.

As said I'll have a detailed look at the affected projects (Nebula, CDT, SWTForms? and I found RCPTT also uses SWTEventListener) in detail. I hope that, if at all, it would be sufficient (as it was said before) to restore the SWTEventListener and the few methods accepting it (but I'm in doubt it works at runtime if a listener not implementing SWTEventListener is passed to a method expecting a SWTEventListener, even if it delegates to a EventListener overload).

The good news is all affected projects I know of so far had activity recently so I think there is a good chance we are able to adjust them and we have a lot of time until the next release to resolve this problem. :)

[iloveeclipse]

See the amount of troubles, we need a revert: https://download.eclipse.org/eclipse/downloads/drops4/I20240312-1800
@HannesWell : could you work on this today, or should I try to revert?

[laeubi]

It's not always the case that a downstream library class is directly referencing SWTEventLister, so it's not "misuse of API". > A compiled library such as Nebula can reference TypedListener and this has internal references to SWTEventLister.

They can not

eclipse.platform.swt/bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/widgets/TypedListener.java

Lines 27 to 30 in bd306ac

	* <b>IMPORTANT:</b> This class is <em>not</em> part of the SWT
	* public API. It is marked public only so that it can be shared
	* within the packages provided by SWT. It should never be
	* referenced from application code.

to be 100% sure I have already recommended to keep the interface for some time just in case someone is really referencing that (what is forbidden) or referencing TypedListener (what is forbidden).

Just to clarify: There might be cases were it is required / desired to break this rule but such libraries are tightly coupled to the SWT release, what means they can't be compatible to any older or any never release of SWT they are build with. That is the same as you usually can't randomly combine any newer or older release of platform bundles, JDT, PDE, ...

[laeubi]

See the amount of troubles,

What amount? Do you mean the Jface bundle needs a touch? Thats exactly such case where JFace is tightly coupled to SWT so I would assume this is "normal".

[iloveeclipse]

I've asked Hannes, whether I should help or not.

[HannesWell]

See the amount of troubles, we need a revert: https://download.eclipse.org/eclipse/downloads/drops4/I20240312-1800

Are you referring to the comparator errors or test-failure or both? The latter is probably/maybe caused by the former.

@HannesWell : could you work on this today, or should I try to revert?

I can work on this this evening, if you or somebody else wants to do it earlier, the second commit of this PR should be reverted and then one has to add the countless API-filters to suppress the API-leak issue, for all seven fragments (one can probably copy past the latter part).
But to be honest I would be in favor fixing the callers in JFace and platform.ui, either by just touching them or even moving them away of using TypedListener (which is probably the main cause for the trouble). Also on that I can work this evening, but would appreciate any help if you Andrey or somebody else wants/can help on it. :)
I still have the hope that we can just go forward.

[jukzi]

This Change is not binary compatible see NoSuchMethodError #1105

[iloveeclipse]

I've pushed #1106 as draft now.

[iloveeclipse]

@Phillipus : I've pushed revert.
If you have access to Mac, could you update to latest master state, open SWT Mac native fragment project in the IDE (must be most recent stable build), run clean build and check for API warnings? If there would be some, please select them in Problems view, reght click -> quick fix -> add not commented filter and push the PR with the changed api filters file?

[Phillipus]

I've pushed revert.

Thank-you. :-)

If you have access to Mac, could you update to latest master state, open SWT Mac native fragment project in the IDE (must be most recent stable build), run clean build and check for API warnings?

I need guidance on how to do this.

What API baseline do I set? I have SWT projects in my workspace as cloned from GitHub. My current target is 4.31 latest I-build.

[iloveeclipse]

In Preferences -> Plugin Development -> API baselines click on "Add baseline..."

Click "Add existing installation directory"

Point to the extracted 4.31 RC2 build from https://download.eclipse.org/eclipse/downloads/drops4/S-4.31RC2-202402290520/

Select this new entry now

and say "Apply and Close".

After that "Clean Build" SWT fragment for Mac just to make sure there are no incremental build artifacts.

[Phillipus]

All these methods are internal but at least TypedListener seems to be used relatively often, even in the Eclipse SDK...
Ideally all usages of TypedListener are replaced by SWT API, but I assume there is a reason why it was not done in the past, so it is probably not trivial.

A search through SWT code, the Nebula project, and other projects in GitHub shows that the following pattern is very popular:

public void addSelectionListener(SelectionListener listener) {
    TypedListener typedListener = new TypedListener (listener);
    addListener (SWT.Selection,typedListener);
    addListener (SWT.DefaultSelection,typedListener);
}

Might be better to make TypedListener API. ;-)

[HannesWell]

Thanks Andrey for helping in reverting.

All these methods are internal but at least TypedListener seems to be used relatively often, even in the Eclipse SDK...
Ideally all usages of TypedListener are replaced by SWT API, but I assume there is a reason why it was not done in the past, so it is probably not trivial.

A search through SWT code, the Nebula project, and other projects in GitHub shows that the following pattern is very popular:

public void addSelectionListener(SelectionListener listener) {
    TypedListener typedListener = new TypedListener (listener);
    addListener (SWT.Selection,typedListener);
    addListener (SWT.DefaultSelection,typedListener);
}

Yes I noticed this as well while investigating the usage further. That and TypedListener .getEventListener() are the main if not the only reason why TypedListener is used.

Might be better to make TypedListener API. ;-)

I think I have found a better alternative. I'll push it soon, stay tuned. :)

[HannesWell]

Might be better to make TypedListener API. ;-)

I think I have found a better alternative. I'll push it soon, stay tuned. :)

As I still belive we should work towards removing SWTEventListener and to make TypedListener really internal, but in an ordered fashion I created #1112 to provide an alternative API to do that.
Everybody interested, please participate.