RFR: JDK-8305248: TableView not rendered correctly after column is made visible if fixed cell size is set

[Jose Pereda]

On Thu, 30 Mar 2023 19:58:31 GMT, Marius Hanl <mhanl at 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