Re: RFR: 8274061: Tree-/TableRowSkin: misbehavior on switching skin [v2]
On Fri, 19 Nov 2021 09:45:06 GMT, Ajit Ghaisas wrote: >> FYI: now the listener registration - including the incorrect code comment >> (which is the same as in current master) - is back at the old location in >> the re-inserted setupTreeTableViewListeners. So still need input whether >> it's okay to correct the code comment here. > > I think, it is okay to fix the comment in this review itself. It is very > minor to warrant a separate PR. Also, this anomaly was discovered in this > review makes it a candidate to be fixed here. done - thanks to both of you :) - PR: https://git.openjdk.java.net/jfx/pull/632
Re: RFR: 8274061: Tree-/TableRowSkin: misbehavior on switching skin [v2]
On Mon, 15 Nov 2021 13:28:40 GMT, Jeanette Winzenburg wrote: >> My PR is already merged, so this is not a problem. :) >> I dont know, but since this is only fixing a (also before) wrong comment it >> might be okay as it is very minor? :) > > FYI: now the listener registration - including the incorrect code comment > (which is the same as in current master) - is back at the old location in the > re-inserted setupTreeTableViewListeners. So still need input whether it's > okay to correct the code comment here. I think, it is okay to fix the comment in this review itself. It is very minor to warrant a separate PR. Also, this anomaly was discovered in this review makes it a candidate to be fixed here. - PR: https://git.openjdk.java.net/jfx/pull/632
Re: RFR: 8274061: Tree-/TableRowSkin: misbehavior on switching skin [v2]
On Mon, 1 Nov 2021 12:59:42 GMT, Marius Hanl wrote: >> well .. that would be a merge conflict, had you updated the code comment in >> your PR 😁 As noted in my comments to Ajit's review, the listener >> registration is simply moved (including the code comment .. belatedly :) >> >> Not sure how to handle it from here - following the rules, we might need a >> follow-up issue to the issue fixed in your PR? > > My PR is already merged, so this is not a problem. :) > I dont know, but since this is only fixing a (also before) wrong comment it > might be okay as it is very minor? :) FYI: now the listener registration - including the incorrect code comment (which is the same as in current master) - is back at the old location in the re-inserted setupTreeTableViewListeners. So still need input whether it's okay to correct the code comment here. - PR: https://git.openjdk.java.net/jfx/pull/632
Re: RFR: 8274061: Tree-/TableRowSkin: misbehavior on switching skin [v2]
On Sun, 31 Oct 2021 17:05:52 GMT, Michael Strauß wrote:
>> 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 graphicProperty() {
>> 298: TreeTableRow 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.
none except me having been sloppy ;) - removed both the access and the
left-over code comment from my dev version). And yeah, true: guarding against
null skinnable is always an indication of something being wrong (skinnable can
be null only after dispose - after that, its methods must not be used)
For fun, here's the evolution of `graphicProperty()` (which was `getGraphic()`
early on):
Initially the treeItem is accessed directly from the skinnable: the null guard
already is wrong (and might hide problems elsewhere), but doesn't look so
@Override protected Node getGraphic() {
TreeTableRow treeTableRow = getSkinnable();
if (treeTableRow == null) return null;
TreeItem treeItem = treeTableRow.getTreeItem();
if (treeItem == null) return null;
return treeItem.getGraphic();
}
At some time ([JDK-8124119](https://bugs.openjdk.java.net/browse/JDK-8124119)
was RT-28098) the direct access was replaced by keeping an alias to the
treeItem. Now the skinnable is not needed, and the null guard still is wrong :)
@Override protected Node getGraphic() {
TreeTableRow treeTableRow = getSkinnable();
if (treeTableRow == null) return null;
if (treeItem == null) return null;
return treeItem.getGraphic();
}
-
PR: https://git.openjdk.java.net/jfx/pull/632
Re: RFR: 8274061: Tree-/TableRowSkin: misbehavior on switching skin [v2]
On Mon, 1 Nov 2021 12:54:12 GMT, Jeanette Winzenburg wrote: >> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableRowSkin.java >> line 365: >> >>> 363: // Fix for RT-27782: Need to set isDirty to true, >>> rather than the >>> 364: // cheaper updateCells, as otherwise the text >>> indentation will not >>> 365: // be recalculated in >>> TreeTableCellSkin.leftLabelPadding() >> >> Actually this comment is not correct anymore since my PR got merged >> (https://github.com/openjdk/jfx/pull/568). >> Instead, it should be `TreeTableCellSkin.calculateIndentation()`. > > well .. that would be a merge conflict, had you updated the code comment in > your PR 😁 As noted in my comments to Ajit's review, the listener registration > is simply moved (including the code comment .. belatedly :) > > Not sure how to handle it from here - following the rules, we might need a > follow-up issue to the issue fixed in your PR? My PR is already merged, so this is not a problem. :) I dont know, but since this is only fixing a (also before) wrong comment it might be okay as it is very minor? :) - PR: https://git.openjdk.java.net/jfx/pull/632
Re: RFR: 8274061: Tree-/TableRowSkin: misbehavior on switching skin [v2]
On Sun, 31 Oct 2021 18:07:27 GMT, Marius Hanl wrote: >> 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 365: > >> 363: // Fix for RT-27782: Need to set isDirty to true, >> rather than the >> 364: // cheaper updateCells, as otherwise the text >> indentation will not >> 365: // be recalculated in >> TreeTableCellSkin.leftLabelPadding() > > Actually this comment is not correct anymore since my PR got merged > (https://github.com/openjdk/jfx/pull/568). > Instead, it should be `TreeTableCellSkin.calculateIndentation()`. well .. that would be a merge conflict, had you updated the code comment in your PR 😁 As noted in my comments to Ajit's review, the listener registration is simply moved (including the code comment .. belatedly :) Not sure how to handle it from here - following the rules, we might need a follow-up issue to the issue fixed in your PR? - PR: https://git.openjdk.java.net/jfx/pull/632
Re: RFR: 8274061: Tree-/TableRowSkin: misbehavior on switching skin [v2]
On Wed, 27 Oct 2021 09:56:46 GMT, Jeanette Winzenburg 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 365: > 363: // Fix for RT-27782: Need to set isDirty to true, rather > than the > 364: // cheaper updateCells, as otherwise the text > indentation will not > 365: // be recalculated in > TreeTableCellSkin.leftLabelPadding() Actually this comment is not correct anymore since my PR got merged (https://github.com/openjdk/jfx/pull/568). Instead, it should be `TreeTableCellSkin.calculateIndentation()`. - PR: https://git.openjdk.java.net/jfx/pull/632
Re: RFR: 8274061: Tree-/TableRowSkin: misbehavior on switching skin [v2]
On Wed, 27 Oct 2021 09:56:46 GMT, Jeanette Winzenburg
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 graphicProperty() {
> 298: TreeTableRow 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
Re: RFR: 8274061: Tree-/TableRowSkin: misbehavior on switching skin [v2]
On Wed, 27 Oct 2021 09:56:46 GMT, Jeanette Winzenburg 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 Marked as reviewed by aghaisas (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/632
Re: RFR: 8274061: Tree-/TableRowSkin: misbehavior on switching skin [v2]
On Wed, 27 Oct 2021 09:50:32 GMT, Jeanette Winzenburg wrote: >> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableRowSkin.java >> line 134: >> >>> 132: // that when it changes, we can appropriately add / >>> remove cells that may or may not >>> 133: // be required (because we remove all cells that are >>> not visible). >>> 134: >>> registerChangeListener(getVirtualFlow().widthProperty(), e -> >>> tableView.requestLayout()); >> >> If this listener is removed, then is there a chance of reintroducing >> [JDK-8144500](https://bugs.openjdk.java.net/browse/JDK-8144500)? >> Unfortunately, there is no test added for that bug.. so it is difficult to >> catch regression, if any. > > hmm .. the listener is not removed, its registration is moved to > updateTableViewSkin. There are tests covering that it really is still > registered :) > > Forgot to move the old code comment, though. Re-added. Thanks for the explanation. - PR: https://git.openjdk.java.net/jfx/pull/632
Re: RFR: 8274061: Tree-/TableRowSkin: misbehavior on switching skin [v2]
On Tue, 26 Oct 2021 12:07:23 GMT, Ajit Ghaisas wrote: >> 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/TableRowSkin.java > line 134: > >> 132: // that when it changes, we can appropriately add / >> remove cells that may or may not >> 133: // be required (because we remove all cells that are >> not visible). >> 134: >> registerChangeListener(getVirtualFlow().widthProperty(), e -> >> tableView.requestLayout()); > > If this listener is removed, then is there a chance of reintroducing > [JDK-8144500](https://bugs.openjdk.java.net/browse/JDK-8144500)? > Unfortunately, there is no test added for that bug.. so it is difficult to > catch regression, if any. hmm .. the listener is not removed, its registration is moved to updateTableViewSkin. There are tests covering that it really is still registered :) Forgot to move the old code comment, though. Re-added. > modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableRowSkin.java > line 154: > >> 152: // that when it changes, we can appropriately add / >> remove cells that may or may not >> 153: // be required (because we remove all cells that are >> not visible). >> 154: >> registerChangeListener(getVirtualFlow().widthProperty(), e -> >> treeTableView.requestLayout()); > > Same comment as in TableRowSkin regarding this listener. same comment as to TableRowSkin :) - PR: https://git.openjdk.java.net/jfx/pull/632
Re: RFR: 8274061: Tree-/TableRowSkin: misbehavior on switching skin [v2]
> 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 - Changes: - all: https://git.openjdk.java.net/jfx/pull/632/files - new: https://git.openjdk.java.net/jfx/pull/632/files/52e18d22..5402e1fb Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=632&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=632&range=00-01 Stats: 8 lines in 2 files changed: 8 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/632.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/632/head:pull/632 PR: https://git.openjdk.java.net/jfx/pull/632
