On Mon, 12 Apr 2021 14:45:58 GMT, Jeanette Winzenburg <[email protected]>
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
