On Tue, 30 Mar 2021 09:53:55 GMT, Jeanette Winzenburg <faste...@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
