[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-openjfx-dev
Subject:    Re: RFR: 8274061: Tree-/TableRowSkin: misbehavior on switching skin [v2]
From:       Michael =?UTF-8?B?U3RyYXXDnw==?= <mstrauss () openjdk ! java ! net>
Date:       2021-10-31 17:09:11
Message-ID: jTt0RWGMpD2_kcCYuE9_FMFXEJ5ugaP_nCca3PDl6wQ=.0558fd56-06c9-4e9f-bd1e-78abb86f8f9b () github ! com
[Download RAW message or body]

On Wed, 27 Oct 2021 09:56:46 GMT, Jeanette Winzenburg <fastegal@openjdk.org> wrote:

> > Cleanup of Tree-/TableRowSkin to support switching skins
> > 
> > The misbehavior/s
> > - memory leaks due to manually registered listeners that were not removed
> > - side-effects due to listeners still active on old skin (like NPEs)
> > 
> > Fix
> > - use skin api for all listener registration (for automatic removal in dispose)
> > - do not install listeners that are not needed (fixedCellSize, same as in fix of \
> > ListCellSkin [JDK-8246745](https://bugs.openjdk.java.net/browse/JDK-8246745)) 
> > Added tests for each listener involved in the fix to guarantee it's still working \
> > and does not have unwanted side-effects after switching skins. 
> > Note: there are pecularities in row skins (like not updating themselves on \
> > property changes of its control, throwing NPEs when not added to a VirtualFlow) \
> > which are not part of this issue but covered in \
> > [JDK-8274065](https://bugs.openjdk.java.net/browse/JDK-8274065)
> 
> Jeanette Winzenburg has updated the pull request incrementally with one additional \
> commit since the last revision: 
> re-added forgotten code comments

modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableRowSkin.java \
line 299:

> 297:     @Override protected ObjectProperty<Node> graphicProperty() {
> 298:         TreeTableRow<T> treeTableRow = getSkinnable();
> 299:         // FIXME: illegal access if skinnable is null

What is the purpose of removing the null check, but leaving getSkinnable() in there?
Given the signature of the method, it seems that it shouldn't ever return `null` in \
any case.

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

PR: https://git.openjdk.java.net/jfx/pull/632


[prev in list] [next in list] [prev in thread] [next in thread]