Add API to add and obtain typed listeners to Widgets

[HannesWell]

One of the two main, if not the only reasons why the actually (according to its doc) internal SWT type TypedListener and consequently also the internal type SWTEventListener is referenced in client code is to register typed listeners in custom widgets.

One example in the Eclipse SDK (outside SWT) is:
https://github.com/eclipse-platform/eclipse.platform/blob/c74c80065aa91f5e2f03377aae1cd80928e48534/ua/org.eclipse.help.ui/src/org/eclipse/help/ui/internal/HyperlinkLabel.java#L157-L164

The second pattern why TypedListener is used is to obtain the initially registered typed listeners from a widget:
https://github.com/eclipse-platform/eclipse.platform/blob/c74c80065aa91f5e2f03377aae1cd80928e48534/team/bundles/org.eclipse.compare/compare/org/eclipse/compare/contentmergeviewer/TextMergeViewer.java#L3109-L3119

This pattern is used very often as one can see for example in the repositories of the eclipse organisation or CDT (and probably in more):

• https://github.com/search?q=org%3Aeclipse-cdt+org.eclipse.swt.widgets.TypedListener&type=code
• https://github.com/search?q=org%3Aeclipse+org.eclipse.swt.widgets.TypedListener++language%3AJava&type=code

This PR suggests to add two new methods to the Widget class to add and obtain typed listeners, which can act as API replacement for the patterns mentioned above and also significantly reduces code-duplications in SWT's own code base (see second commit):

• protected void addTypedListener(EventListener listener, int... eventTypes)
• public EventListener[] getTypedListeners(int eventType)

With that we can subsequently deprecate SWTEventListener for removal and TypedListener to be moved to an internal package.

[HannesWell]

At the moment I choose to return an array to not mix styles here, but if I could freely choose I would return a List or even a Stream.
Do you have opinions on that?

[github-actions]

Test Results

   299 files  ±0     299 suites  ±0   6m 30s ⏱️ + 1m 17s
 4 099 tests ±0   4 091 ✅ ±0   8 💤 ±0  0 ❌ ±0 
12 209 runs  ±0  12 134 ✅ ±0  75 💤 ±0  0 ❌ ±0 

Results for commit e92eba1. ± Comparison against base commit 7479681.

♻️ This comment has been updated with latest results.

[Phillipus]

Good idea and it gets rid of a lot of duplicate boiler-plate code (null check etc). Thank-you for your work on this.

With that we can subsequently deprecate SWTEventListener for removal and TypedListener to be moved to an internal package.

How do you see that panning out? Our product uses two Nebula widgets and their code makes use of the TypedListener pattern, and so their distributions are compiled with an indirect dependency on SWTEventListener. We would like to continue to use later versions of Eclipse/SWT for our product but cannot if SWTEventListener is removed and Nebula don't update to the new proposed API. I guess the same is true for several other third-party frameworks that use the TypedListener pattern.

Yes, we know that TypedListener is not public API and should not have been used, but here we are in the real world and it would be better not to set the world on fire. :-)

[vogella]

I personally would prefer List over Array but assume for migration to the new API in libraries it might be easier to use arrays.

[laeubi]

Nebula is an open source project and has regular releases and is tightly coupled to SWT/Platform so I don't see why it should not be possible to change to modern SWT patterns....

[HannesWell]

I just updated the code handle custom typed Listeners that omit the SWTEventListener and implement EventListener directly before SWTEventListener is removed. When using the new methods that should be possible now.
I also renamed the new removeListener to removeTypedListener() to match the add/getTypedListener() methods and to ease migration to it (no need to explicitly cast the arguments to avoid calling the old methods).

With that this PR is ready for me.
Does anybody wants to review this in more detail or is everybody fine with the suggestion?
Btw. the PR consists of two commit and the first one adds the new methods while the second apply them in the SWT code-base.

Good idea and it gets rid of a lot of duplicate boiler-plate code (null check etc). Thank-you for your work on this.

Your welcome, glad you like it.

