RFR: 8294809: ListenerHelper for managing and disconnecting listeners [v9]

[Ajit Ghaisas]

On Mon, 14 Nov 2022 21:26:52 GMT, Kevin Rushforth <kcr at openjdk.org> wrote:

>> you are correct: the original intent for this class was to make it a general purpose facility to help with listeners, something that might be useful at the application level (and it used `ListenerHelper.get(Node)`).  Since that would require CSR and a public discussion, we decided to hide it as an implementation detail in skins (to unblock skin memory leak fixes).
>> 
>> Once we go through all the necessary discussions and everyone agrees, we could introduce it as a public API in the javafx.base module.
>
> Exactly. In its current form, `ListenerHelper` is a utility helper class for Skins, and should be reviewed with that in mind. As discussed in previous comments, there are several things that will need to change when/if this is proposed as a more general utility. We can defer that discussion, since this is entirely an internal helper class for now.
> 
> As a next step, after this is integrated and before any discussion of making this a public utility, it can be used as a replacement for `LambdaMultiplePropertyChangeListenerHandler` -- see [JDK-8296076](https://bugs.openjdk.org/browse/JDK-8296076).

Although it is an internal (non-public) utility class, once introduced, it will be tempting to use it from non-Skin code as well.
If the current scope is limited to provide "a utility helper class for Skins" then how about doing one of two things-
1) Mention the current intention and possible future modifications as a block comment above the `ListenerHelper` class
2) Keep only common methods in`ListenerHelper` class and move `Skin` specific code to a class derived from `ListenerHelper`. Skins should start using this derived class.

>> I don't think this is the case.
>> When this method is called, SkinBase class must be already loaded and resolved, that means it's static { } block, which sets the accessor, has been executed.  But just in case, I should move the static block in SkinBase, so we don't have any surprises.
>
> Right, there is no way the accessor can be null, since it is initialized at class load time in the static initializer, We do this in many other places (e.g., Node.java).

The `accessor` cannot be null **only if** it is used from *Skins.
In the current form, as we provide a no-argument public constructor, it is possible to create an object of `ListenerHelper` without calling setAccessor. Many unit tests added as part of this PR create such `ListenerHelper` objects.

-------------

PR: https://git.openjdk.org/jfx/pull/908