~eliasnaur/gio-patches

This thread contains a patchset. You're looking at the original emails, but you may wish to use the patch review UI. Review patch
4 4

[PATCH gio 0/1] v3 layout: implement list.ScrollTo

~whereswaldon
Details
Message ID
<160104445236.26083.2834620510949644290-0@git.sr.ht>
DKIM signature
missing
Download raw message
I've been bugging Larry off and on about this feature for months now,
and I offered to attempt to synthesize the feedback that Elias provided
on the last iteration myself. Larry accepted my offer, and here we are.
The last version of the patch is here:
https://lists.sr.ht/~eliasnaur/gio-
patches/%3C20200720130808.48510-1-larry%40theclapp.org%3E#%3CC4QW8FQ0ZFZC.1RVWXDLW3QZGD@testmac%3E
Changes:

- I've simplified the patch by only working on ScrollTo and
removing ScrollPage. I still want the ScrollPage feature, but I think it
can be implemented in a later patch.
- I've adopted the recommended
state tracking approach of making the zero of the List value useful
while also tracking the most recent scrollto index.
- I've removed
helper functions that were deemed unnecessary.

Notable choices:

-
I opted to leave the `last` field within the Position type and to also
leave the accessor method for it. These are not strictly necessary for
the functionality of the list type, but they're extremely useful to
external code that incorporates a list within its layout. I've had to
hack around the lack this a frustrating number of times, and I'd rather
the list provide it (my previous hack often overcounted). I'm happy to
discuss this, but I think a feature like this is necessary.

Larry's
previous iteration of this patchset was signed-off by him, and this
patch is a transformation of that signed-off by me. I'm not sure whether
we need to also have Larry sign off this one? I thought about sending a
patch on top of Larry's, but that seemed like it would make review
unnecessarily complicated.

As always, I welcome your feedback.
Cheers,
Chris

Larry Clapp (1):
  layout: implement list.ScrollTo

 layout/list.go      |  76 +++++++++++++++++++--
 layout/list_test.go | 162 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 233 insertions(+), 5 deletions(-)
 create mode 100644 layout/list_test.go

-- 
2.26.2

[PATCH gio 1/1] layout: implement list.ScrollTo

~whereswaldon
Details
Message ID
<160104445236.26083.2834620510949644290-1@git.sr.ht>
In-Reply-To
<160104445236.26083.2834620510949644290-0@git.sr.ht> (view parent)
DKIM signature
missing
Download raw message
Patch: +233 -5
From: Larry Clapp <larry@theclapp.org>

Co-authored-by: Chris Waldon <christopher.waldon.dev@gmail.com>
Signed-off-by: Chris Waldon <christopher.waldon.dev@gmail.com>
---
 layout/list.go      |  76 +++++++++++++++++++--
 layout/list_test.go | 162 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 233 insertions(+), 5 deletions(-)
 create mode 100644 layout/list_test.go

