Re: RFR: 8258663: Fixed size TableCells are not removed from sene graph when column is removed [v2]
On Wed, 31 Mar 2021 13:20:09 GMT, Kevin Rushforth wrote: >> 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.*` ? > > I don't think @kleopatra was referring to whether or not to use a wild-card > import, but rather that the `Assert` class should be imported from the > `org.junit` package instead of `junit.framework`. We are inconsistent in our > usage in some of the older tests, but yes, we should use `org.junit`. Oh, alright, didn't saw that. I will change it. :) - PR: https://git.openjdk.java.net/jfx/pull/444
Re: RFR: 8258663: Fixed size TableCells are not removed from sene graph when column is removed [v2]
On Wed, 31 Mar 2021 12:32:02 GMT, Jeanette Winzenburg wrote: > > > 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 I didn't wrote tests for TreeTableView, as they have no issue (and I'm lazy :P). But it might be a good idea to do so in future. I chose not to make the tests parameterized so other people can add tests as well (without the parameterized 'dependency'). As far as I saw, you can't just define some tests to be parameterized (only the whole class is). I know that his is possible in JUnit5 with @MethodSource, but unfortunatly we run on JUnit4. - PR: https://git.openjdk.java.net/jfx/pull/444
Re: RFR: 8258663: Fixed size TableCells are not removed from sene graph when column is removed [v2]
On Wed, 31 Mar 2021 12:18:54 GMT, Jeanette Winzenburg 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/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.*` ? I don't think @kleopatra was referring to whether or not to use a wild-card import, but rather that the `Assert` class should be imported from the `org.junit` package instead of `junit.framework`. We are inconsistent in our usage in some of the older tests, but yes, we should use `org.junit`. - PR: https://git.openjdk.java.net/jfx/pull/444
Re: RFR: 8258663: Fixed size TableCells are not removed from sene graph when column is removed [v2]
On Wed, 31 Mar 2021 12:00:39 GMT, Jeanette Winzenburg
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 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
Re: RFR: 8258663: Fixed size TableCells are not removed from sene graph when column is removed [v2]
On Tue, 30 Mar 2021 15:54:37 GMT, Marius Hanl
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 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
Re: RFR: 8258663: Fixed size TableCells are not removed from sene graph when column is removed [v2]
On Tue, 30 Mar 2021 13:27:21 GMT, Jeanette Winzenburg
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/test/java/test/javafx/scene/control/skin/TableRowSkinTest.java
> line 132:
>
>> 130: // We save the first table row to check it later.
>> 131: AtomicReference> tableRowRef = new
>> AtomicReference<>();
>> 132:
>
> wondering a bit about this complicated test setup .. are you aware of the
> VirtualFlowTestUtils (in test.something.infrastructure)? Using it, a test
> would shrink down to something like:
>
> @Test
> public void testRemoveColumnsFixed() {
> tableView.setFixedCellSize(20);
> tableView.getColumns().remove(0, 2);
> Toolkit.getToolkit().firePulse();
> assertEquals(tableView.getVisibleLeafColumns().size(),
> VirtualFlowTestUtils.getCell(tableView,
> 0).getChildrenUnmodifiable().size());
> }
>
> Or what am I missing?
Nice catch! I tried it out and it works! And indeed the code looks much better
now.
To be fair, I had a brief look at VirtualFlowTestUtils, as other table/cell
tests uses it. Next time I should take a closer look. :P
I pushed your suggested change.
-
PR: https://git.openjdk.java.net/jfx/pull/444
Re: RFR: 8258663: Fixed size TableCells are not removed from sene graph when column is removed [v2]
> 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 - Changes: - all: https://git.openjdk.java.net/jfx/pull/444/files - new: https://git.openjdk.java.net/jfx/pull/444/files/acc19ee4..a2a331a2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=444=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=444=00-01 Stats: 55 lines in 1 file changed: 4 ins; 37 del; 14 mod Patch: https://git.openjdk.java.net/jfx/pull/444.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/444/head:pull/444 PR: https://git.openjdk.java.net/jfx/pull/444
