On Mon, 21 Dec 2020 11:54:06 GMT, Ambarish Rapte <[email protected]> wrote:
>> `TabPaneSkin` installs some listeners that are not removed when
>> `TabPaneSkin` is changed.
>> The fix converts listeners to WeakListeners and also removes them on dispose.
>>
>> There is a NPE check change needed in `isHosrizontal()`. Without this check
>> it causes NPE if pulse is in progress while TabPaneSkin is getting disposed.
>>
>> `SkinMemoryLeakTest` already had a test which only needed to be enabled.
>> Test fails before and passes after this change.
>
> Ambarish Rapte has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Review update
Fix looks good, verified tests failing (except one, commented) before and
passing after the fix.
Left a couple of comments inline. Plus there's an @Ignore related to the issue
in TabPaneTest (`testNPEOnSwitchSkinAndChangeSelection`) that should be removed.
modules/javafx.controls/src/main/java/javafx/scene/control/skin/TabPaneSkin.java
line 1239:
> 1237: }
> 1238: controlButtons.removeListeners();
> 1239: }
would suggest to rename the method to dispose - even though this here indeed
does nothing but removing listeners (future changes might need to do additional
cleanup). That would be a clear indication of its responsibility, just the
same name as the cleanup method you added for TabContentRegion. If we decide to
go for the rename, it should be done for all occurences of removeListeners in
the other internal panes.
modules/javafx.controls/src/main/java/javafx/scene/control/skin/TabPaneSkin.java
line 1577:
> 1575: listener.dispose();
> 1576: inner.getChildren().clear();
> 1577: getChildren().clear();
I'm aware this is internal and old api but .. could we consider cleaning it up
a bit?
- its task is to cleanup itself, doing whatever it deems is needed (here:
removing listeners and children), so removeListeners is a misnomer as it
describes only part of it. A (well-established) name for the task would be
dispose
- passing in the tab is .. strange, as can be seen in its usage in
TabHeaderArea.dispose: `header.removeListeners(header.getTab())` Actually, it
would be an error to call the method with a `tab != header.getTab()`
modules/javafx.controls/src/main/java/javafx/scene/control/skin/TabPaneSkin.java
line 1735:
> 1733: removeListeners();
> 1734: }
> 1735:
Wondering why the removal of children down the hierarchy? Shouldn't that be
unnecessary as long as the direct children of the skin/control are and all
listeners of the grandChildren to the skin are removed?
modules/javafx.controls/src/main/java/javafx/scene/control/skin/TabPaneSkin.java
line 1846:
> 1844:
> getSkinnable().sideProperty().removeListener(weakSidePropListener);
> 1845:
> getSkinnable().getTabs().removeListener(weakTabsListenerForPopup);
> 1846: }
yet another candidate for rename to dispose (see above)
modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinCleanupTest.java
line 275:
> 273: assertEquals(3, tabPane.getChildrenUnmodifiable().size());
> 274: }
> 275:
hmm .. looks a bit unusual (could be me, though :)
- why showControl? just install/replace the skin should be enough?
- the outcome of testing an illegal state (after instantiating the skin and
before really setting it as the control's skin at which time we have a skin :
control relation of 2:1) is unspecified. F.i. there are skins that replace all
children in the constructor (which may or may not be a good idea) that would
fail a test similar to this.
How about
TabPane tabPane ..
installDefaultSkin(tabPane);
int children = tabPane.getxxChildren().size();
replaceSkin(tabPane);
assertEquals(children, tabPane.getxxChildren().size());
If we decide to go without showing, the other tests should be changed as well :)
modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinCleanupTest.java
line 289:
> 287: assertNull(tabPane.getSkin());
> 288: assertEquals(0, tabPane.getChildrenUnmodifiable().size());
> 289: }
Same as above: what's the reason to actually show?
And just curious: why is this passing already before the fix?
modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinCleanupTest.java
line 302:
> 300: @Test
> 301: public void testNPEWhenRemTabAfterSkinIsReplaced() {
> 302: TabPane tabPane = new TabPane();
Personally I don't like abbreviation in method names, not even test methods.
`testNPEWhenRemoveTabAfterSkinIsReplaced` isn't that much longer :)
modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinCleanupTest.java
line 311:
> 309: @Test
> 310: public void testAddRemTabsAfterSkinIsReplaced() {
> 311: TabPane tabPane = new TabPane();
see above
-------------
Changes requested by fastegal (Committer).
PR: https://git.openjdk.java.net/jfx/pull/318
