Adding scrolling to TextGrid and a new Append method too #5434

andydotxyz · 2025-01-19T20:24:25Z

Super fast monospace text is now possible.

Congratulations to anyone using Fyne for their first app by logging text ;)

Checklist:

Tests included.
Lint and formatter run with no errors.
Tests all pass.
Public APIs match existing style and have Since: line.

package main

import (
	"strconv"
	"time"

	"fyne.io/fyne/v2"
	"fyne.io/fyne/v2/app"
	"fyne.io/fyne/v2/container"
	"fyne.io/fyne/v2/widget"
)

func main() {
	a := app.New()
	w := a.NewWindow("Hello")

	tg := widget.NewTextGrid()
	tg.ShowLineNumbers = true
	tg.Scroll = container.ScrollBoth
	w.SetContent(tg)
	w.Resize(fyne.NewSize(120, 100))

	go func() {
		i := 0
		for {
			time.Sleep(time.Millisecond * 25)
			fyne.Do(func() {
				tg.Append("Another line added, this time it's..." + strconv.Itoa(i))
				i++
			})
		}
	}()
	w.ShowAndRun()
}

It's still inefficient, but that comes next :)

This enables the row pool for fast scroll which comes next

…ow possible!

coveralls · 2025-01-19T21:00:52Z

coverage: 59.122% (+0.08%) from 59.043%
when pulling 61ca852 on andydotxyz:feature/scrolltextgrid
into 351aebe on fyne-io:develop.

Jacalz

Thanks. This is a really nice change. I left a few suggestions and a comment about a potential bug I found. Would it not be nice to also update fyne_demo so this can be showcased in some way?

Jacalz · 2025-01-22T19:20:01Z

widget/textgrid.go

+		t.scroll.Hide()
+		t.scroll.Content = nil
+		content.Resize(t.text.Size())
+		t.SetObjects([]fyne.CanvasObject{t.text})


I think it would be faster and potentially even cleaner, to do just not use a base renderer (or expose its internal object slice) to just set t.objects[0] = t.text.

I've honestly thought about removing that helper sometimes. It rarely adds much in my opinion and many renderers don't even use it.

Jacalz · 2025-01-22T19:20:12Z

widget/textgrid.go

+
+		t.scroll.Resize(t.text.Size())
+		content.Resize(content.MinSize())
+		t.SetObjects([]fyne.CanvasObject{t.scroll})


See comment above.

Jacalz · 2025-01-22T19:22:26Z

widget/textgrid.go

