~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 2

[PATCH] layout: make List scroll position settable

Details
Message ID
<20191125033246.82018-1-larry@theclapp.org>
DKIM signature
missing
Download raw message
Patch: +43 -36
- Added List.LayoutPos, which displays the list starting at the given
  Position, and returns the list's Position after layout (including any
  scrolling).
- Make List.Layout call LayoutPos, for the common case where you don't
  care about setting the list position.

Signed-off-by: Larry Clapp <larry@theclapp.org>
---
 layout/list.go | 79 +++++++++++++++++++++++++++-----------------------
 1 file changed, 43 insertions(+), 36 deletions(-)

diff --git a/layout/list.go b/layout/list.go
index bd4d08a..119e308 100644
--- a/layout/list.go
+++ b/layout/list.go
@@ -23,7 +23,7 @@ type scrollChild struct {
type List struct {
	Axis Axis
	// ScrollToEnd instructs the list to stay scrolled to the far end position
	// once reahed. A List with ScrollToEnd enabled also align its content to
	// once reached. A List with ScrollToEnd enabled also align its content to
	// the end.
	ScrollToEnd bool
	// Alignment is the cross axis alignment of list elements.
@@ -39,12 +39,7 @@ type List struct {
	scroll      gesture.Scroll
	scrollDelta int

	// first is the index of the first visible child.
	first int
	// offset is the signed distance from the top edge
	// to the child with index first.
	offset int

	pos Position
	len int

	// maxSize is the total size of visible children.
@@ -59,6 +54,12 @@ type ListElement func(index int)

type iterationDir uint8

// First is the index of the first visible child.  Offset is the signed
// distance from the top edge to the child with the index First.
type Position struct {
	First, Offset int
}

const (
	iterateNone iterationDir = iota
	iterateForward
@@ -68,7 +69,7 @@ const (
const inf = 1e6

// init prepares the list for iterating through its children with next.
func (l *List) init(gtx *Context, len int) {
func (l *List) init(gtx *Context, len int, p Position) {
	if l.more() {
		panic("unfinished child")
	}
@@ -76,14 +77,12 @@ func (l *List) init(gtx *Context, len int) {
	l.maxSize = 0
	l.children = l.children[:0]
	l.len = len
	l.update()
	if l.scrollToEnd() {
		l.offset = 0
		l.first = len
	if p.First >= 0 && p.Offset >= 0 {
		l.pos = p
	}
	if l.first > len {
		l.offset = 0
		l.first = len
	l.update()
	if l.scrollToEnd() || l.pos.First > len {
		l.pos = Position{First: len}
	}
	l.macro.Record(gtx.Ops)
	l.next()
@@ -91,7 +90,14 @@ func (l *List) init(gtx *Context, len int) {

// Layout the List.
func (l *List) Layout(gtx *Context, len int, w ListElement) {
	for l.init(gtx, len); l.more(); l.next() {
	l.LayoutPos(gtx, len, l.pos, w)
}

// Layout the list, at position p.  Position{} is the top of the list.
// Negative p.First or p.Offset are ignored.  The list's position is returned,
// for use in the next call to Layout.
func (l *List) LayoutPos(gtx *Context, len int, p Position, w ListElement) Position {
	for l.init(gtx, len, p); l.more(); l.next() {
		cs := axisConstraints(l.Axis, Constraint{Max: inf}, axisCrossConstraint(l.Axis, l.ctx.Constraints))
		i := l.index()
		l.end(ctxLayout(gtx, cs, func() {
@@ -99,6 +105,7 @@ func (l *List) Layout(gtx *Context, len int, w ListElement) {
		}))
	}
	gtx.Dimensions = l.layout()
	return l.pos
}

func (l *List) scrollToEnd() bool {
@@ -113,7 +120,7 @@ func (l *List) Dragging() bool {
func (l *List) update() {
	d := l.scroll.Scroll(l.ctx.Config, l.ctx.Queue, l.ctx.Now(), gesture.Axis(l.Axis))
	l.scrollDelta = d
	l.offset += d
	l.pos.Offset += d
}

// next advances to the next child.
@@ -123,7 +130,7 @@ func (l *List) next() {
	// list end.
	if l.scrollToEnd() && !l.more() && l.scrollDelta < 0 {
		l.beforeEnd = true
		l.offset += l.scrollDelta
		l.pos.Offset += l.scrollDelta
		l.dir = l.nextDir()
	}
	if l.more() {
@@ -135,9 +142,9 @@ func (l *List) next() {
func (l *List) index() int {
	switch l.dir {
	case iterateBackward:
		return l.first - 1
		return l.pos.First - 1
	case iterateForward:
		return l.first + len(l.children)
		return l.pos.First + len(l.children)
	default:
		panic("Index called before Next")
	}
@@ -150,20 +157,20 @@ func (l *List) more() bool {

func (l *List) nextDir() iterationDir {
	vsize := axisMainConstraint(l.Axis, l.ctx.Constraints).Max
	last := l.first + len(l.children)
	last := l.pos.First + len(l.children)
	// Clamp offset.
	if l.maxSize-l.offset < vsize && last == l.len {
		l.offset = l.maxSize - vsize
	if l.maxSize-l.pos.Offset < vsize && last == l.len {
		l.pos.Offset = l.maxSize - vsize
	}
	if l.offset < 0 && l.first == 0 {
		l.offset = 0
	if l.pos.Offset < 0 && l.pos.First == 0 {
		l.pos.Offset = 0
	}
	switch {
	case len(l.children) == l.len:
		return iterateNone
	case l.maxSize-l.offset < vsize:
	case l.maxSize-l.pos.Offset < vsize:
		return iterateForward
	case l.offset < 0:
	case l.pos.Offset < 0:
		return iterateBackward
	}
	return iterateNone
@@ -180,8 +187,8 @@ func (l *List) end(dims Dimensions) {
		l.children = append(l.children, child)
	case iterateBackward:
		l.children = append([]scrollChild{child}, l.children...)
		l.first--
		l.offset += mainSize
		l.pos.First--
		l.pos.Offset += mainSize
	default:
		panic("call Next before End")
	}
@@ -199,14 +206,14 @@ func (l *List) layout() Dimensions {
	for len(children) > 0 {
		sz := children[0].size
		mainSize := axisMain(l.Axis, sz)
		if l.offset <= mainSize {
		if l.pos.Offset <= mainSize {
			break
		}
		l.first++
		l.offset -= mainSize
		l.pos.First++
		l.pos.Offset -= mainSize
		children = children[1:]
	}
	size := -l.offset
	size := -l.pos.Offset
	var maxCross int
	for i, child := range children {
		sz := child.size
@@ -220,7 +227,7 @@ func (l *List) layout() Dimensions {
		}
	}
	ops := l.ctx.Ops
	pos := -l.offset
	pos := -l.pos.Offset
	// ScrollToEnd lists lists are end aligned.
	if space := mainc.Max - size; l.ScrollToEnd && space > 0 {
		pos += space
@@ -255,8 +262,8 @@ func (l *List) layout() Dimensions {
		stack.Pop()
		pos += childSize
	}
	atStart := l.first == 0 && l.offset <= 0
	atEnd := l.first+len(children) == l.len && mainc.Max >= pos
	atStart := l.pos.First == 0 && l.pos.Offset <= 0
	atEnd := l.pos.First+len(children) == l.len && mainc.Max >= pos
	if atStart && l.scrollDelta < 0 || atEnd && l.scrollDelta > 0 {
		l.scroll.Stop()
	}
-- 
2.23.0
Details
Message ID
<BYOVRE6EWUMZ.AAA4TKQY323Y@toolbox>
In-Reply-To
<20191125033246.82018-1-larry@theclapp.org> (view parent)
DKIM signature
missing
Download raw message
Nice.

On Sun Nov 24, 2019 at 10:32 PM Larry Clapp wrote:
> - Added List.LayoutPos, which displays the list starting at the given
>   Position, and returns the list's Position after layout (including any
>   scrolling).
> - Make List.Layout call LayoutPos, for the common case where you don't
>   care about setting the list position.
> 
> Signed-off-by: Larry Clapp <larry@theclapp.org>
> ---
>  layout/list.go | 79 +++++++++++++++++++++++++++-----------------------
>  1 file changed, 43 insertions(+), 36 deletions(-)
> 
> diff --git a/layout/list.go b/layout/list.go
> index bd4d08a..119e308 100644
> --- a/layout/list.go
> +++ b/layout/list.go
> @@ -23,7 +23,7 @@ type scrollChild struct {
>  type List struct {
>  	Axis Axis
>  	// ScrollToEnd instructs the list to stay scrolled to the far end position
> -	// once reahed. A List with ScrollToEnd enabled also align its content to
> +	// once reached. A List with ScrollToEnd enabled also align its content to
>  	// the end.
>  	ScrollToEnd bool
>  	// Alignment is the cross axis alignment of list elements.
> @@ -39,12 +39,7 @@ type List struct {
>  	scroll      gesture.Scroll
>  	scrollDelta int
>  
> -	// first is the index of the first visible child.
> -	first int
> -	// offset is the signed distance from the top edge
> -	// to the child with index first.
> -	offset int
> -
> +	pos Position
>  	len int
>  
>  	// maxSize is the total size of visible children.
> @@ -59,6 +54,12 @@ type ListElement func(index int)
>  
>  type iterationDir uint8
>  
> +// First is the index of the first visible child.  Offset is the signed

Nit: use a single space after periods.

> +// distance from the top edge to the child with the index First.

The documentation should start with the type. How about:

// Position is a List scroll offset represented as an offset from the top edge
// of a child element.

> +type Position struct {
	// Index of the child element in the underlying element list
	// from the last Layout call.
	Index int
	// Offset is the signed scroll distance (in pixels).
> +	Offset int
> +}
> +
> @@ -91,7 +90,14 @@ func (l *List) init(gtx *Context, len int) {
>  
>  // Layout the List.
>  func (l *List) Layout(gtx *Context, len int, w ListElement) {

Comparing two Positions does not reveal the distance scrolled during Layout.
If you find the need for the distance, a future change can return it from Layout.

> -	for l.init(gtx, len); l.more(); l.next() {
> +	l.LayoutPos(gtx, len, l.pos, w)
> +}
> +
> +// Layout the list, at position p.  Position{} is the top of the list.
> +// Negative p.First or p.Offset are ignored.  The list's position is returned,
> +// for use in the next call to Layout.
> +func (l *List) LayoutPos(gtx *Context, len int, p Position, w ListElement) Position {

Most Lists don't have their position changed by the program. I suggest you drop
LayoutPos altogether and export List.Position. Document that Layout
"normalizes" or "resolves" the position.
Details
Message ID
<20191125152931.GA86078@larrymbp14.local>
In-Reply-To
<BYOVRE6EWUMZ.AAA4TKQY323Y@toolbox> (view parent)
DKIM signature
missing
Download raw message
On Mon, Nov 25, 2019 at 10:46:09AM +0100, Elias Naur wrote:
> Nice.

Thanks!  :)

> On Sun Nov 24, 2019 at 10:32 PM Larry Clapp wrote:
> > +// First is the index of the first visible child.  Offset is the signed
> 
> Nit: use a single space after periods.

Okay.

> > +// distance from the top edge to the child with the index First.
> 
> The documentation should start with the type. How about:
> 
> // Position is a List scroll offset represented as an offset from the top edge
> // of a child element.

Well, sort of.  If I read that I'd expect Position to be sort of
continuous (like an int or a float), but it's in units of "index +
pixel offset", where each "index" can be an arbitrary number of
pixels, based on that particular item's size.

So I can agree that the description needs work, but I don't think your
description is really accurate or helpful, either.

I don't have time just at the moment to think about it, but I will
later.  Or you can, of course.  :)

> > +type Position struct {
> 	// Index of the child element in the underlying element list
> 	// from the last Layout call.
> 	Index int
> 	// Offset is the signed scroll distance (in pixels).
> > +	Offset int
> > +}
> > +
> > @@ -91,7 +90,14 @@ func (l *List) init(gtx *Context, len int) {
> >  
> >  // Layout the List.
> >  func (l *List) Layout(gtx *Context, len int, w ListElement) {
> 
> Comparing two Positions does not reveal the distance scrolled during
> Layout.

I agree, but so what?  That wasn't the point of returning the scroll
position.

And, of course, it's important for people to keep in mind that List
doesn't really *scroll* itself during layout.  (It actually took a bit
of experimentation before *I* understood this!)  It updates its
position and calls for a redraw, and then (at the later Layout), it
redraws the list in a different position.

> If you find the need for the distance, a future change can return it
> from Layout.

I think if anything you'd want a method on Position to subtract one
from the other, like time.Sub().

It's still perilous because, as mentioned above, Position is not
really in continuous units.  I guess we could make it so that it is,
though, or have two different "subtract" methods, one that returned a
Position, and one that returned pixels.

> > -	for l.init(gtx, len); l.more(); l.next() {
> > +	l.LayoutPos(gtx, len, l.pos, w)
> > +}
> > +
> > +// Layout the list, at position p.  Position{} is the top of the list.
> > +// Negative p.First or p.Offset are ignored.  The list's position is returned,
> > +// for use in the next call to Layout.
> > +func (l *List) LayoutPos(gtx *Context, len int, p Position, w ListElement) Position {
> 
> Most Lists don't have their position changed by the program. I
> suggest you drop LayoutPos altogether and export List.Position.
> Document that Layout "normalizes" or "resolves" the position.

Ah.  Okay.

-- L
Details
Message ID
<BYP3YB64WDIZ.17R7B9QICESPC@toolbox>
In-Reply-To
<20191125152931.GA86078@larrymbp14.local> (view parent)
DKIM signature
missing
Download raw message
On Mon Nov 25, 2019 at 10:29 AM Larry Clapp wrote:
> > > +// distance from the top edge to the child with the index First.
> > 
> > The documentation should start with the type. How about:
> > 
> > // Position is a List scroll offset represented as an offset from the top edge
> > // of a child element.
> 
> Well, sort of.  If I read that I'd expect Position to be sort of
> continuous (like an int or a float), but it's in units of "index +
> pixel offset", where each "index" can be an arbitrary number of
> pixels, based on that particular item's size.
> 

That's exactly what Position is :) You're free to improve my description,
of course.

> So I can agree that the description needs work, but I don't think your
> description is really accurate or helpful, either.
> 
> I don't have time just at the moment to think about it, but I will
> later.  Or you can, of course.  :)
> 

I look forward to your description. Perhaps we're misunderstanding each
other.

> > > +type Position struct {
> > 	// Index of the child element in the underlying element list
> > 	// from the last Layout call.
> > 	Index int
> > 	// Offset is the signed scroll distance (in pixels).
> > > +	Offset int
> > > +}
> > > +
> > > @@ -91,7 +90,14 @@ func (l *List) init(gtx *Context, len int) {
> > >  
> > >  // Layout the List.
> > >  func (l *List) Layout(gtx *Context, len int, w ListElement) {
> > 
> > Comparing two Positions does not reveal the distance scrolled during
> > Layout.
> 
> I agree, but so what?  That wasn't the point of returning the scroll
> position.
> 

It was just an off-hand remark for future work. Sorry if it confused
matters.

> And, of course, it's important for people to keep in mind that List
> doesn't really *scroll* itself during layout.  (It actually took a bit
> of experimentation before *I* understood this!)  It updates its
> position and calls for a redraw, and then (at the later Layout), it
> redraws the list in a different position.
> 

Perhaps another misunderstanding, but List does recalculate and redraw
during Layout. If there is an active fling then sure, List asks for a
redraw to animate it.

> > If you find the need for the distance, a future change can return it
> > from Layout.
> 
> I think if anything you'd want a method on Position to subtract one
> from the other, like time.Sub().
> 
> It's still perilous because, as mentioned above, Position is not
> really in continuous units.  I guess we could make it so that it is,
> though, or have two different "subtract" methods, one that returned a
> Position, and one that returned pixels.
> 

I don't see how Position could be made comparable, do you? The reason
Position.Index exists is to make scroll offsets be relative. List only
knows the child element sizes it asks for in Layout, but even those
sizes are stale as soon as Layout returns.
Details
Message ID
<20191126164019.GA88089@larrymbp14.local>
In-Reply-To
<BYP3YB64WDIZ.17R7B9QICESPC@toolbox> (view parent)
DKIM signature
missing
Download raw message
On Mon, Nov 25, 2019 at 05:11:20PM +0100, Elias Naur wrote:
> That's exactly what Position is :) You're free to improve my
> description, of course.

I went with your description after all. :)

> > > Comparing two Positions does not reveal the distance scrolled
> > > during Layout.
> > 
> > I agree, but so what?  That wasn't the point of returning the
> > scroll position.
> > 
> 
> It was just an off-hand remark for future work. Sorry if it confused
> matters.

Ah.  No worries.

> > And, of course, it's important for people to keep in mind that
> > List doesn't really *scroll* itself during layout.  (It actually
> > took a bit of experimentation before *I* understood this!)  It
> > updates its position and calls for a redraw, and then (at the
> > later Layout), it redraws the list in a different position.
> > 
> 
> Perhaps another misunderstanding, but List does recalculate and
> redraw during Layout. If there is an active fling then sure, List
> asks for a redraw to animate it.

Ah.  I didn't realize that!

v2 submitted.

-- Larry
Review patch Export thread (mbox)