On Wed, 31 Mar 2021 12:00:39 GMT, Jeanette Winzenburg <faste...@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
