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

[Marius Hanl]

On Wed, 31 Mar 2021 12:00:39 GMT, Jeanette Winzenburg <fastegal at openjdk.org> wrote:

>> 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
>
> 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.

oh, right. It's called getVisibleLeafColumns() for a reason. �� 
I will remove the visibility check.

> 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.*` ?

Don't know, is there any kind of convention for that? Or is a static import fine?

> 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 :)

Thanks, didn't recognized that. I will change it so that **really** the last 2 columns are removed.

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

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