diff --git a/layout/list.go b/layout/list.go
index 2375fea..1aae45c 100644
--- a/layout/list.go
+++ b/layout/list.go
@@ -23,9 +23,15 @@ type List struct {
	Axis Axis
	// ScrollToEnd instructs the list to stay scrolled to the far end position
	// once reached. A List with ScrollToEnd == true and Position.BeforeEnd ==
	// false draws its content with the last item at the bottom of the list
	// area.
	// false draws its content with the last item at the end of the list area.
	ScrollToEnd bool

	// scrollToItemPlusOne is the last item passed to ScrollTo, plus
	// one to keep the zero List useful. It is reset by Layout. Zero indicates
	// no scrolling requests since the last layout, and higher values indicate
	// requests to scroll to index value-1.
	scrollToItemPlusOne int

	// Alignment is the cross axis alignment of list elements.
	Alignment Alignment

@@ -36,7 +42,7 @@ type List struct {
	// Position is updated during Layout. To save the list scroll position,
	// just save Position after Layout finishes. To scroll the list
	// programatically, update Position (e.g. restore it from a saved value)
	// before calling Layout.
	// before calling Layout, or use ScrollTo and related functions.
	Position Position

	len int
@@ -66,8 +72,10 @@ type Position struct {
	BeforeEnd bool
	// First is the index of the first visible child.
	First int
	// last is the index of the last visible child.
	last int
	// Offset is the distance in pixels from the top edge to the child at index
	// First.
	// First. Positive offsets are before (above or left of) the window edge.
	Offset int
}

@@ -95,11 +103,37 @@ func (l *List) init(gtx Context, len int) {
	}
}

func (l *List) processScrollRequests() {
	item := l.scrollToItemPlusOne - 1
	if l.scrollToItemPlusOne <= 0 {
		return
	}
	if l.Position.First < item && item < l.Position.last {
		return
	}
	if (l.Position.First > 0 || l.Position.Offset > 0) && item <= l.Position.First {
		// Item is before, or equal to, the first item drawn. Draw item at
		// offset 0, at the beginning of the list, and go forward.
		l.Position.First = item
		l.Position.Offset = 0
		l.Position.BeforeEnd = true
		return
	}
	if item >= l.Position.last {
		// Item is after the last item drawn. Draw the end of item at the end of
		// the list, and go backward.
		l.Position.First = item + 1
	}
}

// Layout the List.
func (l *List) Layout(gtx Context, len int, w ListElement) Dimensions {
	l.init(gtx, len)
	crossMin, crossMax := axisCrossConstraint(l.Axis, gtx.Constraints)
	gtx.Constraints = axisConstraints(l.Axis, 0, inf, crossMin, crossMax)

	l.processScrollRequests()

	macro := op.Record(gtx.Ops)
	for l.next(); l.more(); l.next() {
		child := op.Record(gtx.Ops)
@@ -119,6 +153,31 @@ func (l *List) Dragging() bool {
	return l.scroll.State() == gesture.StateDragging
}

// ScrollTo makes sure list index item i is in view.
//
// If it's above the top, it becomes the top item. If it's below the bottom,
// it becomes the bottom item, with said item positioned starting at the end of the
// item. (This means that if the item is taller/longer than the list area, the
// beginning of the item will be out of view.)
//
// If i < 0, uses 0.
//
// If you ScrollTo(n) and then layout a list shorter than n, Layout scrolls to
// the end of the list.
func (l *List) ScrollTo(item int) {
	if item < 0 {
		return
	}
	if l.len == 0 {
		return
	}
	if item >= l.len {
		l.scrollToItemPlusOne = l.len
		return
	}
	l.scrollToItemPlusOne = item + 1
}

func (l *List) update() {
	d := l.scroll.Scroll(l.ctx.Metric, l.ctx, l.ctx.Now, gesture.Axis(l.Axis))
	l.scrollDelta = d
@@ -158,7 +217,8 @@ func (l *List) nextDir() iterationDir {
	_, vsize := axisMainConstraint(l.Axis, l.ctx.Constraints)
	last := l.Position.First + len(l.children)
	// Clamp offset.
	if l.maxSize-l.Position.Offset < vsize && last == l.len {
	if l.maxSize-l.Position.Offset < vsize &&
		(last == l.len || (l.scrollToItemPlusOne == last)) {
		l.Position.Offset = l.maxSize - vsize
	}
	if l.Position.Offset < 0 && l.Position.First == 0 {
@@ -230,6 +290,11 @@ func (l *List) layout(macro op.MacroOp) Dimensions {
	if space := mainMax - size; l.ScrollToEnd && space > 0 {
		pos += space
	}
	if len(children) == 0 {
		l.Position.last = 0
	} else {
		l.Position.last = l.Position.First + len(children) - 1
	}
	for _, child := range children {
		sz := child.size
		var cross int
@@ -277,5 +342,6 @@ func (l *List) layout(macro op.MacroOp) Dimensions {
	pointer.Rect(image.Rectangle{Max: dims}).Add(ops)
	l.scroll.Add(ops)
	call.Add(ops)
	l.scrollToItemPlusOne = 0
	return Dimensions{Size: dims}
}
diff --git a/layout/list_test.go b/layout/list_test.go
new file mode 100644
index 0000000..a3ae8bb
--- /dev/null
+++ b/layout/list_test.go
@@ -0,0 +1,162 @@
package layout

import (
	"image"
	"testing"

	"gioui.org/op"
)

func TestScrollFunctions(t *testing.T) {
	gtx := Context{
		Ops: new(op.Ops),
		Constraints: Constraints{
			Max: image.Pt(1000, 1000),
		},
	}

	l := List{Axis: Vertical}
	listLen := 1000
	layoutList := func(gtx Context) Dimensions {
		return l.Layout(gtx, listLen, func(gtx Context, i int) Dimensions {
			var dims Dimensions
			switch i {
			case 24:
				// Item is really tall: 3x the window size
				dims.Size = image.Pt(1000, 3000)
			default:
				dims.Size = image.Pt(1000, 100)
			}
			return dims
		})
	}
	checkFirstLast := func(t *testing.T, first, last int) {
		t.Helper()
		check(t, "first", first, l.Position.First)
		check(t, "last", last, l.Position.last)
	}

	t.Run("ScrollTo", func(t *testing.T) {
		dims := layoutList(gtx)
		check(t, "size", image.Pt(1000, 1000), dims.Size)
		checkFirstLast(t, 0, 9)

		// ScrollTo an item that's already in view
		l.ScrollTo(1)
		layoutList(gtx)
		checkFirstLast(t, 0, 9)

		// ScrollTo an item that's not in view -- in this case, should shift down
		// one item.
		l.ScrollTo(10)
		layoutList(gtx)
		checkFirstLast(t, 1, 10)

		l.ScrollTo(25)
		layoutList(gtx)
		checkFirstLast(t, 24, 25)
	})

	t.Run("Scroll to large item", func(t *testing.T) {
		// Item 24 is 3x as tall as the window: show its bottom.
		l.Position.First = 0
		l.Position.Offset = 0
		layoutList(gtx)
		l.ScrollTo(24)
		layoutList(gtx)
		checkFirstLast(t, 24, 24)
		check(t, "offset", 2000, l.Position.Offset)

		// Go there again to show its top. (Could also just set Position.First = 24
		// & Position.Offset = 0.)
		l.ScrollTo(24)
		layoutList(gtx)
		checkFirstLast(t, 24, 24)
		check(t, "offset", 0, l.Position.Offset)

		// Starting from the end of the list, scroll back to item 24: make sure
		// we're at the beginning of the item.
		l.ScrollTo(1000)
		layoutList(gtx)
		l.ScrollTo(24)
		layoutList(gtx)
		checkFirstLast(t, 24, 24)
		check(t, "offset", 0, l.Position.Offset)

	})

	t.Run("ScrollToEnd", func(t *testing.T) {
		l.ScrollToEnd = true
		l.Position.BeforeEnd = false

		// Draw from the end and go back.
		layoutList(gtx)
		checkFirstLast(t, 990, 999)

		// Add an item: still draws at end.
		listLen++
		layoutList(gtx)
		checkFirstLast(t, 991, 1000)

		// Remove the item: still at end.
		listLen--
		layoutList(gtx)
		checkFirstLast(t, 990, 999)
	})

	t.Run("Small list", func(t *testing.T) {
		l.ScrollToEnd = false

		t.Run("len=0", func(t *testing.T) {
			listLen = 0
			layoutList(gtx)
			checkFirstLast(t, 0, 0)

			l.ScrollTo(1)
			layoutList(gtx)
			checkFirstLast(t, 0, 0)
		})
		t.Run("len=1", func(t *testing.T) {
			listLen = 1
			layoutList(gtx)
			checkFirstLast(t, 0, 0)

			l.ScrollTo(2)
			layoutList(gtx)
			checkFirstLast(t, 0, 0)
		})
		t.Run("len=5", func(t *testing.T) {
			listLen = 5
			l.ScrollTo(0)
			layoutList(gtx)
			checkFirstLast(t, 0, 4)

			l.ScrollTo(2)
			layoutList(gtx)
			checkFirstLast(t, 0, 4)

			t.Run("ScrollToEnd", func(t *testing.T) {
				l.ScrollToEnd = true
				l.Position.BeforeEnd = false

				layoutList(gtx)
				checkFirstLast(t, 0, 4)

				l.ScrollTo(2)
				layoutList(gtx)
				checkFirstLast(t, 0, 4)

				l.ScrollTo(10)
				layoutList(gtx)
				checkFirstLast(t, 0, 4)
			})
		})
	})
}

func check(t *testing.T, description string, exp, got interface{}) {
	t.Helper()
	if exp != got {
		t.Errorf("Expected %v, got %v for %s", exp, got, description)
	}
}
-- 
2.26.2

[gio/patches] build failed

builds.sr.ht
Details
Message ID
<C5WJ3OF8WXC7.1WAF7XNPKTZW2@cirno2>
In-Reply-To
<160104445236.26083.2834620510949644290-1@git.sr.ht> (view parent)
DKIM signature
missing
Download raw message
gio/patches: FAILED in 9m50s

[v3 layout: implement list.ScrollTo][0] from [~whereswaldon][1]

[0]: https://lists.sr.ht/~eliasnaur/gio-patches/patches/14082
[1]: mailto:christopher.waldon.dev@gmail.com

✗ #308766 FAILED  gio/patches/apple.yml   https://builds.sr.ht/~eliasnaur/job/308766
✓ #308767 SUCCESS gio/patches/freebsd.yml https://builds.sr.ht/~eliasnaur/job/308767
✓ #308769 SUCCESS gio/patches/openbsd.yml https://builds.sr.ht/~eliasnaur/job/308769
✓ #308768 SUCCESS gio/patches/linux.yml   https://builds.sr.ht/~eliasnaur/job/308768
Details
Message ID
<CAE_4BPA7gDsL3jNOdco+D6HCrv58dETfZqTMCZ-i6UNT79zn2Q@mail.gmail.com>
In-Reply-To
<160104445236.26083.2834620510949644290-0@git.sr.ht> (view parent)
DKIM signature
missing
Download raw message
All of what Chris says below is true.

Thanks, Chris!

Signed-off-by: Larry Clapp <larry@theclapp.org>

On Fri, Sep 25, 2020 at 10:34 AM ~whereswaldon <whereswaldon@git.sr.ht> wrote:
>
> I've been bugging Larry off and on about this feature for months now,
> and I offered to attempt to synthesize the feedback that Elias provided
> on the last iteration myself. Larry accepted my offer, and here we are.
> The last version of the patch is here:
> https://lists.sr.ht/~eliasnaur/gio-
> patches/%3C20200720130808.48510-1-larry%40theclapp.org%3E#%3CC4QW8FQ0ZFZC.1RVWXDLW3QZGD@testmac%3E
> Changes:
>
> - I've simplified the patch by only working on ScrollTo and
> removing ScrollPage. I still want the ScrollPage feature, but I think it
> can be implemented in a later patch.
> - I've adopted the recommended
> state tracking approach of making the zero of the List value useful
> while also tracking the most recent scrollto index.
> - I've removed
> helper functions that were deemed unnecessary.
>
> Notable choices:
>
> -
> I opted to leave the `last` field within the Position type and to also
> leave the accessor method for it. These are not strictly necessary for
> the functionality of the list type, but they're extremely useful to
> external code that incorporates a list within its layout. I've had to
> hack around the lack this a frustrating number of times, and I'd rather
> the list provide it (my previous hack often overcounted). I'm happy to
> discuss this, but I think a feature like this is necessary.
>
> Larry's
> previous iteration of this patchset was signed-off by him, and this
> patch is a transformation of that signed-off by me. I'm not sure whether
> we need to also have Larry sign off this one? I thought about sending a
> patch on top of Larry's, but that seemed like it would make review
> unnecessarily complicated.
>
> As always, I welcome your feedback.
> Cheers,
> Chris
>
> Larry Clapp (1):
>   layout: implement list.ScrollTo
>
>  layout/list.go      |  76 +++++++++++++++++++--
>  layout/list_test.go | 162 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 233 insertions(+), 5 deletions(-)
>  create mode 100644 layout/list_test.go
>
> --
> 2.26.2
Details
Message ID
<CAFcc3FR8B1z_NsYHCOjkkhxZo+sQUVdD9w9_oX1Qmd1Z+1isGQ@mail.gmail.com>
In-Reply-To
<160104445236.26083.2834620510949644290-0@git.sr.ht> (view parent)
DKIM signature
pass
Download raw message
So the tests were all passing on this, but I'm hitting a weird
behavior when trying to incorporate this into an application. I think
I'll need to post a v4 of this soon, but it may take some time to
understand *why* the tests are not catching this (it's a problem that
they explicitly check for).

Anyway, don't merge this version. Sorry for the email noise. I think
the changes are likely to be minor though, so it's probably still
worth reviewing this for high-level feedback.

Cheers,
Chris
Reply to thread Export thread (mbox)