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

[Nir Lisker]

On Tue, 23 Feb 2021 12:06:08 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:
> 
>   fixed missing/incorrect @since tags in new api doc

I reviewed only the public API methods.

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

> 61:  */
> 62: public final class LambdaMultiplePropertyChangeListenerHandler {
> 63: // FIXME: name doesn't fit after widening to support more notification event types

Maybe `MultipleObservableListenersHandler`? I didn't look at the class too much.

modules/javafx.controls/src/main/java/javafx/scene/control/SkinBase.java line 250:

> 248:      *
> 249:      * @param observable the observable to observe for invalidation events
> 250:      * @param consumer the consumer

Could be just me because I'm unfamiliar with this API, but I'd like to see an explanation for what the consumer is used for. If the consumer is executed on invalidation events on this observable, then clearly state it. Maybe the doc could be something like:

    Registers an action to take when an {@code Observable} is invalidated. 
    An {@code InvalidationListener} is registered on the given {@code observable} and the {@code consumer} is run 
    whenever the listener sends an invalidation event. If multiple consumers are registered on the same observable, 
    they are run in the order in which they were registered.

    @param observable the observable to observe for invalidation events
    @param consumer the action to take when the observable is invalidated

Since it's a `protected final` method, I don't think that the "Subclasses can use this..." portion is necessary - that's the only use of these methods.

modules/javafx.controls/src/main/java/javafx/scene/control/SkinBase.java line 253:

> 251:      * @since 17
> 252:      */
> 253:     protected final void registerInvalidationListener(Observable observable, Consumer<Observable> consumer) {

Can either of the arguments be `null`? Should there be an `Objects#RequireNonNull` check(s)?

modules/javafx.controls/src/main/java/javafx/scene/control/SkinBase.java line 264:

> 262:      * {@link #registerInvalidationListener(Observable, Consumer)}
> 263:      * for the given observable. The end result is that the given observable is no longer observed by any of the invalidation
> 264:      * listeners, but it may still have additional listeners registered on it through means outside of

Maybe instead of "The end result..." something like:

    Only listeners that were registered using the aforementioned method are removed; listeners registered through other means are unaffected.

modules/javafx.controls/src/main/java/javafx/scene/control/SkinBase.java line 267:

> 265:      * {@link #registerInvalidationListener(Observable, Consumer)}.
> 266:      *
> 267:      * @param observable The observable for which all listeners should be removed.

Maybe not "all listeners", but "the listeners". Will be less confusing and in line with the method description.

Also, the style is to use lowercase for the first word and no period at the end.

modules/javafx.controls/src/main/java/javafx/scene/control/SkinBase.java line 268:

> 266:      *
> 267:      * @param observable The observable for which all listeners should be removed.
> 268:      * @return A single chained {@link Consumer} consisting of all {@link Consumer consumers} registered through

1. There's no need for a `@link` on a class that is in the arguments or return of the method since they are linked there anyway, and it is recommended to use `@link` only the first time the class appears.

2. You might want to clarify that by "chained" you mean that they were composed using `Consumer#andThen`, either using `@plainlink` on "chained", or explicitly by "chained using Consumer#andThen`. Then again, it might be obvious.

modules/javafx.controls/src/main/java/javafx/scene/control/SkinBase.java line 270:

> 268:      * @return A single chained {@link Consumer} consisting of all {@link Consumer consumers} registered through
> 269:      *      {@link #registerInvalidationListener(Observable, Consumer)}. If no consumers have been registered on this
> 270:      *      property, null will be returned.

"null" --> `{@code null}`

"property" --> "observable"

modules/javafx.controls/src/main/java/javafx/scene/control/SkinBase.java line 273:

> 271:      * @since 17
> 272:      */
> 273:     protected final Consumer<Observable> unregisterInvalidationListeners(Observable observable) {

Is `null` check on `observable` needed?

modules/javafx.controls/src/main/java/javafx/scene/control/SkinBase.java line 290:

> 288:      * @since 17
> 289:      */
> 290:     protected final void registerListChangeListener(ObservableList<?> observableList, Consumer<Change<?>> consumer) {

Same comments as for the invalidation method.

modules/javafx.controls/src/main/java/javafx/scene/control/SkinBase.java line 311:

> 309:      * @since 17
> 310:      */
> 311:     protected final Consumer<Change<?>> unregisterListChangeListeners(ObservableList<?> observableList) {

Same comments as for the invalidation method.

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

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