On Tue, 30 Mar 2021 15:54:37 GMT, Marius Hanl
<github.com+66004280+mara...@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
