[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-openjfx-dev
Subject:    Re: RFR: 8258777: SkinBase: add api to un-/register invalidation-/listChange listeners [v2]
From:       Kevin Rushforth <kcr () openjdk ! java ! net>
Date:       2021-03-30 19:54:13
Message-ID: 27_EOvST9gK-UUEFu927mhSWZChYJWaSIbuJJHy_n5I=.cd1547bb-39f2-476c-a995-52d90a683b41 () github ! com
[Download RAW message or body]

On Mon, 29 Mar 2021 15:39:58 GMT, Jeanette Winzenburg <fastegal@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


[prev in list] [next in list] [prev in thread] [next in thread]