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

[Kevin Rushforth]

On Mon, 29 Mar 2021 15:39:58 GMT, Jeanette Winzenburg <fastegal at openjdk.org> wrote:

>> 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.
>
> - kept only the link to the registerXX method (to clarify the scope of the removal)
> - replaced "chained" by "composed" 
> - concentrated on what that composed consumer does (performs all removed operations)
> 
> Not entirely certain whether it's clear enough now 
> 
> - should the description have an additional sentence `Returns a composed [same-as-in-returns]` Undecided.
> - should the `same-as-in-returns` contain the specification of the sequence of performing (from the register method)? Tend to no (because it's getting too long), but then ..

I haven't gone back and done a detailed review yet, but I like the overall changes. The one thing I did notice is that the language used to describe the return value of `unregisterInvalidationListeners` and `unregisterListChangeListeners` is different:

unregisterInvalidationListeners:
     * @return a composed consumer that performs all removed operations, or
     *  {@code null} if none have been registered or the observable is {@code null}

unregisterListChangeListeners:
     * @return A single chained {@link Consumer} consisting of all {@link Consumer consumers} registered through
     *      {@link #registerListChangeListener(ObservableList, Consumer)}. If no consumers have been registered on this
     *      list, null will be returned.

I find it confusing to say that the returned list "performs all removed operations", so I like the language in the latter method better. Some might interpret the former to mean that is is somehow necessary to call the returned chained consumer as part of the removal, which isn't what you meant to say.

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

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