search

eclipse / nebula

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

Replace usage of internal SWT TypedListener and SWTEventListener #583

Closed HannesWell closed 3 weeks ago

HannesWell commented 3 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.

Leverage new methods introduced in https://github.com/eclipse-platform/eclipse.platform.swt/pull/1112 in order to avoid the usage of that two classes. I don't know if there is a specific backwards compatibility policy in Nebula regarding SWT, but at the moment the new methods are only available in Eclipse I-builds. I guess therefore this change cannot be applied immediately. In order to demonstrate it works I changed temporarily changed the target-platform of the build.

@lcaron could you please have a look at this one?

@Phillipus for your information (if you want to review this too, please do so).

laeubi commented 3 months ago

I guess therefore this change cannot be applied immediately.

Yes it will at least require a new SWT release

HannesWell commented 3 months ago

I guess therefore this change cannot be applied immediately.

Yes it will at least require a new SWT release

Yes.

I also want to mention that, even if SWT will eventually deprecate and (re-)move the SWTEventListener and TypedListener it will not happen before at least the two year deprecation period has passed. So it would also be possible to await the actual removal before submitting this PR (although I wouldn't mind to submit it earlier :) ) in order to avoid using tricks.

lcaron commented 3 months ago

Thank you @HannesWell . It seems to be a big change. @wimjongman what do you think about this proposal ?

wimjongman commented 3 months ago

Thank you @HannesWell . It seems to be a big change. @wimjongman what do you think about this proposal ?

When the method is officially released, we can take it.

HannesWell commented 1 month ago

Thank you @HannesWell . It seems to be a big change. @wimjongman what do you think about this proposal ?

When the method is officially released, we can take it.

It was just released today. I rebased the branch and resolved all conflicts.

merks commented 1 month ago

@wimjongman @lcaron

Any objections to merging this now?

wimjongman commented 3 weeks ago

Thanks, Hannes!

HannesWell commented 3 weeks ago

Thanks for your review!