On Thu, 1 Apr 2021 13:22:47 GMT, Kevin Rushforth <k...@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
