On Fri, 14 Jan 2022 00:04:49 GMT, Marius Hanl <mh...@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