On Sat, 8 Apr 2023 23:56:10 GMT, Nir Lisker <nlis...@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
