Re: RFR: 8276313: ScrollPane scroll delta incorrectly depends on content height [v3]
On Thu, 2 Dec 2021 22:49:45 GMT, Michael Strauß wrote: >> This PR fixes an issue where the scroll delta of ScrollPane incorrectly >> depends on the size of its content. >> This leads to extremely slow scrolling when the content is only slightly >> larger than the ScrollPane. > > Michael Strauß has updated the pull request incrementally with one additional > commit since the last revision: > > Changes per review This looks good to me. This new approach is based on the delta between the total height and the visible height, instead of only the total height. It might be good to capture this idea somehow in the commit message, but since it is clear from the code, I am totally ok with not changing the commit message. - Marked as reviewed by jvos (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/660
Re: RFR: 8276313: ScrollPane scroll delta incorrectly depends on content height [v3]
On Thu, 2 Dec 2021 22:49:45 GMT, Michael Strauß wrote: >> This PR fixes an issue where the scroll delta of ScrollPane incorrectly >> depends on the size of its content. >> This leads to extremely slow scrolling when the content is only slightly >> larger than the ScrollPane. > > Michael Strauß has updated the pull request incrementally with one additional > commit since the last revision: > > Changes per review I'll review in the next hours. - PR: https://git.openjdk.java.net/jfx/pull/660
Re: RFR: 8276313: ScrollPane scroll delta incorrectly depends on content height [v3]
On Thu, 2 Dec 2021 22:49:45 GMT, Michael Strauß wrote: >> This PR fixes an issue where the scroll delta of ScrollPane incorrectly >> depends on the size of its content. >> This leads to extremely slow scrolling when the content is only slightly >> larger than the ScrollPane. > > Michael Strauß has updated the pull request incrementally with one additional > commit since the last revision: > > Changes per review Marked as reviewed by aghaisas (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/660
Re: RFR: 8276313: ScrollPane scroll delta incorrectly depends on content height [v3]
On Thu, 2 Dec 2021 22:49:45 GMT, Michael Strauß wrote: >> This PR fixes an issue where the scroll delta of ScrollPane incorrectly >> depends on the size of its content. >> This leads to extremely slow scrolling when the content is only slightly >> larger than the ScrollPane. > > Michael Strauß has updated the pull request incrementally with one additional > commit since the last revision: > > Changes per review Looks good. - Marked as reviewed by kcr (Lead). PR: https://git.openjdk.java.net/jfx/pull/660
Re: RFR: 8276313: ScrollPane scroll delta incorrectly depends on content height [v3]
> This PR fixes an issue where the scroll delta of ScrollPane incorrectly > depends on the size of its content. > This leads to extremely slow scrolling when the content is only slightly > larger than the ScrollPane. Michael Strauß has updated the pull request incrementally with one additional commit since the last revision: Changes per review - Changes: - all: https://git.openjdk.java.net/jfx/pull/660/files - new: https://git.openjdk.java.net/jfx/pull/660/files/147858d7..6b8985f7 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=660=02 - incr: https://webrevs.openjdk.java.net/?repo=jfx=660=01-02 Stats: 12 lines in 1 file changed: 0 ins; 0 del; 12 mod Patch: https://git.openjdk.java.net/jfx/pull/660.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/660/head:pull/660 PR: https://git.openjdk.java.net/jfx/pull/660
Re: RFR: 8276313: ScrollPane scroll delta incorrectly depends on content height [v2]
On Thu, 2 Dec 2021 21:51:59 GMT, Kevin Rushforth wrote: >> Michael Strauß has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Handle division by zero > > modules/javafx.controls/src/main/java/javafx/scene/control/skin/ScrollPaneSkin.java > line 895: > >> 893: double vRange = >> getSkinnable().getVmax()-getSkinnable().getVmin(); >> 894: double vPixelValue = vRange / (nodeHeight - >> contentHeight); >> 895: vPixelValue = Double.isFinite(vPixelValue) ? >> vPixelValue : 0.0; > > I liked the previous logic better and would have just changed the comparison > to check `(nodeHeight - contentHeight) > 0.0` (possibly saving the adjusted > width or height in a local various to avoid doing it twice). As it is, this > is replacing an explicit check on the source values with an out of range > check on the result, which seems less intuitive. Also, the previous code did > a `> 0` test and the new code effectively does a `!= 0` test. I changed it to check the denominator values instead. - PR: https://git.openjdk.java.net/jfx/pull/660
Re: RFR: 8276313: ScrollPane scroll delta incorrectly depends on content height [v2]
On Wed, 1 Dec 2021 09:45:40 GMT, Ajit Ghaisas wrote:
>> Michael Strauß has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Handle division by zero
>
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/ScrollPaneSkin.java
> line 896:
>
>> 894: double vPixelValue;
>> 895: if (nodeHeight > 0.0) {
>> 896: vPixelValue = vRange / (nodeHeight - contentHeight);
>
> This may result in divide by 0.
Good catch! I've also fixed a few similar issues in other places.
-
PR: https://git.openjdk.java.net/jfx/pull/660
Re: RFR: 8276313: ScrollPane scroll delta incorrectly depends on content height [v2]
> This PR fixes an issue where the scroll delta of ScrollPane incorrectly > depends on the size of its content. > This leads to extremely slow scrolling when the content is only slightly > larger than the ScrollPane. Michael Strauß has updated the pull request incrementally with one additional commit since the last revision: Handle division by zero - Changes: - all: https://git.openjdk.java.net/jfx/pull/660/files - new: https://git.openjdk.java.net/jfx/pull/660/files/1605eec4..147858d7 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=660=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=660=00-01 Stats: 19 lines in 1 file changed: 4 ins; 11 del; 4 mod Patch: https://git.openjdk.java.net/jfx/pull/660.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/660/head:pull/660 PR: https://git.openjdk.java.net/jfx/pull/660
Re: RFR: 8276313: ScrollPane scroll delta incorrectly depends on content height
On Tue, 2 Nov 2021 10:49:45 GMT, Michael Strauß wrote:
> This PR fixes an issue where the scroll delta of ScrollPane incorrectly
> depends on the size of its content.
> This leads to extremely slow scrolling when the content is only slightly
> larger than the ScrollPane.
modules/javafx.controls/src/main/java/javafx/scene/control/skin/ScrollPaneSkin.java
line 896:
> 894: double vPixelValue;
> 895: if (nodeHeight > 0.0) {
> 896: vPixelValue = vRange / (nodeHeight - contentHeight);
This may result in divide by 0.
modules/javafx.controls/src/main/java/javafx/scene/control/skin/ScrollPaneSkin.java
line 928:
> 926: double hPixelValue;
> 927: if (nodeWidth > 0.0) {
> 928: hPixelValue = hRange / (nodeWidth - contentWidth);
This may result in divide by 0.
-
PR: https://git.openjdk.java.net/jfx/pull/660
RFR: 8276313: ScrollPane scroll delta incorrectly depends on content height
This PR fixes an issue where the scroll delta of ScrollPane incorrectly depends on the size of its content. This leads to extremely slow scrolling when the content is only slightly larger than the ScrollPane. - Commit messages: - Fixed scrolling speed for ScrollPaneSkin - failing test Changes: https://git.openjdk.java.net/jfx/pull/660/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=660=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8276313 Stats: 52 lines in 2 files changed: 49 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jfx/pull/660.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/660/head:pull/660 PR: https://git.openjdk.java.net/jfx/pull/660
