RFR: JDK-8303897 ObservableValue's when binding should only invalidate when strictly needed [v2]

[John Hendrikx]

On Sat, 8 Apr 2023 23:56:10 GMT, Nir Lisker <nlisker at openjdk.org> wrote:

> 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.

I think something will need to change with the `ExpressionHelper` given the amount of problems it has currently.  At that time the boolean could be removed again.
 
> Also, I really wish we could make `add/removeListener` throw on `null` :)

You mean immediately?  Because the `null` check is done by `ExpressionHelper` already.  What I wish for is that `removeListener` would throw when removing non-existing listeners; that would expose a ton of logic errors where code is removing some listener that it thought was present, but wasn't...
 
> 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?

I mainly did that as I felt it didn't fit well in the structure of that other test, but it could be added there.  I also think that big test class may benefit from splitting up; it was getting a bit unwieldy.

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

PR Comment: https://git.openjdk.org/jfx/pull/1056#issuecomment-1501081120