find/replace overlay: clean up PreferenceChangeListener

[Wittmaxi]

The PreferenceChangeListener now starts listening when an Overlay was created and stops listening once the Overlay is disposed of. Fixes a previous resource leak.

fixes jdt#1444

[Wittmaxi]

@jukzi can you please look into this and tell me whether it improves things? When/How are the resource-leak tests ran?

[jukzi]

@jukzi can you please look into this and tell me whether it improves things? When/How are the resource-leak tests ran?

I did not know how to reproduce the test error locally so i can not verify locally if it is solved.
But the idea of the test is that there should be no object leaked - which can be better manually tested with a tool like VisualVm comparing heapDump snapshot before/after

[github-actions]

Test Results

 1 210 files   -   605   1 210 suites   - 605   1h 3m 30s ⏱️ - 27m 37s
 7 636 tests ±    0   7 405 ✅  -     3  231 💤 +  3  0 ❌ ±0 
16 046 runs   - 8 023  15 533 ✅  - 7 787  513 💤  - 236  0 ❌ ±0 

Results for commit 7abb920. ± Comparison against base commit 385b30b.

This pull request skips 3 tests.

UiTestSuite org.eclipse.ui.tests.api.ApiTestSuite org.eclipse.ui.tests.api.WorkbenchPluginTest ‑ testGetImageRegistryFromAdditionalDisplay
org.eclipse.jface.text.tests.contentassist.ContextInformationTest ‑ testContextInfo_hide_focusOut
org.eclipse.urischeme.internal.registration.TestUnitWinRegistry ‑ testWinRegistry

[iloveeclipse]

Wouldn't be better to move overlayDialogPreferenceListener to the FindReplaceOverlay itself? There one could simply add listener in constructor and remove on close().

[Wittmaxi]

I had moved this listener out of the FindReplaceOverlay as a response to @HeikoKlare's comment: #1192 (comment)

I believe that he is right and believe that it is indeed best if the Overlay is independent from the "global parametrization".

[iloveeclipse]

I did not know how to reproduce the test error locally so i can not verify locally if it is solved.

Just start the test from IDE as any other test, it will fail saying that JVM args should be modified, so proceed and add all the javavmargs arguments from eclipse.jdt.ui/org.eclipse.jdt.ui.tests/test.xml to the JVM arguments launch config section.
Eventually you should remove all non-standard bundles from the launch config (e.g. I had to remove spotbugs as the test can't find some class referenced in one of its dependencies).

This change fixes the leak, but as said above, I would prefer the listener would be moved from the action to the overlay class so no memory leak should happen by design.

[Wittmaxi]

@iloveeclipse the leak does NOT happen in the find/replace overlay. The leak happens because I am not notified when the Action is disposed of and since the PreferenceChangeListeners is a field of the Action, I could not know when it was destroyed.

[iloveeclipse]

@iloveeclipse the leak does NOT happen in the find/replace overlay.

Sure, I never said that. What I mean is that if you would move listener from the action to the overlay, no leak would happen.

The leak happens because I am not notified when the Action is disposed of and since the PreferenceChangeListeners is a field of the Action, I could not know when it was destroyed.

Right, but why should action listen to overlay preferences in first place, if it's what the overlay should care about.
@HeikoKlare : not sure why you didn't want listener to be in the overlay class. Could you please check this bug?

[Wittmaxi]

@iloveeclipse a leak would still happen: the listener is attached to the Preferences object of Eclipse, the Overlay still would have to implement it's own "dispose"-method which then disposes of this listener.

The action parametrizes the overlay as it is the "consumer" of the overlay.
Imagine in the future you have a specific type of View which wants to use the Overlay (which is something we are aiming for) but needs the overlay to be at the bottom - it would have to be able to disregard global parametrization of the Overlay.

[laeubi]

@Wittmaxi can you explain where / why you need to listen to preference changes? Maybe one can solve it without a listener or using the listener on a different place...

[Wittmaxi]

@laeubi When changing the Overlay preferences from "display overlay on top" to "display overlay on bottom", I want to be able to instantly display the overlay on the bottom without needing to close/re-open the overlay

@iloveeclipse is it okay for you if we create an issue to discuss this a bit later down the line (when Heiko is back from Vacation)? For now, I believe that it is most important to fix the JavaLeakTest, the design decision of "who is responsible for parametrizing the overlay?" is out of scope for this PR.

[jukzi]

Just start the test from IDE as any other test, it will fail saying that JVM args should be modified, so proceed and add all the javavmargs arguments from eclipse.jdt.ui/org.eclipse.jdt.ui.tests/test.xml to the JVM arguments launch config section.

i did that but the test was green for me locally.

[iloveeclipse]

i did that but the test was green for me locally.

Haven't checked the code, might be something not possible to test on Windows.

is it okay for you if we create an issue to discuss this a bit later down the line (when Heiko is back from Vacation)? For now, I believe that it is most important to fix the JavaLeakTest, the design decision of "who is responsible for parametrizing the overlay?" is out of scope for this PR.

OK

[HeikoKlare]

@HeikoKlare : not sure why you didn't want listener to be in the overlay class. Could you please check this bug?

is it okay for you if we create an issue to discuss this a bit later down the line (when Heiko is back from Vacation)? For now, I believe that it is most important to fix the JavaLeakTest, the design decision of "who is responsible for parametrizing the overlay?" is out of scope for this PR.

If I recap correctly, the reason for not wanting the preference listener in the FindReplaceOverlay is that this would unnecessarily tie the FindReplayOverlay to the according preference and thus a specific paramerization mechanism. This has several drawbacks, just to mention some of them:

1. As a very general remark, it introduces an unnecessary dependency and coupling.
2. It increases the number of responsibilities of the class (by also making it care about its own parameterization), while the class is too complex already now.
3. It makes the class parameterize itself instead of allowing its parametrization by consumers. This reduces its reusability and is usually rather an antipattern in terms of properly assigning responsibilities.
4. It reduces/complicates testability as it makes the class depend on some global parameterization state.

Anyway, is this still an open point for discussion and has the solution implemented so far to be revised or it okay for everyone?