Re: Aw: Re: Skin ListenerHelper

[Andy Goryachev]

The chapter with leaking skins is almost completed: memory leaks are plugged, the testing framework is in place, switching is fully supported.

The only open ticket under umbrella

   JDK-8241364<https://bugs.openjdk.org/browse/JDK-8241364> ☂ Cleanup skin implementations to allow switching

is due to InputMap https://bugs.openjdk.org/browse/JDK-8245303  , which can be extracted into a separate issue (and which will be solved by design if the InputMap v2 proposal is accepted).

As far as the removal goes, we do have

   JDK-8296076<https://bugs.openjdk.org/browse/JDK-8296076> Remove internal class LambdaMultiplePropertyChangeListenerHandler

which is waiting its turn.

-andy

From: openjfx-dev <openjfx-dev-retn at openjdk.org> on behalf of Marius Hanl <mariushanl at web.de>
Date: Tuesday, March 12, 2024 at 11:27
To: openjfx-dev at openjdk.org <openjfx-dev at openjdk.org>, John Hendrikx <john.hendrikx at gmail.com>
Subject: Aw: Re: Skin ListenerHelper
Thanks for providing the examples with the Subscription API.

Note though that I rather want to talk about that we have 2 different components taking care of the listeners and both are mixed up. ListenerHelper is totally fine for me, but that should be the only one.

It would be nice to complete the chapter of leaking skins completely before tackling the next big changes. My opinion though.

-- Marius
Am 12.03.24, 09:40 schrieb John Hendrikx <john.hendrikx at gmail.com>:

With subscriptions these kinds of helpers are really not needed anymore.

Just do:

     Subscription combined = Subscription.combine(
         property1.subscribe( ... ),
         property2.subscribe( ... )
     );

And clean it up with `combined.unsubscribe` in `dispose`.

For non-property clean-ups, you can add additional cleanup like this:

     Subscription combined = subscription.and(() -> { extra clean-up code });

Alternatively, you can use `when(skinActive)` on most of your observers, and set `skinActive` to `false` in dispose.

SkinBase was a mistake IMHO.  Helper methods that require state are better placed in an instanced utility, not in a base class.  What does it mean that a "TableViewSkin" is-a "SkinBase". Using inheritance to get access to such helpers (which need a little bit of state) is really a classic mistake and misuse of inheritance.  ListenerHelper is much better in that regard, much more generally usable (since it uses composition), and stores its state in its own instance.

--John
On 12/03/2024 00:19, Marius Hanl wrote:
Since I see multiple discussions about the behaviour API and the RichTextControl, I want to ask another, still important question:
What is about the Skin cleanup ListenerHelper?

We now have 2 implementations of such utility - the

LambdaMultiplePropertyChangeListenerHandler
and the

ListenerHelper

I would really see that this topic is closed and cleaned up, with all TODOs that emerged from this than 2-3 new other open implemention changes on different topics.
The class is also not public, so extending from something else than SkinBase makes it hard to write your own skin as there is no proper help to register and dispose your listener.

-- Marius
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/openjfx-dev/attachments/20240312/ffe7a416/attachment-0001.htm>