On Thu, 15 Apr 2021 12:57:30 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
>> Jeanette Winzenburg has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> changed description as suggested in review
>
> 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.
removed
> 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`.
removed
> 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?
yes - renamed :)
> 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`).
changed as suggested. I used a similar pattern in BehaviorMemoryTest - should I
file a testbug to change it in the same way?
> 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.
changed
-------------
PR: https://git.openjdk.java.net/jfx/pull/409
