[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-openjfx-dev
Subject:    Re: RFR: 8276167: VirtualFlow.scrollToTop doesn't scroll to the top of the last element [v3]
From:       Johan Vos <jvos () openjdk ! java ! net>
Date:       2021-12-10 15:04:18
Message-ID: nfrJxL22Qt1Th4UJxxjetRXEu0mYhAz6gB9JTS_-hJg=.cf48b3e6-787f-496e-a696-f6b29ed95459 () github ! com
[Download RAW message or body]

On Fri, 10 Dec 2021 13:34:53 GMT, eduardsdv <duke@openjdk.java.net> wrote:

> > modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java \
> > line 1584: 
> > > 1582:         boolean posSet = false;
> > > 1583: 
> > > 1584:         if (index > getCellCount() - 1) {
> > 
> > I agree that the previous code violates the contract that the flow should show \
> > the specified cell aligned to the top. 
> > I wonder though if this test is needed at all. What is the goal of providing an \
> > index that is larger than getCellCount() -1?
> 
> This part was originally copied from the 'VirtualContainerBase'. There was this \
> comment: _special-case the 'greater than row count' condition to have it be \
> perfectly at position 1_. 
> Maybe to avoid throwing the IndexOutOfBoundsException or simply to scroll to the \
> end.

I am ok with this behavior, but then it should be specified in the javadoc for \
`@parameter index` that the value can be greater than row count, and that in that \
case the view will be positioned at the very end. @kevinrushforth do you have an \
opinion on this? This is unrelated to the current fix, so that should not stop this \
PR.

> > modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewKeyInputTest.java \
> > line 1844: 
> > > 1842:         }
> > > 1843:         listView.setPrefHeight(130); // roughly room for four rows
> > > 1844:         Toolkit.getToolkit().firePulse();
> > 
> > Why is this needed here?
> 
> Without this line the test fails in the line 1865 with the message 'expected:<7> \
> but was:<8>'. I think this is because in the line 1850 'listView.scrollTo(99)' is \
> executed, which now does not set the position to 1. 
> It can also be caused by the https://bugs.openjdk.java.net/browse/JDK-8277785. When \
> it is solved, this call will probably no longer be necessary.

That is very well possible indeed. Since we estimate the size of the different cells, \
it is hard to deterministically put values on the counts. Therefor, I changed a \
number of tests from `equals` to `<` . I would personally suggest not adding the \
`pulse` here, but rather change the test in line 1865 with `... < 10 ` to make sure \
that there is no abnormal amount of processing. (and maybe also `> 0 ` to make sure \
that the updateItem is at least counted once).

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

PR: https://git.openjdk.java.net/jfx/pull/656


[prev in list] [next in list] [prev in thread] [next in thread]