On Mon, 10 Mar 2025 02:07:04 GMT, Nir Lisker <nlis...@openjdk.org> wrote:
>> John Hendrikx has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix non-convergence logic one more time...
>
> modules/javafx.base/src/main/java/com/sun/javafx/binding/OldValueCachingListenerManager.java
> line 53:
>
>> 51: * within listener list. If possible use {@link ListenerManager}, as it
>> has less
>> 52: * storage requirements and is faster.
>> 53: *
>
> I suggest adding a line that says that this class is used by properties.
That doesn't seem like something you should be adding to the docs I think?
> modules/javafx.base/src/main/java/com/sun/javafx/binding/OldValueCachingListenerManager.java
> line 100:
>
>> 98: * @throws NullPointerException when listener is {@code null}
>> 99: */
>> 100: public void addListener(I instance, ChangeListener<? super T>
>> listener) {
>
> Same remarks from the invalidation listener method, but does `instance` not
> need a `null` check or does `getData` do that? It's not so clear.
This is basically handed off to individual implementations (`getData` is
abstract), for example in `FloatBinding`:
private static final ListenerManager<Number, FloatBinding> LISTENER_MANAGER
= new ListenerManager<>() {
@Override
protected Object getData(FloatBinding instance) {
return instance.listenerData;
}
@Override
protected void setData(FloatBinding instance, Object data) {
instance.listenerData = data;
}
};
I've updated the docs for the abstract methods to include that these should
throw NPE when `instance` is `null`.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1986834518
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1986828001
