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

[Kevin Rushforth]

On Tue, 30 Mar 2021 09:53:55 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:
> 
>   xxInvalidationListener: changed doc as per review

One comment on the API that should be resolved before submitting the CSR.

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

> 268:      *  may be {@code null}
> 269:      * @return a composed consumer that performs all removed operations, or
> 270:      *  {@code null} if none have been registered or the observable is {@code null}

Somehow my comment seems to have been deleted, so here it is again, slightly modified, along with a suggestion.

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". Some might interpret this 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.

How about something like this for both newly added unregister methods?

     * @return a composed consumer consisting of all previously registered consumers, or
     * {@code null} if none have been registered or the observable is {@code null}

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

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