On Sun, 9 Apr 2023 09:04:15 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
> > 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 thought about throwing immediately, but you're right, it throws early enough. Not sure if the NPE should be documented on those methods or if it's obvious. And yes, throwing on non-existing listeners removal could make sense. I'm curious what breaks in JavaFX if this change is done. Could reveal some bugs maybe. > > 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. Yeah, `ObservableValueFluentBindingsTest` is very long. ------------- PR Comment: https://git.openjdk.org/jfx/pull/1056#issuecomment-1501171072
