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

[Jeanette Winzenburg]

On Thu, 1 Apr 2021 13:22:47 GMT, Kevin Rushforth <kcr at openjdk.org> wrote:

>> Jeanette Winzenburg has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   xxInvalidationListener: changed doc as per review
>
> 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}

@Kevin 

don't worry, I have seen your comment (and been thinking about it :) - but also can't find it right now (nor can I find Nir's comment which started the overhaul ..), no idea what happened.

Seeing your suggestion, it's similar to what keeps running in my mind.

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

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