On Thu, 30 Mar 2023 19:58:31 GMT, Marius Hanl <mh...@openjdk.org> wrote:
> The determined `prefWidth` of a `TableCell` could be `0.0` when a
> `fixedCellSize` is set.
> This happened because the `TableCell` may not have a skin since it was never
> added to the scene graph yet.
>
> The fix is to make sure we get the `prefWidth` after the `TableCell` was
> added to the scene graph.
> That is also the reason why the problem only happened the first time and
> never again after (skin is then already created).
The proposed fix works and solves the test case in JDK-8305248. One of the
added tests fails without the fix, passes with it (the other one passes in both
cases).
I have added a few comments
modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableRowSkinBase.java
line 357:
> 355: }
> 356: // Note: We have to determine the pref width here
> because the add operation above may trigger the skin
> 357: // creation first, which is what makes it possible to
> get a correct value here in the first place.
The comment needs some rewording. As is now, it only explains why the change
proposed in this PR (moving the `prefWidth()` call, that is), but then it will
be out of context.
Something like:
// prefWidth() has to be called only after the tableCell is added to the
tableRow, if it wasn't already.
// Otherwise it might not have its skin yet, and its width value would be 0.
modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableRowSkinBase.java
line 358:
> 356: // Note: We have to determine the pref width here
> because the add operation above may trigger the skin
> 357: // creation first, which is what makes it possible to
> get a correct value here in the first place.
> 358: width = tableCell.prefWidth(height);
Alternatively, you could have just kept the first call to
`tableCell::prefWidth` as it was, and add a second one only inside the above if
expression, right after the tableCell is added to the tableRow.
I take that, only for such case, there would be two calls instead of one, but
it seems to be somehow a cleaner patch and explanation
modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableRowSkinTest.java
line 251:
> 249:
> 250: /**
> 251: * When we make an invisible column visible we expect the underlying
> cells to be visible, e.g. as width > 0.
This only applies to the case of fixed cell size set (this test will fail if
you comment out line 256), so maybe you could clarify the comment?
modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableRowSkinTest.java
line 276:
> 274: public void testMakeVisibleColumnInvisible() {
> 275: tableView.setFixedCellSize(24);
> 276: TableColumn<Person, ?> firstColumn =
> tableView.getColumns().get(0);
Might be unnecessary, but you could assert that the first column was visible,
just to make sure that the next call to make it invisible has a real change?
-------------
PR Review: https://git.openjdk.org/jfx/pull/1077#pullrequestreview-1366491500
PR Review Comment: https://git.openjdk.org/jfx/pull/1077#discussion_r1154176903
PR Review Comment: https://git.openjdk.org/jfx/pull/1077#discussion_r1154197254
PR Review Comment: https://git.openjdk.org/jfx/pull/1077#discussion_r1154215820
PR Review Comment: https://git.openjdk.org/jfx/pull/1077#discussion_r1154206575
