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

[Kevin Rushforth]

On Mon, 6 Feb 2023 14:15:17 GMT, JoachimSchriek <duke at openjdk.org> wrote:

>> This is my (Joachim.Schriek at gmx.de) first contribution to openjfx. My Contributor Agreement is signed but still in review.
>> So please be patient with an absolute beginner as contributor ... .
>> The work of this pull request was fully done in my spare time.
>> 
>> I first filed the bug myself in 2017. I had begun working with JavaFX in 2014.
>> 
>> The two changes address the two problems mentioned in JDK-8173321:
>> - Using a JavaFX TableView, a click on the right trough has no effect when the cell height of the cell currently displayed is higher than viewport height
>> - The ScrollBar ist displayed with a minimal height.
>> 
>> The changes were tested and ran well with Java 17 and the current master branch of openjfx.
>
> 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

The fix looks good, although I think you need to add a null check in one place (see inline comments). The test looks good and verifies the fix, in that it fails without the fix and passes with the fix. I left a few minor comments on the test.

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.

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.

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

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

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.

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.

tests/system/src/test/java/test/robot/javafx/scene/tableview/TableViewClickOnTroughTest.java line 101:

> 99:         Util.sleep(1000); // Delay for table moving Scrollbar
> 100:         double newPosition = verticalBar.getValue();
> 101:         Assert.assertTrue("moveTroughTest failed", oldPosition != newPosition);

Suggestion: You might consider using `assertNotEquals` with a delta of, say, `0.1`.

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

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