On Tue, 4 Oct 2022 14:20:46 GMT, Jeanette Winzenburg <faste...@openjdk.org>
wrote:
>>> Thanks again, @kleopatra With your permission, I'll add tests with and
>>> without scene property set. Or do we want to keep the original set?
>>
>> SkinMemoryLeakTest already has both methods, that is testing the replacement
>> of a skin with/out residing in a scene .. no need for adding anything ;)
>
>> In fact, it's so easy that most built-in skins do it (and these were created
>> by the developers of JavaFX itself, so obviously it's a major problem if
>> even those people get it wrong so often).
>>
>
> well, just guessing games here but: mine is that they didn't put too many
> thoughts into the possibility of replacing a skin and if they did, they
> actively prevented it - as seen in the implementation of TextAreaSkin.dispose
> when we started the current cleanup round:
>
> /** {@inheritDoc} */
> @Override public void dispose() {
> super.dispose();
>
> if (behavior != null) {
> behavior.dispose();
> }
>
> // TODO Unregister listeners on text editor, paragraph list
> throw new UnsupportedOperationException();
> }
> You are right, @kleopatra - this executeOnceWhenPropertyIsNonNull() is
> installing a listener. perhaps we should add a similar functionality to
> LambdaMultiplePropertyChangeListenerHandler and actually install a
> WeakListener instead.
- yes, we need weakListeners (to wait for a not-null value to add the event
handler) _and_ remove both the weakListener and the event handlers in dispose
- adding functionality to the LambdaHandler sounds like a good idea, but needs
some thinking and should be done in a separate issue which also adds delegate
methods to expose the new functionality in SkinBase for use in sub classes
For this issue, you might simply inline the utility method and register the
listeners using skinbase api (didn't try, it's your issue :), see TableRowSkin
for a similar pattern
This conditional adding of handlers/listeners is needed if we have to support
path properties (also f.i. when listening to selectedXX in any of the skins
with selectionModels as properties). Which basically should be supported by
base - but probably without any chance to ever get them ;)
-------------
PR: https://git.openjdk.org/jfx/pull/906
