On Mon, 14 Nov 2022 21:26:52 GMT, Kevin Rushforth <k...@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
