On Fri, 7 Apr 2023 06:36:50 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:
> 
>   Fix review comments

I reviewed the first bug and the fix looks good.

The fix adds a `boolean` field to all `ObjectBinding`s, which could be a slight 
performance hit. I have a local version of a refactored `ExpressionHelper` 
class that uses a singleton instance for an empty helper instead of `null`. It 
makes the code cleaner (no special `null` handling) and will allow to get rid 
of the new `observed` field because that info can be encoded in 
`ExpressionHelper` instead of in `ObjectBinding`. After all, the info of being 
observed really belongs in the class that manages the listeners IMO. Not sure 
if it's worth pursuing at the moment.

Also, I really wish we could make `add/removeListener` throw on `null` :)

I still need to finish reviewing the second bug. The logic flow is a bit 
complicated when taking into account the many states the binding can be at 
(valid, observed, active...). One question I have is about the tests: why split 
a When test class from an already existing one in the fluent bindings test?

-------------

PR Review: https://git.openjdk.org/jfx/pull/1056#pullrequestreview-1376817231

Reply via email to