On Thu, 30 Mar 2023 21:53:48 GMT, John Hendrikx <[email protected]> wrote:
> Fixes three issues in ExpressionHelper:
>
> - Current Value was not retained when changing from SingleChange to Generic,
> this can lead to missed changes
> - Current Value was not retained when changing from Generic to SingleChange,
> this can lead to missed changes
> - Current Value was not cleared when last change listener was removed in
> Generic variant, resulting in an older value being referenced and not
> becoming eligible for GC until either a ChangeListener is added again, or
> sufficient InvalidationListeners are removed to switch to the
> SingleInvalidation implementation...
Looks good. Added a couple of comments about documentation.
modules/javafx.base/src/main/java/com/sun/javafx/binding/ExpressionHelper.java
line 1:
> 1: /*
I would write in the class doc a bit of an explanation on handling the current
value here compared to the one in the observable, including why
`ExpressionHelper` needs its own value and why when creating a new
`ExpressionHelper` the value needs to be passed form the previous one and not
taken from the observable (implying they are not the same). This is a fine
point that can be overlooked easily that I think it worth documenting
internally here.
modules/javafx.base/src/test/java/test/com/sun/javafx/binding/ExpressionHelperTest.java
line 673:
> 671: p.set("b");
> 672:
> 673: assertTrue(invalidated.get()); // true because it was added
> before called
Maybe the comment be more specific with something like "true because the
invalidation listener was added before it could be called"?
Same for the other methods.
-------------
PR Review: https://git.openjdk.org/jfx/pull/1078#pullrequestreview-1378296924
PR Review Comment: https://git.openjdk.org/jfx/pull/1078#discussion_r1162203751
PR Review Comment: https://git.openjdk.org/jfx/pull/1078#discussion_r1162192266