+	var toRemove []*textGridRow
+	for _, row := range t.visible {
+		if row.(*textGridRow).row < start || row.(*textGridRow).row > end {
+			toRemove = append(toRemove, row.(*textGridRow))
+			break


It looks like toRemove is only ever going to be of length one? Seems strange.

Jacalz · 2025-01-22T19:25:27Z

widget/textgrid.go

 	x := 0
+	if t.obj.row >= len(t.obj.text.text.Rows) {
+		return // empty?


The question mark in the comment makes me worried. Is there something strange going on here? Are unsure about that the effect of returning here does?

widget/textgrid.go

Jacalz · 2025-01-22T19:29:31Z

widget/textgrid.go

+	if t.scroll.Content != nil {
+		t.scroll.Resize(s)
+	}
+	t.text.Resize(s)


With the suggestion about accessing the objects slice below, this could be:

Suggested change

if t.scroll.Content != nil {

t.scroll.Resize(s)

}

t.text.Resize(s)

t.objects[0].Resize(s)

Jacalz · 2025-01-22T19:33:26Z

widget/textgrid.go

+	var scroll *widget.Scroll
+	content := newTextGridContent(t)
+	objs := make([]fyne.CanvasObject, 1)
+	if t.Scroll == widget.ScrollNone {
+		scroll = widget.NewScroll(nil)
+		objs[0] = content
+	} else {
+		scroll = widget.NewScroll(content)
+		scroll.Direction = t.Scroll
+		objs[0] = scroll
+	}
+	t.scroll = scroll
+	r := &textGridRenderer{text: content, scroll: scroll}
+	r.SetObjects(objs)


This can be cleaned up a bit:

Suggested change

var scroll *widget.Scroll

content := newTextGridContent(t)

objs := make([]fyne.CanvasObject, 1)

if t.Scroll == widget.ScrollNone {

scroll = widget.NewScroll(nil)

objs[0] = content

} else {

scroll = widget.NewScroll(content)

scroll.Direction = t.Scroll

objs[0] = scroll

}

t.scroll = scroll

r := &textGridRenderer{text: content, scroll: scroll}

r.SetObjects(objs)

var inScroller fyne.CanvasObject

content := newTextGridContent(t)

objs := []fyne.CanvasObject{content}

if t.Scroll != widget.ScrollNone {

inScroller = content

scroll.Direction = t.Scroll

objs[0] = scroll

}

t.scroll = widget.NewScroll(inScroller)

r := &textGridRenderer{text: content, scroll: t.scroll}

r.SetObjects(objs)

This feels subjective - can it be definitive that creating "inScroller" to avoid an else is better?

Hmm, yeah. Might be subjective. I just try to avoid branches as much as possible because it is less code indented to the right. Do as you wish :)

I see that but it is not removing an indent (as that level deep was already there).
I was a little confused because the nil content of Scroller becomes implicit which could look like a bug.

andydotxyz · 2025-01-23T22:23:57Z

Would it not be nice to also update fyne_demo so this can be showcased in some way?

I didn't bother with the fyne_demo because, as you saw in another PR, I'm hoping to replace the tutorials with our new code style assisting demo.

andydotxyz · 2025-01-23T22:41:34Z

I've addressed what I think are the objective fixes. The other items are commenting on pre-existing algorithms or are suggestions about how the code could be - but I think we should stick to what needs to be better for a clear reason TBH.

Jacalz · 2025-01-23T23:20:14Z

Sorry if it wasn't clear but I basically tried to give suggestions and so on as well. I think your points are fair :)

andydotxyz added 7 commits January 18, 2025 18:46

Add scrolling to TextGrid at an API level.

e6b6241

It's still inefficient, but that comes next :)

Migrate the text grid to a row based layout

48e6c7c

This enables the row pool for fast scroll which comes next

Cache and re-use lines in the TextGrid widget for speed when scrolling

00d2e9a

And add an Append function that can be used to show off the speed n…

f75367f

…ow possible!

Remove unused and unnecessary canvas reference caching

e388189

Remove unneeded position variable

b7550ce

Merge branch 'develop' into feature/scrolltextgrid

f7c8c86

Merge branch 'develop' into feature/scrolltextgrid

c2b348f

Jacalz requested changes Jan 22, 2025

View reviewed changes

Corrections and clarifications

e063118

Updating missed test file change

61ca852

Jacalz approved these changes Jan 23, 2025

View reviewed changes

andydotxyz merged commit f8dfaaa into fyne-io:develop Jan 24, 2025
12 checks passed

andydotxyz deleted the feature/scrolltextgrid branch January 24, 2025 09:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding scrolling to TextGrid and a new Append method too #5434

Adding scrolling to TextGrid and a new Append method too #5434

andydotxyz commented Jan 19, 2025

coveralls commented Jan 19, 2025 •

edited

Loading

Jacalz left a comment

Jacalz Jan 22, 2025

Jacalz Jan 22, 2025

Jacalz Jan 22, 2025

Jacalz Jan 22, 2025

andydotxyz Jan 23, 2025

Jacalz Jan 22, 2025

Jacalz Jan 22, 2025

Jacalz Jan 22, 2025

andydotxyz Jan 23, 2025

Jacalz Jan 23, 2025

andydotxyz Jan 23, 2025

andydotxyz commented Jan 23, 2025

andydotxyz commented Jan 23, 2025

Jacalz commented Jan 23, 2025

Adding scrolling to TextGrid and a new Append method too #5434

Adding scrolling to TextGrid and a new Append method too #5434

Conversation

andydotxyz commented Jan 19, 2025

Checklist:

coveralls commented Jan 19, 2025 • edited Loading

Jacalz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andydotxyz commented Jan 23, 2025

andydotxyz commented Jan 23, 2025

Jacalz commented Jan 23, 2025

coveralls commented Jan 19, 2025 •

edited

Loading