On Tue, 9 Feb 2021 12:21:28 GMT, Johan Vos <j...@openjdk.org> wrote:
> This PR introduces a refactory for VirtualFlow, fixing a number of issues
> reported about inconsistent scrolling speed (see
> https://bugs.openjdk.java.net/browse/JDK-8089589)
> The problem mentioned in the JBS issue (and in related issues) is that the
> VirtualFlow implementation depends on cell index and cell count, instead of
> on pixel count. The latter is unknown when the VirtualFlow is created, and
> pre-calculating the size of a large set of items would be very expensive.
> Therefore, this PR uses a combination of a gradual calculation of the total
> size in pixels (estimatedSize) and a smoothing part that prevents the
> scrollback to scroll in the reverse direction as the requested change.
> This PR currently breaks a number of tests that hard-coded depend on a number
> of evaluations. This is inherit to the approach of this PR: if we want to
> estimate the total size, we need to do some additional calculations. In this
> PR, I try to balance between consistent behavior and performance.
I reviewed the code, and tested this, both using automated tests and various
manual tests, and it looks good to me. In addition to the simple test attached
to the bug report, I can reproduce the bug with `HelloTableView` and the
`HorizontalListView` sample of `Ensemble8`.
`HelloTableView`
1. Run `HelloTableview` program
2. Sort by first name
3. Slowly scroll down by dragging the thumb
BUG: the scrolling jitters slightly when you reach the taller row (Jacob Smith
Smith Smith)
`Ensemble8`
1. Run `Ensemble8` and select `HorizontalListView` sample
2. Scroll all the way to the right
3. Slowly scroll to the left by dragging the thumb
BUG: the scrolling jitters slightly when you reach the wider row (Long Row 3)
All three test cases run fine with the fix.
I left a few inline questions and comments (mostly about code style w.r.t.,
inline whitespace, and a couple typos).
modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java
line 254:
> 252: * The following relation should always be true:
> 253: * 0 <= absoluteOffset <= (estimatedSize - viewportLength)
> 254: * Based on this relation, he position p is defined as
Typo: `the` position p...
modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java
line 1034:
> 1032: */
> 1033: void adjustAbsoluteOffset() {
> 1034: absoluteOffset = (estimatedSize - viewportLength)*
> getPosition();
Minor: missing space before the `*`
modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java
line 2883:
> 2881: double newPosition = Math.min(1.0d,
> absoluteOffset/(estimatedSize - viewportLength));
> 2882: // estimatedSize changes may result in opposite effect on
> position
> 2883: // in that case, modify current position 1% in the requested
> direction
Have you done testing to show that a 1% adjustment is a good heuristic?
modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java
line 2892:
> 2890:
> 2891: // once at 95% of the total estimated size, we want a correct
> size, not
> 2892: // an estimated size anymore.
I presume that this is a balance between performance and accuracy? It seems to
work well. Have you tried other values?
Do you also need to check for `newPosition < 0.05` if you are scrolling
backwards?
modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java
line 2901:
> 2899: }
> 2900:
> 2901: // if we are at or beyond the edge, correct the absolteOffset
Typo: should be `absoluteOffset`
modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java
line 2902:
> 2900:
> 2901: // if we are at or beyond the edge, correct the absolteOffset
> 2902: if (newPosition >= 1.d) {
Do you need to also check for `newPosition < 0` if you are scrolling backwards?
modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java
line 2914:
> 2912: double total = 0;
> 2913: int currentCellCount = getCellCount();
> 2914: double estSize = estimatedSize/currentCellCount;
MInor: missing spaces before and after `/`
modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java
line 2972:
> 2970: // Make sure we have enough space in the cache to store this
> index
> 2971: while (idx >= itemSizeCache.size()) {
> 2972: itemSizeCache.add(itemSizeCache.size(), null);
Can this be simplified to `itemSizeCache.add(null);`?
modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/VirtualFlowTest.java
line 1229:
> 1227: }
> 1228:
> 1229: private ArrayLinkedListShim<GraphicalCellStub> circlelist= new
> ArrayLinkedListShim<GraphicalCellStub>();
Minor: missing space before `=`
modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/VirtualFlowTest.java
line 1233:
> 1231: private VirtualFlowShim createCircleFlow() {
> 1232: // The second VirtualFlow we are going to test, with 7
> cells. Each cell
> 1233: // contains a Circle whith a radius that varies between cells.
Minor: indentation
modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/VirtualFlowTest.java
line 1305:
> 1303: double s1 = s0;
> 1304: double position = vf.getPosition();
> 1305: double newPosition =0d;
Minor: missing space after `=`
modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/VirtualFlowTest.java
line 1336:
> 1334: static ArrayList<Circle> circleList = new ArrayList<>();
> 1335: static {
> 1336: circleList.add(new Circle(10));
Minor: Could use `List.of`? (just a suggestion)
modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/VirtualFlowTest.java
line 1350:
> 1348: public GraphicalCellStub() { init(); }
> 1349:
> 1350: private void init( ) {
Minor: can remove extra space between `(` and `)`
modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/VirtualFlowTest.java
line 1381:
> 1379: double answer = super.computePrefHeight(width);
> 1380: if ((idx > -1) && (idx < circleList.size())){
> 1381: answer = 2* circleList.get(idx).getRadius()+ 6;
Minor: spacing is a little inconsistent for these two lines.
-------------
PR: https://git.openjdk.java.net/jfx/pull/398
