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 Code changes are OK. I have noted a few minor comments. modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java line 1305: > 1303: // Otherwise, our goal is to leave the index of the cell at > the > 1304: // top consistent, with the same translation etc. > 1305: if (position != 0 && position != 1 && (currentIndex >= > cellCount)) { Comparing a double for equality or inequality is not the best coding practice. Anyway, I see this pattern throughout this file. We can live with it for now. modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/VirtualFlowTest.java line 658: > 656: * laid out properly. > 657: */ > 658: @Test Only new line is added after @Test tag. There are a lot of changes of this type which I presume were done by the IDE. I think, we can revert these safely as not to cause unnecessary diffs. 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. Typo : "whith" -> "with" modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/VirtualFlowTest.java line 1282: > 1280: flow.scrollPixels(-50); > 1281: double neg = flow.getPosition(); > 1282: assertFalse("Moving in negative direction should not decrease > position", neg > pos); "decrease" in log message should be "increase" modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/VirtualFlowTest.java line 1294: > 1292: vf.scrollPixels(-50); > 1293: double neg = vf.getPosition(); > 1294: assertFalse("Moving in negative direction should not decrease > position", neg > pos); "decrease" in log message should be "increase" ------------- PR: https://git.openjdk.java.net/jfx/pull/398
