RFR: 8251481: TableCell accessing row: NPE on auto-sizing

[Jeanette Winzenburg]

On Fri, 14 Jan 2022 00:04:49 GMT, Marius Hanl <mhanl at openjdk.org> wrote:

> This PR will fix a simple NPE which may happens when using the `TableRow` inside a `TableCell` during the initial auto sizing.
> In the auto sizing code, no `TableRow` is set, therefore `getTableRow()` will return null and it is not possible to e.g. access the row item.
> 
> This is fixed by adding the `TableRow` in the `resizeColumnToFitContent` method, similar as it is already done for the `TreeTableView` (`TreeTableRow`).

fix looks good: verified that the bug example doesn't throw after the fix, also that the test failed/passed before/after the fix

left a couple of inline comments for the test

modules/javafx.controls/src/test/java/test/javafx/scene/control/TableCellTest.java line 371:

> 369:     @Test
> 370:     public void testRowIsNotNullWhenAutoSizing() {
> 371:         TableColumn<String, String> tableColumn = new TableColumn<>();

- the bug that's fixed in this PR is in TableColumnHeader, shouldn't the test be in TableColumnHeaderTest?
- if you decide to keep it here: it's in the middle of some edit-related tests, you might consider moving it up/down before/after those
- the fix aligns the resizeToFit method for TableView with that for TreeTableView: for symmetry, I would also expect a test method for the latter (which will pass both before and after the fix)

modules/javafx.controls/src/test/java/test/javafx/scene/control/TableCellTest.java line 379:

> 377: 
> 378:                 assertNotNull(getTableRow());
> 379:             }

feeling slightly uncomfortable with the generality of this: looks a bit like it's guaranteed that tableRow != null always (bullet 2 of our previous conversation) - would suggest to make it more specific to auto-sizing (f.i. start without auto-size, then trigger autosizing by code). Or at least doc it (add a message to the assertion, add the bug id) so that future readers will know what exactly is tested here :)

modules/javafx.controls/src/test/java/test/javafx/scene/control/TableCellTest.java line 385:

> 383:         StageLoader loader = new StageLoader(table);
> 384: 
> 385:         loader.dispose();

note that the loader isn't disposed if the test fails - that's why there's an instance field (which will be disposed in cleanup), which should be used here

-------------

Changes requested by fastegal (Reviewer).

PR: https://git.openjdk.java.net/jfx/pull/716