Rework SplitButton to not use SWT's internal TypedListener #582
Conversation
| return evt; | ||
| }; | ||
| for (Listener listener : selectionListeners) { | ||
| listener.handleEvent(eventFactory.get()); |
There was a problem hiding this comment.
Actually it looks wrong to create a new event for each listener, @lcaron what do you think?
There was a problem hiding this comment.
It used to be like that, this is why I replicated it.
But when looking at SWT I get the impression that there is only one Event send to all listeners, unless one uses a typed listener (e.g. SelectionListener), then the one event is wrapped in a new SelectionEvent for each SelectionListener, see TypedListener. But the latter is probably a technical detail.
And although Events are not immutable, I think good listeners don't change events.
There was a problem hiding this comment.
Except for "doit" when the event has that sort of thing.
There was a problem hiding this comment.
Except for "doit" when the event has that sort of thing.
Didn't knew that, thanks for the hint. So maybe there should be one Event send to all Listeners and a new SelectionEvent for each SelectionListener?
There was a problem hiding this comment.
I think that's how SWT generally does it...
There was a problem hiding this comment.
And although Events are not immutable, I think good listeners don't change events.
Actually listeners can give some things back to the caller (e.g. as mentioned doit), there is even a field data mentioned for "application use" in TypedEvent, I think if one can install listeners to an SWT object one should always be "nice" and only ever modify things if the API tells so.
|
Unless bounds are changed that will allow this latest Nebula bundle to only installed along with latest version of SWT, this type of change is going to cause grief. If it were "my project" I would probably try to create a utility that allowed me to work reflectively with both the old "APIs" and the new APIs so that these things continue to "just work". |
This change does not use the recent API addition in SWT (as you can see in the CI it builds with the latest SimRel) and just avoids
The new APIs are only used in #583, but there I raised the version bounds. But lets discuss there if any other strategy should be used. |
|
Sorry, I didn't look in detail of what's being used! |
HannesWell
force-pushed
the
rework-splitbutton
branch
2 times, most recently
from
f806c5f to
81bba97
Compare
|
@lcaron, @wimjongman can you please review this as well? |
...lipse.nebula.widgets.splitbutton/src/org/eclipse/nebula/widgets/splitbutton/SplitButton.java
Outdated
Show resolved
Hide resolved
HannesWell
force-pushed
the
rework-splitbutton
branch
from
81bba97 to
ea4438c
Compare
I'm not familiar with Nebula in general and the |
HannesWell
force-pushed
the
rework-splitbutton
branch
from
ea4438c to
52365cf
Compare
|
@wimjongman is there anything missing here or can this be submitted? |
|
Go for it. Thanks, Hannes! |
I'm not a committer here, but thanks Ed for submitting. |
You get a special mention when we release. 🏅 |
The
org.eclipse.swt.widgets.TypedListenershould be considered an internal class (as the doc states) and leaks the classSWTEventListenerwhich also resides in an internal package.This reworks
SplitButtonto not useTypedListeneranymore.