RFR: 8292353: TableRow vs. TreeTableRow: inconsistent visuals in cell selection mode [v10]

[Andy Goryachev]

On Tue, 6 Sep 2022 10:38:44 GMT, Jeanette Winzenburg <fastegal at openjdk.org> wrote:

>> Andy Goryachev has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   8292353: whitespace
>
> modules/javafx.controls/src/test/java/test/javafx/scene/control/ControlUtils.java line 157:
> 
>> 155:         }
>> 156:         throw new Error("TableRow not found at " + row);
>> 157:     }
> 
> no review, just a couple of comments:
> 
> Generally, should prefer api over lookup - we have shims to access package/protected api. 
> 
> In this particular case, the lookup is brittle: it assumes that
> 
> - either there is a single row configured with the given index 
> - or that it's the first when iterating over the set
> 
> With the first being incorrect (corner case, of course :)
> 
>     @Test
>     public void testLookupLastNotEmptyCell() {
>         stageLoader = new StageLoader(table);
>         int index = table.getItems().size()- 1;
>         Set<Node> tableRows = table.lookupAll(".table-row-cell").stream()
>                 .filter(n -> n instanceof TableRow<?> && ((TableRow<?>) n).getIndex() == index)
>                 .collect(Collectors.toSet());
>         assertEquals(1, tableRows.size());
>     }
> 
> we fall back to the second assumption, which is implementation dependent (and unspecified) - it's accidental that we actually do get the row we want.
> 
> Having done a bit of digging into VirtualFlowTestUtils (see [JDK-8293356](https://bugs.openjdk.org/browse/JDK-8293356)) I now think it's safe enough to use it (even though it's on the oldish side, not using recently added api, probably needs some polish) - as long as  we keep the scenegraph stable during the test, f.i. by manually adding the table to a stage (either via StageLoader or showing in our own).

Is it possible to have two or more rows with the same row index?  i would imagine that will break a lot of things.

I am not sure why you think the lookup is brittle - after all, VirtualFlowTestUtils uses lookup to get a pointer to the VirtualFlow, is it not (line 333)?  Is it because the lookup may not return a cell if it is outside of the current view port area and therefore has not been created (while the .getCell() guarantees to create one)? 

Is this the only objection to the changes in this PR?  I suppose i could change the tests to use VirtualFlowTestUtils, though my preference would be still to test close to the real world scenario, using public APIs (as opposed to getting into internals using some internal test utils), given the fact that I *do* expect these cells to be visible given the dimensions of the table and columns.

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

PR: https://git.openjdk.org/jfx/pull/875