Add smooth/interpolated scrolling to the scroll container #5971
Conversation
|
|
||
| smoothScrollingFactor = 0.5 | ||
|
|
||
| disableSmoothScrollingEnvKey = "FYNE_DISABLE_SMOOTH_SCROLLING" |
There was a problem hiding this comment.
Would this perhaps be better off in fyne_settings instead?
There was a problem hiding this comment.
Maybe? On Slack yesterday Andy didn't even think it was needed at all but I ended up adding it as a relatively easy way to turn it off in unit test code. We do have a precedent of environment variables being used to control things that aren't exposed in fyne_settings, like FYNE_CACHE and FYNE_DISABLE_DPI_DETECTION.
There was a problem hiding this comment.
It may be an unpopular opinion but I'm just kind of against environment variables like this and many of the ones we already have. It feels to me like they never are documented anywhere and just kind of rot away. As such, I get a feeling that very few actually use them so the code becomes more complicated for little gain.
There was a problem hiding this comment.
I could just remove it and test if the fyne.CurrentApp() is the test app?
internal/widget/scroller.go
Outdated
|
|
||
| checkSmoothScrollOnce = sync.Once{} | ||
| checkEnvDisableSmoothScrolling = func() { | ||
| checkSmoothScrollOnce.Do(func() { |
There was a problem hiding this comment.
FYI: We actually can get away with just an if statement and a Boolean now that the code is single threaded.
There was a problem hiding this comment.
Don't we still need to worry about people who haven't completed the migration?
There was a problem hiding this comment.
not really, race condition prevention on the old model is on a "reasonable effort" level now rather than an absolute promise
There was a problem hiding this comment.
This is also a place where a race condition would be of little consequence if it happens. Worst case, it is loaded twice
There was a problem hiding this comment.
I'm a little unsure about the algorithm here.
It looks like it will move a half pixel after each frame following a scroll event - is this assuming a 2x scale factor?
What about differences in how fast the user was scrolling? How does it relate to the inertial scroll on mobile?
|
|
||
| func (s *Scroll) animateScroll(_ float32) { | ||
| diff := s.Offset.Subtract(s.targetOffset) | ||
| if math.Abs(float64(diff.X)) < 0.5 && math.Abs(float64(diff.Y)) < 0.5 { |
There was a problem hiding this comment.
This 0.5 check seems like it should be comparing to the smooth scroll factor instead?
There was a problem hiding this comment.
They're different values which just both happen to be 0.5. This is just an absolute threshold in dp where a scroll will just snap to the requested position instead of animating (or continuing to animate). The smoothScrollingFactor is an exponential decay factor.
There was a problem hiding this comment.
OK thanks - then in this case I have a different question - why is 0.5 the right value and should it too be a named constant?
| // what fraction of the page to scroll when tapping on the scroll bar area | ||
| pageScrollFraction = float32(0.95) | ||
|
|
||
| smoothScrollingFactor = 0.5 |
There was a problem hiding this comment.
Why 0.5? does this assume HiDPI? What about UHDPI like smart phones?
There was a problem hiding this comment.
It's somewhat arbitrary but it's the exponential decay factor of the animation curve. It's pixel-independent. Since we're holding this off until 2.7.1 I may try to implement a more sophisticated ease-in/ease-out curve instead of just this decaying exponential.
There was a problem hiding this comment.
I see - it seems I misread the "*" as "+", apologies.
The curves are already coded into the animation code so maybe using the value parameter in the callback could use that instead of coding it in?
There was a problem hiding this comment.
Yeah I was thinking about that, the issue with that for now is that there's no way to extend a running animation properly (ie when the user continues to scroll so the endpoint target for the animation needs to be adjusted on-the-fly) which is what led to me opening #5970. It doesn't look too hard to fix though so I can look into it for 2.7.1
4 participants
Description:
Use an animation to smooth out the scroll, similar to what is done in web browsers and some other UI frameworks
Checklist: