search

eclipse / nebula

Nebula Project
https://eclipse.org/nebula
Eclipse Public License 2.0
85 stars 98 forks source link

Rework SplitButton to not use SWT's internal TypedListener #582

Closed HannesWell closed 5 months ago

HannesWell commented 6 months ago

The org.eclipse.swt.widgets.TypedListener should be considered an internal class (as the doc states) and leaks the class SWTEventListener which also resides in an internal package.

This reworks SplitButton to not use TypedListener anymore.

merks commented 6 months ago

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".

HannesWell commented 6 months ago

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.

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 TypedListener by reworking the listener handling. I assume that's the source of your concern, isn't it? Although I don't know if this is compatible with SWT 1.0, but it shouldn't raise the requirements on the SWT version compared to the previous state.

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".

The new APIs are only used in https://github.com/eclipse/nebula/pull/583, but there I raised the version bounds. But lets discuss there if any other strategy should be used.

merks commented 6 months ago

Sorry, I didn't look in detail of what's being used!

HannesWell commented 6 months ago

@lcaron, @wimjongman can you please review this as well? This only uses already existing SWT API.

HannesWell commented 6 months ago

One remark. Otherwise LGTM. I did not run it so I don't know if the widget works the same way it used to.

I'm not familiar with Nebula in general and the SplitButton especially, but when I run the SplitButtonSnippet everything seem to work fine as far as I understood it.

HannesWell commented 5 months ago

@wimjongman is there anything missing here or can this be submitted?

wimjongman commented 5 months ago

Go for it. Thanks, Hannes!

HannesWell commented 5 months ago

Go for it. Thanks, Hannes!

I'm not a committer here, but thanks Ed for submitting.

wimjongman commented 5 months ago

Go for it. Thanks, Hannes!

I'm not a committer here, but thanks Ed for submitting.

You get a special mention when we release. 🏅