On Mon, 15 Jan 2024 08:31:59 GMT, Florian Kirmaier <fkirma...@openjdk.org> wrote:
>> As seen in the unit test of the PR, when we click on the area above/below >> the scrollbar the position jumps - but the jump is now not always consistent. >> In the current version on the last cell - the UI always jumps to the top. In >> the other cases, the assumed default cell height is used. >> >> With this PR, always the default cell height is used, to determine how much >> is scrolled. >> This makes the behavior more consistent. >> >> Especially from the unit-test, it's clear that with this PR the behavior is >> much more consistent. >> >> This is also related to the following PR: >> https://github.com/openjdk/jfx/pull/1194 > > Florian Kirmaier has updated the pull request incrementally with one > additional commit since the last revision: > > JDK-8323511 > reverted accidental indentation chang Intuitively, the added test seems to be the right thing to do. It indeed fails before and succeeds after the patch. However, I'm not sure about the implementation. The suggested patch changes the conceptual idea of `VirtualFlow.scrollTo(int index)` where a negative index is not specified (this is probably what @Maran23 asked at https://github.com/openjdk/jfx/pull/1326#discussion_r1530233902 . The way the `scrollTo(int index)` is modified doesn't sound right to me. In case of the test, where the function is called with `index = -1`, it will first try to obtain the cell at index -2 (which will always return false), and then it will try to obtain the cell at index 0 (the one we need to have indeed), and then scroll X pixels where X is the height of the cell at index -1. In the test, all cells except the one at 0 have height 100, so the cell at -1 will have 100 as well, so it will use that. This seems a complex way to deal with the issue. It would need a clear definition of "default size" and it changes the implicit assumption that scrollTo(index) should be called with a valid index (which does not have to be visible in the current viewport). I believe the first thing that needs to be done, is to extend the "specification" that is currently in comments in the VirtualScrollBar: /* * Scroll one cell further in the direction the user has clicked if only one cell is shown. * Otherwise, a click on the trough would have no effect when cell height > viewport height. */ What does this mean if there is no cell further in the direction the user has clicked? Should we them scroll to the top of the current cell (current behavior), or should we use a more gradual scroll (which is the new behavior, which I believe is better indeed)? If the latter is the preferred case, this looks to a behavior that is more similar to the Event that is received when the mousewheel is used (and which invokes `VirtualFlow.scrollPixels(double delta)`) ------------- PR Comment: https://git.openjdk.org/jfx/pull/1326#issuecomment-2025937722
