RFR: 8258777: SkinBase: add api to un-/register invalidation-/listChange listeners [v9]

[Kevin Rushforth]

On Mon, 12 Apr 2021 14:45:58 GMT, Jeanette Winzenburg <fastegal at openjdk.org> wrote:

>> Changes in Lambda..Handler:
>> - added api and implemenation to support invalidation and listChange listeners in the same way as changeListeners
>> - added java doc 
>> - added tests
>> 
>> Changes in SkinBase
>> - added api (and implementation delegating to the handler)
>> - copied java doc from the change listener un/register methods 
>> - added tests to verify that the new (and old) api is indeed delegating to the handler
>> 
>> Note that the null handling is slightly extended: all methods now can handle both null consumers (as before) and null observables (new) - this allows simplified code on rewiring "path" properties (see reference example in the issue)
>
> Jeanette Winzenburg has updated the pull request incrementally with one additional commit since the last revision:
> 
>   changed description as suggested in review

The fix and tests looks good, with one suggestion for the test (and a couple minor comments about formatting and one naming question) left inline.

I'll review the CSR now, you can then move it to Finalized when you are ready.

modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/LambdaMultiplePropertyChangeListenerHandler.java line 60:

> 58:  * <p>
> 59:  * Disposing removes all listeners added by this handler from all registered observables.
> 60:  *

Minor: you can remove this extra blank line.

modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/LambdaMultiplePropertyChangeListenerHandler.java line 66:

> 64: 
> 65:     @SuppressWarnings("rawtypes")
> 66:     private  static final Consumer EMPTY_CONSUMER = e -> {};

Minor: remove extra space between `public` and `static`.

modules/javafx.controls/src/test/java/test/com/sun/javafx/scene/control/LambdaMultipleHandlerTest.java line 51:

> 49:  * Test support of listChange listeners.
> 50:  */
> 51: public class LambdaMultipleHandlerTest {

Would `LambdaMultipleListHandlerTest ` be a better name for this class?

modules/javafx.controls/src/test/java/test/com/sun/javafx/scene/control/LambdaMultipleHandlerTest.java line 211:

> 209:         WeakReference<LambdaMultiplePropertyChangeListenerHandler> ref =
> 210:                 new WeakReference<>(new LambdaMultiplePropertyChangeListenerHandler());
> 211:         ref.get().registerListChangeListener(items, consumer);

This is fragile. It is possible, although unlikely, that the referent could be GCed between its construction in the previous statement (after which it goes out of scope), and this statement. If this rare event happened, it would cause an NPE here. I recommend to keep a local reference to the referent and then set it to null after this (and before the `attemptGC`).

modules/javafx.controls/src/test/java/test/com/sun/javafx/scene/control/LambdaMultipleObservableHandlerTest.java line 217:

> 215:         WeakReference<LambdaMultiplePropertyChangeListenerHandler> ref =
> 216:                 new WeakReference<>(new LambdaMultiplePropertyChangeListenerHandler());
> 217:         registerListener(ref.get(), p, consumer);

Same comment as with previous test.

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

PR: https://git.openjdk.java.net/jfx/pull/409