On Sun, 22 Jan 2023 15:34:25 GMT, JoachimSchriek <d...@openjdk.org> wrote:
>> This is my (joachim.schr...@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
