On Mon, 24 Feb 2025 23:56:01 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
>> Andy Goryachev has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 22 commits: >> >> - Merge remote-tracking branch 'origin/master' into >> 8349091.charts.thread.safety >> - enabled pie chart test >> - Merge branch 'master' into 8349091.charts.thread.safety >> - Merge branch 'master' into 8349091.charts.thread.safety >> - whitespace >> - Merge remote-tracking branch 'origin/master' into >> 8349091.charts.thread.safety >> - cleanup >> - tests pass >> - chart tests only >> - more jitter >> - ... and 12 more: https://git.openjdk.org/jfx/compare/307e3087...e60c027b > > modules/javafx.controls/src/main/java/javafx/scene/chart/Chart.java line 551: > >> 549: ChangeListener<Scene> li = new ChangeListener<>() { >> 550: @Override >> 551: public void changed(ObservableValue<? extends Scene> >> src, Scene prev, Scene scene) { > > This is insufficient. It will miss the case where both the node and the scene > are created in the background thread, and the scene is later added to a > window on the FX app thread. I tested this case and verified my assertion. > This also means that in some cases, if this listener is called from a > background thread, you will create a new scene listener from the current > scene listener. > > Consider instead using `TreeShowingProperty`, which is used by > `ProgressIndicatorSkin` to set up its animation.`TreeShowingProperty` sets up > a listener chain that fires when the the "tree showing" status of a node > changed (tree showing means the node and all ancestors are visible and is > attached to a scene that is attached to a window that is showing. > > Alternatively, since you might not care whether the node is visible, you can > just set up a binding using `ObservableValue::flatMap`. The code snippet of > the docs: > > > ObservableValue<Boolean> isShowing = sceneProperty() > .flatMap(Scene::windowProperty) > .flatMap(Window::showingProperty) > .orElse(false); > > > And then you can then add a listener (or a value subscription might be > better) on `isShowing` -- when it becomes true, you can setup the binding or > listener to `Platform.accessibilityActiveProperty`. > > With either of the above two solutions, the showing state will only become > true on the FX app thread. thanks for noticing! the old code was indeed flawed. ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1697#discussion_r1970371417
