On Tue, 25 Feb 2025 22:33:40 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> Root Cause: >> (Multiple) properties are getting bound to the global >> `Platform.accessibilityActive` property. Binding (and I say, accessing) of >> properties is not thread-safe. >> >> I also changed the design a bit. Originally, every symbol in a chart had >> its `focusTraversableProperty` bound to `Platform.accessibilityActive` in >> order to enable the accessibility navigation across the chart data points. >> This is rather inefficient, as the property has to manage (thousands?) of >> listeners. >> >> Instead, a single boolean property is added to each chart, with a listener >> added to it which iterates over data symbols to toggle the >> `focusTraversableProperty` directly. >> >> The exact moment when the new property gets bound is also important, and has >> to happen in the FX application thread. >> >> With this change, it is safe to create and populate charts with data in a >> background thread. >> >> --- >> >> ## NOTES >> >> 1. It looks like the `Platform.accessibilityActive` property never >> transitions back to false after it transitioned to true. Some say it is >> because there is no mechanism in the platform to get notified which cannot >> possibly be true. >> 2. The javadoc for `Platform.accessibilityActiveProperty()` method >> stipulates that "_This method may be called from any thread_" which is >> patently not true as the current implementation is simply not thread-safe. >> >> ## Note to the Reviewers >> >> To avoid merge conflicts, the preferred order of integrations: >> >> #1697 >> #1713 >> #1717 > > Andy Goryachev has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 25 commits: > > - Merge remote-tracking branch 'origin/master' into > 8349091.charts.thread.safety > - review comments > - Merge remote-tracking branch 'origin/master' into > 8349091.charts.thread.safety > - 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 > - ... and 15 more: https://git.openjdk.org/jfx/compare/7a7854c9...4288d1d0 What you have now works in all cases I've tried. I left a couple suggestions and will reapprove if you decide to make changes. modules/javafx.controls/src/main/java/javafx/scene/chart/Chart.java line 106: > 104: > 105: // SimpleBooleanProperty or ObjectBinding > 106: private volatile Object accessibilityActive; You can use `ObservableValue<?>` instead of `Object` as the type. Alternatively, use two fields, a `SimpleBooleanProperty` for use by the FX app thread and an ObjectBinding for use by a background thread. They wouldn't need to be volatile in that case. What you have is OK, but using two properties might simplify the logic a bit. modules/javafx.controls/src/main/java/javafx/scene/chart/Chart.java line 561: > 559: accessibilityActive = winProp; // keep the reference so > it won't get gc > 560: > 561: // lambda cannot be used in place of a ChangeListener in > removeListener() Why not use a Subscription then? It seems tailor-made for what you are trying to do. ------------- Marked as reviewed by kcr (Lead). PR Review: https://git.openjdk.org/jfx/pull/1697#pullrequestreview-2644733749 PR Review Comment: https://git.openjdk.org/jfx/pull/1697#discussion_r1971704069 PR Review Comment: https://git.openjdk.org/jfx/pull/1697#discussion_r1971713667
