On Sun, 9 Apr 2023 21:27:36 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> Description copied from issue:
>>
>> There are up to two additional invalidations performed that really should be
>> avoided, causing downstream fluent bindings to be recomputed with the same
>> values. This is very confusing as these should only be called when there is
>> an actual change, and not called for the same value multiple times in a row.
>>
>> These two extra invalidations have two different causes, each causing an
>> additional invalidation to be triggered:
>>
>> 1) ObjectBinding's `isObserved` is delayed slightly. When you add a
>> listener, the listener is added internally and the binding is made valid;
>> this triggers some downstream activity which checks the `isObserved` status
>> to decide whether to start observation of properties -- unfortunately this
>> still returns `false` at that time. A work-around for this existed by
>> calling `getValue` again in `LazyObjectBinding` with a huge comment
>> explaining why this is needed. Although this works, it still means that a
>> downstream function like `map` is called an additional time while it should
>> really only be called once.
>>
>> The solution is to ensure `isObserved` returns `true` before the
>> `ExpressionHelper` is called. Already verified this solves the problem.
>> This also means the work-around in `LazyObjectBinding` is no longer needed,
>> which seems like a big win.
>>
>> 2) The second additional call originates from a different issue. When
>> `ConditionalBinding` (which implements the `when` construct) sees its
>> condition property changing, it always invalidates itself. This is however
>> only necessary if the current cached value (if it was valid) differs from
>> the current source value. To prevent an unnecessary invalidation, and the
>> resulting revalidation calls that this will trigger, a simple check to see
>> if the value actually changed before invalidating solves this problem.
>
> John Hendrikx has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Address review comments
>
> - Added extra tests cases that count invalidations
> - Added explanatory comment
I like the test coverage now.
I tried to test the condition
else if (isValid() && source.getValue() != getValue()) {
invalidate();
}
with this sequence:
property.set("e");
condition.set(false);
property.set("y");
property.set("e");
condition.set(true);
I wanted to test the change to the behavior of the condition changing while the
binding is valid, and the value is the same. However, the check for the value
is done with reference equality, so it will always invalidate (unless I create
my own objects and reuse them). Is this to align with the behavior of a change,
or will `equals` make more sense here?
Also, some of the comments are too long. You can move them above the line you
are commenting on and break the lines.
modules/javafx.base/src/test/java/test/javafx/beans/value/ObservableValueWhenTest.java
line 144:
> 142:
> 143: @Test
> 144: void
> shouldCallDownstreamMapFunctionOnlyWhenAbsolutelyNecessary() {
Minor: in other functions you used "AbsolutelyNeeded". You can align the names
if you want.
modules/javafx.base/src/test/java/test/javafx/beans/value/ObservableValueWhenTest.java
line 260:
> 258: assertEquals(0, observedInvalidations.get());
> 259:
> 260: when.getValue(); // would make no difference, inactive
> when bindings are always valid
Did you mean "always valid when bindings are inactive"? Or maybe that when the
binding is valid the listener is inactive? This sentence is confusing to me.
-------------
PR Review: https://git.openjdk.org/jfx/pull/1056#pullrequestreview-1376963070
PR Comment: https://git.openjdk.org/jfx/pull/1056#issuecomment-1501236106
PR Review Comment: https://git.openjdk.org/jfx/pull/1056#discussion_r1161349908
PR Review Comment: https://git.openjdk.org/jfx/pull/1056#discussion_r1161350295
