On Wed, 15 Apr 2020 10:17:41 GMT, Ambarish Rapte <ara...@openjdk.org> wrote:
>> Looked again, and actually seeing two different issues ;)
>>
>> A) your test - that is with firing the pulse: fails for both not/removing
>> the listener
>> B) basically same test, but not firing the pulse - it fails without removing
>> and passes with removing the listener
>>
>> So I think we should include B as it is directly related to this fix (and
>> verifies the need to remove the listener). As
>> to A: we should keep it somewhere to not forget, but where?
>> Test code B:
>>
>> @Test
>> public void testNPEOnSwitchSkinAndSelect() {
>> TabPane testPane = new TabPane(new Tab("tab0"), new Tab("tab1"));
>> Group root = new Group(testPane);
>> Scene scene = new Scene(root);
>> Stage stage = new Stage();
>> stage.setScene(scene);
>> stage.show();
>>
>> testPane.setSkin(new TabPaneSkin1(testPane));
>> testPane.getSelectionModel().select(1);
>> }
>>
>>
>> The exception (seen on the test stack when rewiring the uncaughthandler as
>> usual, else on sysout):
>>
>> Exception in thread "main" java.lang.NullPointerException
>> at
>> javafx.controls/javafx.scene.control.skin.TabPaneSkin.lambda$0(TabPaneSkin.java:447)
>> at
>> javafx.base/javafx.beans.WeakInvalidationListener.invalidated(WeakInvalidationListener.java:83)
>> at
>> javafx.base/com.sun.javafx.binding.ExpressionHelper$Generic.fireValueChangedEvent(ExpressionHelper.java:348)
>> at
>> javafx.base/com.sun.javafx.binding.ExpressionHelper.fireValueChangedEvent(ExpressionHelper.java:80)
>> at
>> javafx.base/javafx.beans.property.ReadOnlyObjectPropertyBase.fireValueChangedEvent(ReadOnlyObjectPropertyBase.java:74)
>> at
>> javafx.base/javafx.beans.property.ReadOnlyObjectWrapper.fireValueChangedEvent(ReadOnlyObjectWrapper.java:102)
>> at
>> javafx.base/javafx.beans.property.ObjectPropertyBase.markInvalid(ObjectPropertyBase.java:113)
>> at
>> javafx.base/javafx.beans.property.ObjectPropertyBase.set(ObjectPropertyBase.java:147)
>> at
>> javafx.controls/javafx.scene.control.SelectionModel.setSelectedItem(SelectionModel.java:105)
>> at
>> javafx.controls/javafx.scene.control.TabPane$TabPaneSelectionModel.select(TabPane.java:736)
>> at
>> javafx.controls/test.javafx.scene.control.SelectionFocusModelMemoryTest.testNPEOnSwitchSkinAndRemoveTabPaneFirePulse(SelectionFocusModelMemoryTest.java:261)
>
>> B) basically same test, but not firing the pulse - it fails without removing
>> and passes with removing the listener
>
> Seems like this test randomly crashes (not always) any other test from
> `TabPaneTest`. It might be crashing the test
> executed just after this one OR the next test which does `tk.firePulse()`.
> Also not including `tk.firePulse()` would
> make the test incomplete/ unreliable.
>> Personally, I would tend to keep and ignore that test with doc:
>
> That is better to include the test now, else very doubtful of adding it in
> future. Including the test in next commit
> with @Ignore("JDK-8242621").
can't reproduce the random crashing (but then, it's random :) And don't quite
understand why you think the test being
incomplete/unreliable without a firePulse (there are tons of tests without) -
as long as we don't want to test the
result of a layout pass, we should be fine.
anyway, that's a different story, to me your fix and tests look fine
-------------
PR: https://git.openjdk.java.net/jfx/pull/175
