On Mon, 6 Feb 2023 14:15:17 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 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
