I think the issue in JDK-8206449 might not be entirely resolved.
Even though `value` is now cleared in `ObjectBinding`, I noticed that
`ExpressionHelper` also maintains the current value when there are
change listeners registered. Unfortunately, it forgets to clear this
when all change listeners are removed but there are still sufficient
invalidation listeners registered for it to need the Generic
ExpressionHelper implementation.
I think an overhaul of ExpressionHelper might be warranted:
- Doesn't clear current value when it should
- The locking done makes a new copy of the listener lists each time if
there are multiple changes to these lists during an event emission (this
can happen easily when many listener stubs have gone out of scope)
- Change event emission should probably not be dependent on the order of
listeners, see https://bugs.openjdk.org/browse/JDK-8290310
- Further optimizations are possible:
- Better remove performance -- although there is no JDK collection
class that has fast remove(T) while allowing duplicates, it isn't hard
to construct a specialized class that has fast `addLast`, fast
`remove(T)` and fast iteration
- Single invalidation listener could just use the helper field
directly (instanceof check can determine if it is an invalidation
listener or a helper instance); this can save quite some memory as the
fast majority of properties/bindings may never have more than 1 listener
- Single change maybe as well (requires current value to be passed
into #fireValueChangedEvent, which is how most other ExpressionHelpers,
except the base ExpressionHelper, are done) -- note that many, if not
all, users of ExpressionHelper often also have a copy of the current
value maintained separately...
- Small listener lists could use a single array (using instanceof to
distinguish between invalidation/change)
I also find that there is a test for ExpressionHelper, testing a ton of
edge cases, but I think this class shouldn't be tested at all. Instead,
a test should be there to test the behavior of classes implementing
Observable and ObservableValue, regardless of whether they use
ExpressionHelper internally. Such a test would be reusable, regardless
of how these classes manage their listeners, or if some decide to not
use `ExpressionHelper`.
A good JUnit test should I think be constructed first, validating the
current behavior based on classes implementing Observable and
ObservableValue; these should be thorough enough to cover most (if not
all) of the edge cases now tested in ExpressionHelperTest.
With any luck, the test can be written in such a way that it can be
parameterized on the type of class implementing ObservableValue,
allowing a single test to test behavior of all *Property classes and all
*Binding classes.
--John
On 15/07/2022 16:09, Nir Lisker wrote:
On Fri, 15 Jul 2022 07:19:19 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
I introduced a bug with the fluent bindings PR which affects all ObjectBindings.
This is the code that fails:
SimpleObjectProperty<Boolean> condition = new
SimpleObjectProperty<>(true);
ObservableValue<String> binding = condition.map(Object::toString);
binding.addListener(o -> { binding.getValue(); });
condition.set(false);
assertEquals(false, binding.getValue()); // returns null (!)
This PR fixes this problem and adds a test case to cover it.
John Hendrikx has updated the pull request incrementally with one additional
commit since the last revision:
Change explanatory comment to block comment
Alright, I closed [JDK-8206449](https://bugs.openjdk.org/browse/JDK-8206449) as
duplicated of [JDK-8274771](https://bugs.openjdk.org/browse/JDK-8274771).
-------------
PR: https://git.openjdk.org/jfx/pull/829
