RFR: 8173321: Click on trough has no effect when cell height > viewport [v3]

[Kevin Rushforth]

On Sun, 22 Jan 2023 15:34:25 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 incrementally with one additional commit since the last revision:
> 
>   Deleted trailing whitespace

I left a few inline comments. Most are pretty minor, but I am a bit concerned about the change to the computation of the visible amount of the `lengthBar`.

modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/VirtualScrollBar.java line 135:

> 133:             /**
> 134:              * JDK-8173321: Click on trough has no effect when cell height > viewport height:
> 135:              * Solution: Scroll one cell further up resp. down if only one cell is shown.

We generally do not refer to specific bug IDs in source code comments. Rather, describe the purpose of this block. Also, please spell out `resp.` -- I can't figure out what it is supposed to mean.

modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/VirtualScrollBar.java line 151:

> 149:                     IndexedCell cell = firstVisibleCell;
> 150:                     if (cell == null)
> 151:                         return;

Unless the if statement, including its target, is entirely on one line, you _must_ enclose the target of the `if` with curly braces, like this:


                    if (cell == null) {
                        return;
                    }


Since it fits on one line, the prior code is also acceptable:


                    if (cell == null) return;

modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/VirtualScrollBar.java line 156:

> 154:                     IndexedCell cell = lastVisibleCell;
> 155:                     if (cell == null)
> 156:                         return;

Either revert this or enclose in curly braces.

modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java line 2595:

> 2593:         if (recreate && (lengthBar.isVisible() || Properties.IS_TOUCH_SUPPORTED)) {
> 2594:             final int cellCount = getCellCount();
> 2595:             float numCellsVisibleOnScreen = 0;

Can you explain why this needs to be in floating point?

Presuming that it does need to be in floating-point, I recommend making this a double to match `lengthToAdd`.

modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java line 2602:

> 2600:                     sumCellLength += lengthToAdd;
> 2601:                     if (sumCellLength > flowLength) {
> 2602:                         numCellsVisibleOnScreen += ( 1 - (sumCellLength - flowLength )/ lengthToAdd);

Minor: remove the spaces after the `(` and before the `)`, and add a space before the `/` like this:


                        numCellsVisibleOnScreen += (1 - (sumCellLength - flowLength ) / lengthToAdd);

modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java line 2611:

> 2609: 
> 2610:             lengthBar.setMax(1);
> 2611:             lengthBar.setVisibleAmount( numCellsVisibleOnScreen / (float) cellCount);

I'm concerned with the change to no longer use the estimated size. This will produce very different results for tables with variable row hights. @johanvos will very likely want to comment on this.

Also, The cast is unnecessary.

Minor: remove the space after the `(`

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

> 1: /*
> 2:  * Copyright (c) 2018, 2022, Oracle and/or its affiliates. All rights reserved.

Update the "last touched year" to 2023. Also, unless this is substantially the same as some other class that you copied, you can change the `2018,` to `2022,`.

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

> 97:         } catch (InterruptedException e1) {
> 98:             e1.printStackTrace();
> 99:         }

You can change this to `Util.sleep(1000);` and eliminate the try/catch here and below.

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

> 106:             latch.countDown();
> 107:         });
> 108:         Util.waitForLatch(latch, 5, "Timeout while waiting for mouse click");

Suggestion: If you change `Platform.runLater` to `Util.runAndWait` you can eliminate the latch (it will do that for you).

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

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