On Thu, 30 Mar 2023 21:53:48 GMT, John Hendrikx <jhendr...@openjdk.org> 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