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

[Jeanette Winzenburg]

On Thu, 15 Apr 2021 12:57:30 GMT, Kevin Rushforth <kcr at 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