Dominik Honnef: 6 widget: constrain drag offset to [0, 1] widget: correctly set s.dragging to false when releasing drag widget: clicking on the scrollbar indicator shouldn't jump widget: consider size of indicator when limiting scrollbar dragging widget: move scrollbar indicator if dragging starts outside of it widget: when clicking on scrollbar, center on that point 6 files changed, 69 insertions(+), 16 deletions(-)
Thanks, merged with small tweak. On Thu Jun 30, 2022 at 01:21, Dominik Honnef wrote:
Thanks, merged. Elias
Thanks, merged. Elias
Thanks, merged. Elias
gio/patches: SUCCESS in 19m26s [widget: constrain drag offset to [0, 1]][0] from [Dominik Honnef][1] [0]: https://lists.sr.ht/~eliasnaur/gio-patches/patches/33396 [1]: mailto:dominik@honnef.co ✓ #790902 SUCCESS gio/patches/linux.yml https://builds.sr.ht/~eliasnaur/job/790902 ✓ #790900 SUCCESS gio/patches/apple.yml https://builds.sr.ht/~eliasnaur/job/790900 ✓ #790903 SUCCESS gio/patches/openbsd.yml https://builds.sr.ht/~eliasnaur/job/790903 ✓ #790901 SUCCESS gio/patches/freebsd.yml https://builds.sr.ht/~eliasnaur/job/790901
Thanks, merged. Elias
Thank you for fixing these Dominik! I finally had a chance to look through them in depth. Cheers, Chris
Copy & paste the following snippet into your terminal to import this patchset into git:
curl -s https://lists.sr.ht/~eliasnaur/gio-patches/patches/33396/mbox | git am -3Learn more about email & git
Once the user begins dragging, the cursor can move outside the clip area (or even the window on at least X11), leading to events with positions that are either negative, or larger than the clip area. Negative values outright break the delta tracking and cause the scrollbar to misbehave. Positive values "only" break the invariant of Scrollbar.ScrollDistance that the returned value is in the range [-1, 1]. Signed-off-by: Dominik Honnef <dominik@honnef.co> --- widget/list.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/widget/list.go b/widget/list.go index 962eea7e..224b8321 100644 --- a/widget/list.go +++ b/widget/list.go @@ -74,6 +74,12 @@ func (s *Scrollbar) Layout(gtx layout.Context, axis layout.Axis, viewportStart, continue } dragOffset := axis.FConvert(event.Position).X + // The user can drag outside of the constraints, or even the window. Limit dragging to within the scrollbar. + if dragOffset < 0 { + dragOffset = 0 + } else if dragOffset > trackHeight { + dragOffset = trackHeight + } normalizedDragOffset := dragOffset / trackHeight if !s.dragging { -- 2.36.1
Thanks, merged. Elias
Before, we would set s.dragging to false on pointer.Release and then immediately set it back to true because we were processing the event and saw that s.dragging was false. Signed-off-by: Dominik Honnef <dominik@honnef.co> --- widget/list.go | 1 + 1 file changed, 1 insertion(+) diff --git a/widget/list.go b/widget/list.go index 224b8321..ccbde107 100644 --- a/widget/list.go +++ b/widget/list.go @@ -67,6 +67,7 @@ func (s *Scrollbar) Layout(gtx layout.Context, axis layout.Axis, viewportStart, case pointer.Drag: case pointer.Release: s.dragging = false + continue case pointer.Cancel: s.dragging = false continue
I merged the switch cases instead.
-- 2.36.1
Thanks, merged with small tweak. On Thu Jun 30, 2022 at 01:21, Dominik Honnef wrote:
Signed-off-by: Dominik Honnef <dominik@honnef.co> --- widget/list.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/widget/list.go b/widget/list.go index ccbde107..42425db2 100644 --- a/widget/list.go +++ b/widget/list.go @@ -58,7 +58,11 @@ func (s *Scrollbar) Layout(gtx layout.Context, axis layout.Axis, viewportStart, Y: int(event.Position.Y), }) normalizedPos := float32(pos.X) / trackHeight - s.delta += normalizedPos - viewportStart + // Clicking on the indicator should not jump to that position on the track. The user might've just intended to + // drag and changed their mind. + if !(normalizedPos >= viewportStart && normalizedPos <= viewportEnd) { + s.delta += normalizedPos - viewportStart + } } // Offset to account for any drags. -- 2.36.1
Thanks, merged. Elias
Signed-off-by: Dominik Honnef <dominik@honnef.co> --- widget/list.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/widget/list.go b/widget/list.go index 42425db2..43852196 100644 --- a/widget/list.go +++ b/widget/list.go @@ -92,6 +92,18 @@ func (s *Scrollbar) Layout(gtx layout.Context, axis layout.Axis, viewportStart, s.oldDragPos = normalizedDragOffset } s.delta += normalizedDragOffset - s.oldDragPos + + if viewportStart+s.delta < 0 { + // Adjust normalizedDragOffset - and thus the future s.oldDragPos - so that futile dragging up has to be + // countered with dragging down again. Otherwise, dragging up would have no effect, but dragging down would + // immediately start scrolling. We want the user to undo their ineffective drag first. + normalizedDragOffset -= viewportStart + s.delta + // Limit s.delta to the maximum amount scrollable + s.delta = -viewportStart + } else if viewportEnd+s.delta > 1 { + normalizedDragOffset += (1 - viewportEnd) - s.delta + s.delta = 1 - viewportEnd + } s.oldDragPos = normalizedDragOffset } -- 2.36.1
Thanks, merged. Elias
Signed-off-by: Dominik Honnef <dominik@honnef.co> --- widget/list.go | 42 +++++++++++++++++++++++++++++------------- 1 file changed, 29 insertions(+), 13 deletions(-) diff --git a/widget/list.go b/widget/list.go index 43852196..56dc25b0 100644 --- a/widget/list.go +++ b/widget/list.go @@ -90,21 +90,37 @@ func (s *Scrollbar) Layout(gtx layout.Context, axis layout.Axis, viewportStart, if !s.dragging { s.dragging = true s.oldDragPos = normalizedDragOffset - } - s.delta += normalizedDragOffset - s.oldDragPos - if viewportStart+s.delta < 0 { - // Adjust normalizedDragOffset - and thus the future s.oldDragPos - so that futile dragging up has to be - // countered with dragging down again. Otherwise, dragging up would have no effect, but dragging down would - // immediately start scrolling. We want the user to undo their ineffective drag first. - normalizedDragOffset -= viewportStart + s.delta - // Limit s.delta to the maximum amount scrollable - s.delta = -viewportStart - } else if viewportEnd+s.delta > 1 { - normalizedDragOffset += (1 - viewportEnd) - s.delta - s.delta = 1 - viewportEnd + if normalizedDragOffset < viewportStart || normalizedDragOffset > viewportEnd { + // The user started dragging somewhere on the track that isn't covered by the indicator. Consider this a + // click in addition to a drag and jump to the clicked point. + // + // TODO(dh): this isn't perfect. We only get the pointer.Drag event once the user has actually dragged, + // which means that if the user presses the mouse button and neither releases it nor drags it, nothing + // will happen. + pos := axis.Convert(image.Point{ + X: int(event.Position.X), + Y: int(event.Position.Y), + }) + normalizedPos := float32(pos.X) / trackHeight + s.delta += normalizedPos - viewportStart + } + } else { + s.delta += normalizedDragOffset - s.oldDragPos + + if viewportStart+s.delta < 0 { + // Adjust normalizedDragOffset - and thus the future s.oldDragPos - so that futile dragging up has to be + // countered with dragging down again. Otherwise, dragging up would have no effect, but dragging down would + // immediately start scrolling. We want the user to undo their ineffective drag first. + normalizedDragOffset -= viewportStart + s.delta + // Limit s.delta to the maximum amount scrollable + s.delta = -viewportStart + } else if viewportEnd+s.delta > 1 { + normalizedDragOffset += (1 - viewportEnd) - s.delta + s.delta = 1 - viewportEnd + } + s.oldDragPos = normalizedDragOffset } - s.oldDragPos = normalizedDragOffset } // Process events from the indicator so that hover is -- 2.36.1
Thanks, merged. Elias
Previously, we'd scroll so the new viewportStart corresponded to the clicked position. This felt okay if clicking above the current indicator, but felt jarring when clicking below it. Centering gives a consistent behavior regardless of the scroll direction. Signed-off-by: Dominik Honnef <dominik@honnef.co> --- widget/list.go | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/widget/list.go b/widget/list.go index 56dc25b0..ae395736 100644 --- a/widget/list.go +++ b/widget/list.go @@ -46,6 +46,20 @@ func (s *Scrollbar) Layout(gtx layout.Context, axis layout.Axis, viewportStart, trackHeight := float32(axis.Convert(gtx.Constraints.Max).X) s.delta = 0 + centerOnClick := func(normalizedPos float32) { + // When the user clicks on the scrollbar we center on that point, respecting the limits of the beginning and end + // of the scrollbar. + // + // Centering gives a consistent experience whether the user clicks above or below the indicator. + target := normalizedPos - (viewportEnd-viewportStart)/2 + s.delta += target - viewportStart + if s.delta < -viewportStart { + s.delta = -viewportStart + } else if s.delta > 1-viewportEnd { + s.delta = 1 - viewportEnd + } + } + // Jump to a click in the track. for _, event := range s.track.Events(gtx) { if event.Type != gesture.TypeClick || @@ -61,7 +75,7 @@ func (s *Scrollbar) Layout(gtx layout.Context, axis layout.Axis, viewportStart, // Clicking on the indicator should not jump to that position on the track. The user might've just intended to // drag and changed their mind. if !(normalizedPos >= viewportStart && normalizedPos <= viewportEnd) { - s.delta += normalizedPos - viewportStart + centerOnClick(normalizedPos) } } @@ -103,7 +117,7 @@ func (s *Scrollbar) Layout(gtx layout.Context, axis layout.Axis, viewportStart, Y: int(event.Position.Y), }) normalizedPos := float32(pos.X) / trackHeight - s.delta += normalizedPos - viewportStart + centerOnClick(normalizedPos) } } else { s.delta += normalizedDragOffset - s.oldDragPos -- 2.36.1
builds.sr.ht <builds@sr.ht>gio/patches: SUCCESS in 19m26s [widget: constrain drag offset to [0, 1]][0] from [Dominik Honnef][1] [0]: https://lists.sr.ht/~eliasnaur/gio-patches/patches/33396 [1]: mailto:dominik@honnef.co ✓ #790902 SUCCESS gio/patches/linux.yml https://builds.sr.ht/~eliasnaur/job/790902 ✓ #790900 SUCCESS gio/patches/apple.yml https://builds.sr.ht/~eliasnaur/job/790900 ✓ #790903 SUCCESS gio/patches/openbsd.yml https://builds.sr.ht/~eliasnaur/job/790903 ✓ #790901 SUCCESS gio/patches/freebsd.yml https://builds.sr.ht/~eliasnaur/job/790901
Thanks, merged. Elias