RFR: JDK-8224260: ChangeListener not triggered when adding a new listener in invalidated method

[Nir Lisker]

On Thu, 30 Mar 2023 21:53:48 GMT, John Hendrikx <jhendrikx at 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