On Thu, 6 Mar 2025 15:22:58 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
> 8351276: Prevent redundant computeValue calls when a chain of mappings
> becomes observed
modules/javafx.base/src/main/java/com/sun/javafx/binding/LazyObjectBinding.java
line 62:
> 60: updateSubscriptionBeforeAdd();
> 61:
> 62: super.addListener(listener);
I noticed that reverting this change causes all the
`GivenAQuadMappedObservable` to fails, but reverting the add `ChangeListener`
method change causes only change listener tests to fail. Is this correct?
Shouldn't there be a tests that fails just the invalidation listener code?
modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java line
378:
> 376: * can first become observed (allowing caching) and are then
> queried (likely
> 377: * getting the value from the cache). Doing this the other way
> around would
> 378: * result in the value (or chain of values) being computed twice.
If I understand correctly, it will be computed twice because the observable
would not be able to cache the value yet. It will compute it once without
caching with `getValue()`, and then again with `addListener(...)`. If this is
correct, I suggest writing a short continuation:
"Doing this the other way around would result in the value (or chain of values)
being computed twice because ___."
modules/javafx.base/src/test/java/test/javafx/beans/value/ObservableValueFluentBindingsTest.java
line 61:
> 59: private final InvalidationListener invalidationListener = obs ->
> invalidations++;
> 60:
> 61: private StringProperty property = new SimpleStringProperty("Initial");
`final`?
modules/javafx.base/src/test/java/test/javafx/beans/value/ObservableValueFluentBindingsTest.java
line 65:
> 63: @Nested
> 64: class GivenAQuadMappedObservable {
> 65: AtomicInteger calls1 = new AtomicInteger(0);
Empty line after the class declaration.
modules/javafx.base/src/test/java/test/javafx/beans/value/ObservableValueFluentBindingsTest.java
line 80:
> 78: assertEquals(0, calls2.get());
> 79: assertEquals(0, calls3.get());
> 80: assertEquals(0, calls4.get());
Since these assertions are a repeating pattern, do you want to extract it to a
reusable method?
private void assertValue(int val) {
assertEquals(val, calls1.get());
assertEquals(val, calls2.get());
assertEquals(val, calls3.get());
assertEquals(val, calls4.get());
}
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1730#discussion_r1985752123
PR Review Comment: https://git.openjdk.org/jfx/pull/1730#discussion_r1985677539
PR Review Comment: https://git.openjdk.org/jfx/pull/1730#discussion_r1985679166
PR Review Comment: https://git.openjdk.org/jfx/pull/1730#discussion_r1985680283
PR Review Comment: https://git.openjdk.org/jfx/pull/1730#discussion_r1985667361
