On Fri, 22 Nov 2024 20:31:08 GMT, Marius Hanl <mh...@openjdk.org> wrote:
> This PR improves the `Tree-/TableRowSkin` code by doing a normal live lookup > for the `fixedCellSize` instead of adding listener just to update > variables(`fixedCellSizeEnabled` and `fixedCellSize`) which can otherwise be > also just lookup'd without the hassle of listeners. > > While this is mostly a cleanup, it does improve the state of the > `Tree-/TableRow`, as when the `TableRowSkinBase` constructor is called, the > variables are not yet set. > > It is also consistent with the other cells, see also > [JDK-8246745](https://bugs.openjdk.org/browse/JDK-8246745). > Helps a bit with [JDK-8185887](https://bugs.openjdk.org/browse/JDK-8185887) > (https://github.com/openjdk/jfx/pull/1644), but as written above, not > required as there is no (visible) effect. Tested with the Monkey Tester, both TreeTableView and TableView - looks good, responds to fixed cell size changes. Left a few minor comments. modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableRowSkinBase.java line 323: > 321: > 322: boolean isVisible = true; > 323: if (getFixedCellSize() > 0) { `getFixedCellSize()` is getting called four times, also inside the `for` loop. I'd suggest to add double fixedCellSize = getFixedCellSize(); before L318 (outside of the for loop) and use the local variable instead. modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableRowSkinBase.java line 432: > 430: } > 431: > 432: double getFixedCellSize() { should this method be `abstract`? ------------- PR Review: https://git.openjdk.org/jfx/pull/1645#pullrequestreview-2479255931 PR Review Comment: https://git.openjdk.org/jfx/pull/1645#discussion_r1869884280 PR Review Comment: https://git.openjdk.org/jfx/pull/1645#discussion_r1869886562
