RFR: 8277000: Tree-/TableRowSkin: replace listener to fixedCellSize by live lookup [v2]
Andy Goryachev
angorya at openjdk.org
Tue Dec 10 21:13:45 UTC 2024
On Mon, 9 Dec 2024 23:55:01 GMT, Marius Hanl <mhanl at 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.
>
> Marius Hanl has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
>
> - Merge branch 'master' of https://github.com/openjdk/jfx into 8277000-tree-table-row-skin-live-lookup
> - Call getFixedCellSize() once
> - 8277000: Tree-/TableRowSkin: replace listener to fixedCellSize by live lookup
modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableRowSkinBase.java line 421:
> 419: } else {
> 420: width = tableCell.prefWidth(height);
> 421: // we only add/remove to the scenegraph if the fixed cell
are you sure this is correct?
the tableCell is added in L343 only if `fixedCellSize > 0`
the removal in L424 misses that logic
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1645#discussion_r1878861083
More information about the openjfx-dev
mailing list