On Wed, 15 Feb 2023 23:53:51 GMT, Kevin Rushforth <k...@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
