RFR: 8258663: Fixed size TableCells are not removed from sene graph when column is removed [v2]

[Jeanette Winzenburg]

On Tue, 30 Mar 2021 15:54:37 GMT, Marius Hanl <github.com+66004280+Maran23 at openjdk.org> wrote:

>> This PR fixes an issue, where table cells are not removed from the table row when the corresponding table column got removed. This will lead to empty "ghost" cells laying around in the table.
>> This bug only occurs, when a fixed cell size is set to the table.
>> 
>> I also added 3 more tests beside the mandatory test, which tests my fix.
>> 1 alternative test, which tests the same with no fixed cell size set.
>> The other 2 tests are testing the same, but the table columns are set invisible instead.
>> 
>> -> Either the removal or setVisible(false) of a column should both do the same: Remove the corresponding cells, so that there are no empty cells.
>> Therefore, the additional tests make sure, that the other use cases (still) works and won't break in future (at least, without red tests ;)).
>> See also: TableRowSkinBase#updateCells(boolean)
>
> Marius Hanl has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8258663: Using VirtualFlowTestUtils in tests now instead of own solution -> cleaner code

Fix looks good, verified failing/passing test before/after fix. Left minor comments inline.

As to the test - good to increase test coverage by including tests for invisible columns, IMO :) 

Two thingies you might consider (your decision, I'm fine either way)
- test TreeTable also? Which doesn't seem to have the issue, so would be okay not to .. just for sanity.
- parameterize the test in fixedSize false/true

modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableRowSkinBase.java line 551:

> 549:                 if (!(cell instanceof IndexedCell)) continue;
> 550:                 TableColumnBase<T, ?> tableColumn = getTableColumn((R) cell);
> 551:                 if (!getVisibleLeafColumns().contains(tableColumn) || !tableColumn.isVisible()) {

coming back, now that I understand the overall logic :)

checking column.isVisible is not necessary: not being contained in the visibleLeafColumns implies that it is either not visible or not in the column hierarchy of table's columns.

modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableRowSkinTest.java line 42:

> 40: 
> 41: import static junit.framework.Assert.assertEquals;
> 42: 

shouldn't that be `org.junit.Assert.*` ?

modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableRowSkinTest.java line 116:

> 114:     private void removedColumnsShouldRemoveCorrespondingCellsInRowImpl() {
> 115:         // Remove the last 2 columns.
> 116:         tableView.getColumns().remove(tableView.getColumns().size() - 2, tableView.getColumns().size() - 1);

code comment is not correct - the last parameter of `remove(int, int)` is exclusive. While it doesn't change the outcome (failing/passing before/after fix), it might confuse future readers :)

modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableRowSkinTest.java line 122:

> 120:         // We removed 2 columns, so the cell count should be decremented by 2 as well.
> 121:         assertEquals(tableView.getColumns().size(),
> 122:                 VirtualFlowTestUtils.getCell(tableView, 0).getChildrenUnmodifiable().size());

same incorrect code comment, see above

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

Changes requested by fastegal (Committer).

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