On Fri, 14 Jan 2022 00:04:49 GMT, Marius Hanl <[email protected]> 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
