RFR: 8088998: XYChart: duplicate child added exception when remove & add a series in several charts [v2]

[Andy Goryachev]

On Wed, 8 Feb 2023 07:42:14 GMT, Karthik P K <kpk at 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