With that we can subsequently deprecate SWTEventListener for removal and TypedListener to be moved to an internal package.

How do you see that panning out? Our product uses two Nebula widgets and their code makes use of the TypedListener pattern, and so their distributions are compiled with an indirect dependency on SWTEventListener. We would like to continue to use later versions of Eclipse/SWT for our product but cannot if SWTEventListener is removed and Nebula don't update to the new proposed API. I guess the same is true for several other third-party frameworks that use the TypedListener pattern.

I agree with Christoph here, from all I see/know about Nebula it should absolutely be possible to migrate it to the new methods and I'm willing to help with that. I'll also inform other projects in sim-rel about this.

Yes, we know that TypedListener is not public API and should not have been used, but here we are in the real world and it would be better not to set the world on fire. :-)

Don't worry I don't plan to set the world on fire, just cautiously try to cut a tree. :)
But I suggest we leave the discussion about the eventual deprecation for a separate issue/PR dedicated to that topic.
Just one more note: In case it turns out the eventual removal is not feasible, one can also un-deprecate an element.

I personally would prefer List over Array but assume for migration to the new API in libraries it might be easier to use arrays.

Yes, I think consistency in the API should be maintained.

[HannesWell]

With that this PR is ready for me.
Does anybody wants to review this in more detail or is everybody fine with the suggestion?

I consider no responses as silent consent from everyone already participating.
@iloveeclipse you were also quite active in #1101, do you plan to review this one any time later?

[HannesWell]

While preparing the first applications of this new API and reviewing this change I again I found that Widget.getTypedListeners() can be further enhanced from
public EventListener[] getTypedListeners (int eventType);
to
public <L extends EventListener> L[] getTypedListeners (int eventType, Class<L> listenerType).
This often avoids subsequent type checks because usally a caller is interested in listeners in that specific type.
If one wants only a generic type it is still possible to just pass EventListener.class (or even Object.class).

What do you think?
If there are no objects or requests for more time to review until tomorrow evening I'll plan to submit this then and apply within the eclipse SDK.

[iloveeclipse]

Please never combine arrays and generics. From my experience this will break something sooner or later.

[merks]

As long as there is a Class argument to create the array it’s probably fine. Otherwise I generally agree.

[mickaelistria]

Please never combine arrays and generics. From my experience this will break something sooner or later.

I agree those can be confusing or error-prone. What about using Lists instead?

[HannesWell]

Please never combine arrays and generics. From my experience this will break something sooner or later.

@iloveeclipse Can you give some examples for this specific case?

I agree those can be confusing or error-prone. What about using Lists instead?

If we are fine to be inconsistent with the existing methods I would just return a Stream<L>.
This would also fit well for use-cases in the eclipse SDK.
I'm more and more keen to that latter solution.

[HannesWell]

I agree those can be confusing or error-prone. What about using Lists instead?

If we are fine to be inconsistent with the existing methods I would just return a Stream<L>. This would also fit well for use-cases in the eclipse SDK.

I have now changed the method to return a Stream<L>.

[laeubi]

If we are fine to be inconsistent with the existing methods I would just return a Stream<L>.

We should not keep styles just because they where there forever, Streams are actually the better return type in any aspect:

1. possibly lazy unless calling a terminal operation
2. callers can filter / iterate or collect as they want including to arrays, lists, sets, ...
3. no need to think about if the returned array / collection is read/write and so on

[HannesWell]

If we are fine to be inconsistent with the existing methods I would just return a Stream<L>.

We should not keep styles just because they where there forever, Streams are actually the better return type in any aspect:

1. possibly lazy unless calling a terminal operation

2. callers can filter / iterate or collect as they want including to arrays, lists, sets, ...

3. no need to think about if the returned array / collection is read/write and so on

Absolutely.
If there are no objections or requests for more time to review, I'll squash the last two commit into the second one and submit this PR this evening.

[mickaelistria]

If we are fine to be inconsistent with the existing methods

We are fine with adopting a newer style for newer code.

[HannesWell]

Great, so let's submit this then.
Thanks everyone for the discussion.