On Fri, 10 Jul 2026 17:59:32 GMT, Andy Goryachev <[email protected]> wrote:
>> John Hendrikx has updated the pull request incrementally with six additional
>> commits since the last revision:
>>
>> - Ensure change/invalidation listeners are not mixed up
>> - Clarify ArrayManager docs
>> - Update copyright year on new files
>> - Clarify occupied slots
>> - Add missing LF at end of file
>> - Sealed ListenerManagerBase
>
> modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerManager.java
> line 72:
>
>> 70:
>> 71: private void addAnyListener(I instance, Object listener) {
>> 72: Objects.requireNonNull(listener);
>
> this class makes no distinction between `InvalidationListener` and
> `ChangeListener`. should it?
No, it's not needed at this stage, the caller does distinguish them (to wrap
them as you pointed out when something implements both), and downstream (if
they go into a `ListenerList` when listener count > 1) it will put it in the
correct area (invalidation listener area or change listener area) of a single
array.
> modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerManagerBase.java
> line 63:
>
>> 61: protected abstract void setData(I instance, Object data);
>> 62:
>> 63: protected abstract void addInvalidationListener(I instance,
>> InvalidationListener listener);
>
> this is an internal sealed class, so it's probably ok not to add javadoc to
> protected methods... still, there were javadocs. might be helpful in the
> future.
The javadocs that were present are still there, just moved. I can add docs here
as well, but it will be a bit repetitive.
> modules/javafx.base/src/main/java/javafx/beans/property/LongPropertyBase.java
> line 2:
>
>> 1: /*
>> 2: * Copyright (c) 2011, 2025, Oracle and/or its affiliates. All rights
>> reserved.
>
> in the year 252525... I know we have a script, but it also might create
> merging problems...
I promise to resolve merge conflicts if they occur, until then, I think it is
fine to let the script handle updating modifications to existing files?
> modules/javafx.base/src/test/java/test/com/sun/javafx/binding/ListenerListBaseTest.java
> line 180:
>
>> 178:
>> 179: @Test
>> 180: void shouldNeverSilentlyRemoveWeakListeners() {
>
> question: does this tests the public specification, or the implementation?
Good one; this test is to make sure that dead weak listeners stubs don't
disappear at unexpected moments. Some code (see `ObjectBinding#removeListener`
for example) tracks whether the listener lists is empty through overrides of
`addListener` and `removeListener`:
public void removeListener(ChangeListener<? super T> listener) {
observed = !LISTENER_MANAGER.removeListener(this,
(ChangeListener<Object>) listener);
}
Or the old code:
public void removeListener(ChangeListener<? super T> listener) {
helper = ExpressionHelper.removeListener(helper, listener);
observed = helper != null;
}
To not break such tracking, we can only remove dead listeners when some other
list modification occurs; either an addition (in which case we know there is at
least one new listener, so we can remove any dead listeners), or a removal (in
which case if the list becomes empty, we return `true` or with the old code, we
check if `helper == null`.)
The `observed` flag is very important for the lazy binding code (that powers
`ObservableValue#map/flatMap/orElse`), but can also be very handy when you want
to remove or clean-up your own listeners if you yourself are not observed
anymore (if nobody observes you, then why would you bother updating yourself
:)).
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r3562028597
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r3562031268
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r3562014540
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r3562003256
