On Thu, 9 Feb 2023 09:39:49 GMT, Karthik P K <k...@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
