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

[Andy Goryachev]

On Thu, 9 Feb 2023 09:39:49 GMT, Karthik P K <kpk at openjdk.org> wrote:

>> 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)
>
>> 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
>> 
> Yes timeline can be made null here. Updated the code.
> 
>>     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)
> 
> Since `seriesBeingRemovedIsAdded()` is called from the event handler which gets invoked when the series changes and further there is a check on `setToRemove` boolean, I believe the method will not be called with different `series` and cause unwanted `series` timeline to be stopped.

Thank you for clarification, @karthikpandelu

>> 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.
>
> Updated code to set the handler in function annotated with @BeforeClass and cleaned up in  @AfterClass.

I am not sure setting uncaught exception handler in @BeforeClass and clearing it in @AfterClass is correct.
The junit framework may execute these tests in parallel or sequentially (I think these are actually executed sequentially), but I would not rely on the fact that the individual tests will be executed in the same thread.

So I'd suggest to move this code to @After and @Before, just like BehaviorCleanupTest and other tests.

-------------

PR: https://git.openjdk.org/jfx/pull/1015