On Wed, 8 Feb 2023 07:42:14 GMT, Karthik P K <k...@openjdk.org> wrote:
>> While checking for duplicate series addition to the line chart, >> `setToRemove` value was not considered before throwing exception. Hence code >> to handling the case of adding the removed series was never run. >> >> Added condition to check `setToRemove` boolean value before throwing >> exception. Made changes to call `setChart` method after calling >> `seriesBeingRemovedIsAdded`. Otherwise chart will not be drawn for the >> series, only points will be plotted. >> >> This issue is reproducible only when animation is enabled because timeline >> will be still running when removed series is added back to the same chart. >> Since timeline does not run in unit tests, added system test to validate the >> fix. > > Karthik P K has updated the pull request incrementally with two additional > commits since the last revision: > > - Renamed system test file > - Fixing issue in all XYCharts looks like the main issue has been fixed, tested with MonkeyTester https://github.com/andy-goryachev-oracle/Test/blob/main/src/goryachev/monkey/MonkeyTesterApp.java A few minor things are noted in the comments, most important is whether it's possible to leave the Timeline running forever. modules/javafx.controls/src/main/java/javafx/scene/chart/AreaChart.java line 72: > 70: // -------------- PRIVATE FIELDS > ------------------------------------------ > 71: > 72: /** A multiplier for teh Y values that we store for each series, it > is used to animate in a new series */ while we are at it, could we fix the comment "teh" -> "the" modules/javafx.controls/src/main/java/javafx/scene/chart/AreaChart.java line 562: > 560: if (timeline != null) { > 561: timeline.setOnFinished(null); > 562: timeline.stop(); two questions: 1. should the timeline object be set to null here? the reference will get overwritten in seriesRemoved():414, there is probably no need to keep this object in memory 2. is it possible that `seriesBeingRemovedIsAdded()` will be invoked twice with a different `series` argument? In other words, are we going to face a situation when a `timeline` from unrelated series will not get `stop`'ped? (these question apply to other charts as well) tests/system/src/test/java/test/javafx/scene/control/XYChartExceptionOnAddingRemovedSeriesTest.java line 94: > 92: @Test > 93: public void testLineChartExceptionOnAddingRemovedSeries() throws > Throwable { > 94: Thread.sleep(1000); // Wait for stage to layout I wonder if there is a better way of doing this, other than a long sleep? Perhaps use some kind of a concurrency primitive? tests/system/src/test/java/test/javafx/scene/control/XYChartExceptionOnAddingRemovedSeriesTest.java line 315: > 313: stage.show(); > 314: > 315: Thread.currentThread().setUncaughtExceptionHandler((t2, e) > -> { I think it's better to set this handler in a pre-test method annotated with @Before, and clean up in @After - see, for instance, how it's done in BehaviorCleanupTest. ------------- Changes requested by angorya (Committer). PR: https://git.openjdk.org/jfx/pull/1015
