Do not call addListener in constructors of CCombo and StyledText #2733

[fedejeanne]

The method is not final, which may cause issues if a subclass overrides it because it ends up being called upon instantiation, when the object is not fully initialized

Since these 2 classes are not in the same package as other subclasses of Widget, I had to resort to expanding the functionality in TypedListener.

Fixes #2733

[github-actions]

Test Results

  118 files  ±0    118 suites  ±0   16m 6s ⏱️ - 1m 33s
4 651 tests ±0  4 633 ✅  - 1  18 💤 +1  0 ❌ ±0 
  330 runs  ±0    326 ✅ ±0   4 💤 ±0  0 ❌ ±0 

Results for commit ad8479c. ± Comparison against base commit 136b375.

This pull request skips 1 test.

org.eclipse.swt.tests.junit.Test_org_eclipse_swt_browser_Browser_IE ‑ test_setUrl_remote_with_post

♻️ This comment has been updated with latest results.

[fedejeanne]

Need to rethink this since the replacement is also official API and can therefore also be overridden

[fedejeanne]

I've added 2 internal-API final methods to avoid calling the public addListener(int, Listener) and to not have to change the existing (package-friendly, non-final) _addListener(int, Listener) because that would be a breaking change. It would technically be a breaking change on a non-API method but I decided to go with the new method anyway.

FTR the calls that I am correcting in this PR have been introduced recently:

• Design improvement for scaling win32 widgets #2483

[HeikoKlare]

Maybe an unconventional thought on avoiding such a new API for just a very specific use case: what about wrapping the addListener calls into a display.asyncExec()? I think it's not necessary that the listeners are registered synchronously, so that could move it out of the constructor to avoid the potential problems with overwritten methods as well.

[fedejeanne]

In principle it could work but I fear that it would mean that one needs to check for disposal when overriding the public addListener(...) method. It seems obscure, I would like to avoid imposing implementation burdens on consumers, if possible.

[HeikoKlare]

I fear that it would mean that one needs to check for disposal when overriding the public addListener(...) method.

Why should that be necessary? You would check for disposal before calling addListener as common in asyncExec calls.

[fedejeanne]

hm. on a second thought: consumers should be checking for disposal already. I mean the original addListener does that already via checkWidget().

eclipse.platform.swt/bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Widget.java

Lines 231 to 235 in 136b375

	public void addListener (int eventType, Listener listener) {
	checkWidget();
	if (listener == null) error (SWT.ERROR_NULL_ARGUMENT);
	_addListener (eventType, listener);
	}

You would check for disposal before calling addListener as common in asyncExec calls.

You're right, that would be easily solvable on my side by calling checkWidget().

I'll adapt my PR with your suggestion 👍

[fedejeanne]

Adapted

[HeikoKlare]

Except for the widget check, the change looks sound to me. @akoch-yatta what do you think? Do we miss anything?