RFR: 8173321: TableView: Click on right trough has no effect when cell height is higher than viewport height [v7]

[JoachimSchriek]

On Wed, 15 Feb 2023 23:53:51 GMT, Kevin Rushforth <kcr at openjdk.org> wrote:

>> JoachimSchriek has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains seven additional commits since the last revision:
>> 
>>  - Merge branch 'openjdk:master' into master
>>  - Restore original VirtualFlow.java
>>  - Changes made following findings from review
>>  - Deleted trailing whitespace
>>  - Committed Test Case for [openjdk/jfx] 8173321: Click on trough has no
>>    effect when cell height > viewport (PR #985):
>>  - Replaces Tabs with Spaces
>>  - JDK-8173321: Click on trough has no effect when cell height > viewport
>>    height
>
> modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/VirtualScrollBar.java line 139:
> 
>> 137:             IndexedCell firstVisibleCell = flow.getFirstVisibleCell();
>> 138:             IndexedCell lastVisibleCell = flow.getLastVisibleCell();
>> 139:             if (firstVisibleCell == lastVisibleCell) {
> 
> Since the preexisting code, now in the else block, checks first visible cell for null, it seems safer to add `firstVisibleCell != null &&` to this if check.

yes, without the null check the next line would throw a NullPointerException if the cell was null.
I will change this in my next commit.

> modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/VirtualScrollBar.java line 146:
> 
>> 144:                     flow.scrollTo(index + 1);
>> 145:                 }
>> 146: 
> 
> Minor: you can remove this blank line.

Ok

> tests/system/src/test/java/test/robot/javafx/scene/tableview/TableViewClickOnTroughTest.java line 70:
> 
>> 68:     static final int SCENE_WIDTH = 800;
>> 69:     static final int SCENE_HEIGHT = 250;
>> 70:     static CountDownLatch startupLatch = new CountDownLatch(1);
> 
> Minor: this can be `final`.

Ok

> tests/system/src/test/java/test/robot/javafx/scene/tableview/TableViewClickOnTroughTest.java line 73:
> 
>> 71: 
>> 72:     private static TableView<TableEntry> table;
>> 73:     public static TableRow<TableEntry> tableRow;
> 
> Minor: this need not be `public`.

the variable has no use anymore. So I will delete it

> tests/system/src/test/java/test/robot/javafx/scene/tableview/TableViewClickOnTroughTest.java line 82:
> 
>> 80:     @Test
>> 81:     public void moveTroughTest() {
>> 82:         ScrollBar verticalBar = (ScrollBar) table.lookup(".scroll-bar:vertical");
> 
> Minor: I recommend `assertNotNull(verticalBar);` here.

Ok

> tests/system/src/test/java/test/robot/javafx/scene/tableview/TableViewClickOnTroughTest.java line 84:
> 
>> 82:         ScrollBar verticalBar = (ScrollBar) table.lookup(".scroll-bar:vertical");
>> 83:         StackPane thumb = (StackPane) verticalBar.getChildrenUnmodifiable().stream()
>> 84:                 .filter(c -> c.getStyleClass().contains("thumb")).findFirst().orElse(null);
> 
> Minor: I recommend `assertNotNull(thumb);` here.

Ok

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

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