On Mon, 12 Apr 2021 09:41:03 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. > > Johan Vos has updated the pull request incrementally with one additional > commit since the last revision: > > Process reviewer comments Thanks for the explanations about the algorithm. I have no more questions on the logic. I think Ajit will review this later this week. Btw, I did another pass over the code and noted a few more formatting inconsistencies (missing spaces). modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java line 1045: > 1043: setPosition(0.); > 1044: } else { > 1045: setPosition(absoluteOffset/(estimatedSize - > viewportLength)); Minor: spacing modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java line 1591: > 1589: adjustPositionToIndex(index); > 1590: // double offset = - computeOffsetForCell(index); > 1591: // adjustByPixelAmount(offset); I forgot to ask about this last time. Is there a reason to leave the former, now commented-out, code here, as opposed to just removing it? modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java line 1903: > 1901: void setViewportLength(double value) { > 1902: this.viewportLength = value; > 1903: this.absoluteOffset = getPosition() * (estimatedSize > -viewportLength); Minor: spacing modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java line 2564: > 2562: lengthBar.setVisibleAmount(flowLength / sumCellLength); > 2563: } else { > 2564: > lengthBar.setVisibleAmount(viewportLength/estimatedSize); Minor: spacing modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java line 2834: > 2832: double p = com.sun.javafx.util.Utils.clamp(0, position, 1); > 2833: double bound = 0d; > 2834: double estSize = estimatedSize/getCellCount(); Minor: spacing modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java line 2839: > 2837: double h = getCellSize(i); > 2838: if (h < 0) h = estSize; > 2839: if (bound +h > absoluteOffset) { Minor: spacing modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java line 2840: > 2838: if (h < 0) h = estSize; > 2839: if (bound +h > absoluteOffset) { > 2840: return absoluteOffset-bound; Minor: spacing modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java line 2854: > 2852: double targetOffset = 0; > 2853: double estSize = estimatedSize/cellCount; > 2854: for (int i = 0; i < index;i++) { Minor: spacing modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java line 2881: > 2879: double origAbsoluteOffset = this.absoluteOffset; > 2880: this.absoluteOffset = Math.max(0.d, this.absoluteOffset + > numPixels); > 2881: double newPosition = Math.min(1.0d, > absoluteOffset/(estimatedSize - viewportLength)); Minor: spacing modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java line 3035: > 3033: } > 3034: } > 3035: this.estimatedSize = cnt == 0 ? 1d: tot * itemCount/cnt; Minor: spacing ------------- PR: https://git.openjdk.java.net/jfx/pull/398
