add missing fyne.Do https://github.com/fyne-io/terminal/issues/120#is…

[nagylzs]

…suecomment-3020131266

[andydotxyz]

I think this hints at a race condition if the rows are updating on one thread then refreshing on another.
Perhaps the whole method needs to be in a Do?

[nagylzs]

Yes, scrollUp updates t.content.Rows and t.content is a widget. Probably the whole scrollUp and scrollDown method calls must be inside fyne.Do. They are called from parseEscState and handleOutputLineFeed.

• terminal/output.go

  Line 200 in e2adc1e

  t.scrollDown()

• terminal/output.go

  Line 325 in e2adc1e

  t.scrollDown()

I tracked them down and they are always called from handleOutput one way or another.

I'm going to go over all method calls that are called from handleOutput and wrap them into fyne.Do if they can alter the state of a widget.

[nagylzs]

I was not sure if parseAPC call needs to be wrapped into fyne.Do or not.

terminal/output.go

Line 142 in e2adc1e

t.parseAPC(r)

It is because it can use any function that was registered by RegisterAPCHandler. I think we should make it the responsibility of the handler registrar to wrap inner calls into fyne.Do if it needs to.

Also, when I tested my modifications then it turned out that wrapping these calls into fyne.Do is not enough, fyne.DoAndWait is needed. It is because in a single handleOutput call, multiple UI state changing (widget) methods are called witin a very short amount of time, and unfortunately, they have to run sequentially, in a specific order.

[figuerom16]

I closed my PR since I was going the wrong way. If we are trying to call fyne.Do from higher up the stack wouldn't we target t.updatePTYSize and wrap that in a fyne.Do since that's going to cascade down?

terminal/term.go

Line 253 in e2adc1e

t.updatePTYSize()

[nagylzs]

I closed my PR since I was going the wrong way. If we are trying to call fyne.Do from higher up the stack wouldn't we target t.updatePTYSize and wrap that in a fyne.Do since that's going to cascade down?

terminal/term.go

Line 253 in e2adc1e

t.updatePTYSize()

I don't have an answer for that, possibly Andy can answer. This is what I see:

• Terminal.updatePTYSize is called from Terminal.Resize and Terminal.open
• Terminal.Resize is a CanvasObject method; in theory it must be called from the main fyne thread
• However, Terminal.open is called from Terminal.RunLocalShell which is (I think) always called from a goroutine

So it is hard to tell if Terminal.updatePTYSize will be called from the main fyne thread or not, but probably both is true.

I ran into the same problem several times, and started a discussion about it here fyne-io/fyne#5646

But honestly, I don't know the correct solution in this particular case. :-(

[figuerom16]

For fun I've been playing around with func (t *Terminal) Resize(s fyne.Size) and so far this gives me amazing results for rendering speed and consistency.

func (t *Terminal) Resize(s fyne.Size) {
	cellSize := t.guessCellSize()
	cols := uint(math.Floor(float64(s.Width) / float64(cellSize.Width)))
	rows := uint(math.Floor(float64(s.Height) / float64(cellSize.Height)))
	if (t.config.Columns == cols) && (t.config.Rows == rows) {
		return
	}
	oldRows := int(t.config.Rows)
	t.config.Columns, t.config.Rows = cols, rows
	if t.scrollBottom == 0 || t.scrollBottom == oldRows-1 {
		t.scrollBottom = int(t.config.Rows) - 1
	}
	fyne.Do(func() {
		t.BaseWidget.Resize(s)
		if t.content != nil {
			t.content.Resize(fyne.NewSize(float32(cols)*cellSize.Width, float32(rows)*cellSize.Height))
		}
		t.onConfigure()
		t.updatePTYSize()
	})
}

[nagylzs]

@figuerom16 Did you try to run a program in the terminal that scrolls the screen continuously? Did you test it on Windows or Linux or something else?

[nagylzs]

@figuerom16 Terminal.Resize writes into t.config.Columns and t.config.Rows, and Terminal.updatePtySize reads them. If you call Resize many times quickly, and put Terminal.updatePtySize into fyne.Do, then updatePtySize may be reading these values on the main fyne thread, while Resize is writing them on another thread.

@andydotxyz wrote here that calling Refresh from anywhere but the main fyne thread is an error. I believe it also stands for any CanvasObject method, including Resize.

[figuerom16]

@nagylzs Linux and I used the yes program which just prints y continuously and resized the window which seems to work. cat /dev/random causes it to freeze before or after any code modification. I also did a sudo pacman -Syu and resized the terminal while it ran and it seemed to work like before. Tested systemctl status and resized the terminal and that worked really well.

As for this being a mistake it could be. I'm just reporting what seems to work, but this probably does break something that I'm not thinking of. Give it a try?

[andydotxyz]

unfortunately, they have to run sequentially, in a specific order.

fyne.Do calls will happen in the requested order. It sounds like state outside the function is changing influencing how it functions which would be a bug.

Also note that DoAndWait will be substantially slower than just Do because you have to wait for the toolkit to catch up. The intent is simply that all data is updated then Do is called to perform the graphical changes.

[andydotxyz]

For fun I've been playing around with func (t *Terminal) Resize(s fyne.Size) and so far this gives me amazing results for rendering speed and consistency.

func (t *Terminal) Resize(s fyne.Size) {
	cellSize := t.guessCellSize()
	cols := uint(math.Floor(float64(s.Width) / float64(cellSize.Width)))
	rows := uint(math.Floor(float64(s.Height) / float64(cellSize.Height)))
	if (t.config.Columns == cols) && (t.config.Rows == rows) {
		return
	}
	oldRows := int(t.config.Rows)
	t.config.Columns, t.config.Rows = cols, rows
	if t.scrollBottom == 0 || t.scrollBottom == oldRows-1 {
		t.scrollBottom = int(t.config.Rows) - 1
	}
	fyne.Do(func() {
		t.BaseWidget.Resize(s)
		if t.content != nil {
			t.content.Resize(fyne.NewSize(float32(cols)*cellSize.Width, float32(rows)*cellSize.Height))
		}
		t.onConfigure()
		t.updatePTYSize()
	})
}

If this is giving you great results it means that somewhere the Resize is being called from a goroutine which is an error.
There may be multiple factors at play here.

Resize should be a very lightweight operation - it looks like it is doing more than it should either in the Fyne TextGrid code or inside the updatePTYSize function...

[andydotxyz]

GitHub pinged me on this but I don't see what changed.
Is it ready again, or is is till pending reviewing some comments above?

[mgabeler-lee-6rs]

Found this PR due to having abysmal performance issues with current master -- far worse than shown in the youtube video, I can see the terminal drawing individual characters in my application. And this means that when it scrolls I can watch a multi-second pass as it refreshes each cell left to right top to bottom just to scroll one line.

With this PR, while things aren't ideal -- fast output still falls behind -- they are at least usable.

My use case involves tailing some logs, which sometimes come out at a few dozen lines per second, and this PR still falls behind on that, though it eventually catches up when the log output slows down.

[nagylzs]

FYI, I gave up on using fyne terminal. One problem was that it was slow. Its speed has improved a lot, but it is still very slow compared to other (native) terminals like gnome-terminal or rxvt. The next biggest problem I had was the lack of support for scrolling. There seems to be no easy way for doing it, so I abandoned fyne and tried gtk 4.0 (gotk4).

There are lots of problems with gotk4 as well. It lacks documentation, and it had no support for terminals at all. However, it was quite straightforward to write a go wrapper around libvte, and now it is very stable and very fast. It also supports gestures and mouse interactions with the terminal etc. It is also true that it is much harder to install and use... I lost the ability to build single-file executables with no external dependencies.

Overall, the most important thing for me was to have a terminal that is fast and support scrolling, clipboard and mouse. Unfortunately, I see no chance of doing this with fyne terminal. :-(

[nagylzs]

I don't think that this PR fixes everything that needs to be fixed. The hardest thing that I could not come over is that somehow I have to know which methods should be called from the main fyne thread, and which shouldn't; and many times it is not possible to write a method that can be called from both. This makes it especially hard to write error free code, because some methods can be called from many places, and they can also be called from many other places etc. If I have to go back more than two levels in the stack trace. then it often becomes impossible to find out which threads/goroutines could be calling my method, and without this knowledge, it is not possible to tell if fyne.Do needs to be applied or not. But this would be essential: if fyne.Do is called when not needed, then it is an error (and possibly a deadlock), if it is not called when it is needed, then it may be a concurrent data access, which leads to a panic.

I understand that the new threading model was introduced to make fyne more efficient, but it makes reasoning about the quite hard.

In this concrete case (issue #120) I tried to trace back the calls and found out that there are fyne.io internals calling my code in various ways, and I just don't know enough about fyne internals to have all the answers and solve this problem. To solve the problem, I would have to know about which thread/goroutine is used to run which fyne internal code, but this is not documented (of course, because they are internals). I think this can only be solved by a core fyne developer. I also tried a workaround: calling go fyne.Do(func() { doSomethingHere }) but it also resulted in unexpected errors, possibly because the order of execution is arbitrary/random.

I still think that my PR makes fyne-terminal better though, because these changes will move some UI updates out of the goroutine that runs the local shell, to the main fyne thread. It can't hurt.

[andydotxyz]

Found this PR due to having abysmal performance issues with current master -- far worse than shown in the youtube video, I can see the terminal drawing individual characters in my application. And this means that when it scrolls I can watch a multi-second pass as it refreshes each cell left to right top to bottom just to scroll one line.

With this PR, while things aren't ideal -- fast output still falls behind -- they are at least usable.

My use case involves tailing some logs, which sometimes come out at a few dozen lines per second, and this PR still falls behind on that, though it eventually catches up when the log output slows down.

I'm sorry to hear that. The videos are all genuine and I find that performance is comparable with the builtin terminal for macOS these days! (The Linux ones others mentioned are certainly better optimised and we will strive to match them)
To see such bad performance it sounds like there is some substantial logging or something else slowing things down.
Please do open a new issue about the performance so we can look into it to see if somehow your setup is causing a slower execution.

[andydotxyz]

The next biggest problem I had was the lack of support for scrolling. There seems to be no easy way for doing it

That significantly changed now that scrolling has been added to the fyne TextGrid - we still have to handle the maths of where lines move when new content arrives, but at least the widget has all the graphical capabilities. This was possible since the recent refactoring that returned the terminal to using the base widget renderer.

When I next get time to do feature additions on this repo it would be tabs and scrolling :).

most important [...] clipboard and mouse. Unfortunately, I see no chance of doing this with fyne terminal. :-(

Erm - clipboard and mouse selection are already in Fyne Term...

[andydotxyz]

and many times it is not possible to write a method that can be called from both

Overall the code here does need more refactoring to tidy these things up - it should be clear what needs to be in fyne.Do but this project was built many years before that requirement was even thought of, so it does have some awkward code paths.

In this concrete case (issue #120) I tried to trace back the calls and found out that there are fyne.io internals calling my code in various ways

Apologies - every Fyne callback should be executing your code on the fyne thread. That is the new design but maybe we messed up somewhere...

I still think that my PR makes fyne-terminal better though, because these changes will move some UI updates out of the goroutine that runs the local shell, to the main fyne thread. It can't hurt.

I agree, however the comments from July 4th were not responded to so I wasn't sure how to progress with this. fyne.Do and fyne.DoAndWait but execute in the correct order - the difference is that the wait version will not return until it has completed operation - hence my comment about being a lot slower. It should only be used where it is critical for app state that all the widgets are fully updated before the code continues. Something to avoid when we are looking primarily for speed.

[andydotxyz]

p.s. with the scrolling enabled resize should be much faster - the current slowness is because it must wait for the terminal widget to completely redraw & layout before it can return CPU to the mouse handling. Inside a scroller that can be asynchronous.

[nagylzs]

Erm - clipboard and mouse selection are already in Fyne Term...

Yes, my mistake. Selection can be changed with the mouse. What I meant is that mouse interactions did not work for me. For example, if I start midnight commander in fyne terminal, and click on a file with the mouse, then nothing happens.

I agree, however the comments from July 4th were not responded to so I wasn't sure how to progress with this.

I'm sorry again, here is the answer: any function can be added via RegisterAPCHandler. It is not possible to tell in advance, if the registered function needs to be called from the main fine thread, or outside of it. The workaround that would allow you to safely call any handler is by calling go fyne.Do ( handler ), but that can change the order of execution for sure. I already tried this workaround and it resulted in various errors at unexpected places. If you do not do this workaround, then you cannot tell if calling the handler is an error or not. Simply because you cannot tell what the handler does.

Unfortunately, this does not help you to to solve the problem. :-(

[andydotxyz]

For example, if I start midnight commander in fyne terminal, and click on a file with the mouse, then nothing happens.

Aha! Yes - please open an issue for this feature request and we can get it included :)

[andydotxyz]

I'm sorry again, here is the answer: any function can be added via RegisterAPCHandler.

Ah, I understand. So it's not a core Fyne callback but one for the terminal.

I guess we need to make a judgement call then. To match Fyne design we could ensure it is always called back on the Fyne thread and then if users want to do complicated things they can spin up a goroutine.

It seems like taking that approach would remove any ambiguity, would it fix issues you were seeing?

[nagylzs]

It seems like taking that approach would remove any ambiguity, would it fix issues you were seeing?

Yes. The only other place that was scrollUp, called from parseEscState, but this PR now wraps it into a fyne.DoAndWait.

Just one note: my PR uses fyne.DoAndWait instead of fyne.Do. If I replace DoAndWait with Do, then the terminal comes up, displays garbage and does not work properly. I suspect that it is because handleOutput is called from a goroutine in a loop (term.go method run()), and possibly the output reaches the terminal emulator out-of-order. This is how it looks on my computer:

the test program is this:

package main

import (
	"fyne.io/fyne/v2/app"
	"github.com/fyne-io/terminal"
)

func main() {
	a := app.New()
	w := a.NewWindow("Test")
	t := terminal.New()
	go func() {
		_ = t.RunLocalShell()
	}()
	w.SetContent(t)
	w.ShowAndRun()
}

But if I use DoAndWait then it works (but it is quite slow again) .

[mgabeler-lee-6rs]

To see such bad performance it sounds like there is some substantial logging or something else slowing things down.

Well, my use case is essentially dumping live logs from a docker run (except my app uses the docker client API instead of shelling out to the CLI), but I suspect you meant logging from fyne, of which there was only one instance of a "call from wrong thread should be using fyne.Do" warning. With this PR, that is gone.

The logs my use case emits are ... verbose at times. I'd said before it was a couple dozen lines per second, but I looked closer and it's a few hundred per second at (thankfully rare) times. With this PR it can refresh the screen scrolling about 5 lines per second, so it takes several seconds for it to catch up from those bursts, but since the output for me is read-only the result is ... usable.

I'll try to make a non-proprietary reproducer that I can share, but I'm getting laid off from this job in a couple weeks so this may end up being someone else's problem :(

[mgabeler-lee-6rs]

Please do open a new issue about the performance so we can look into it to see if somehow your setup is causing a slower execution.

Opened #124 with videos and a cut-down reproducer pushed as a public repo.

[andydotxyz]

Just one note: my PR uses fyne.DoAndWait instead of fyne.Do. If I replace DoAndWait with Do, then the terminal comes up, displays garbage and does not work properly. I suspect that it is because handleOutput is called from a goroutine in a loop

The internals to Fyne will be in order (guaranteed) but perhaps we are messing up something internally and the ..AndWait is helping to mask the true source of the race? (Maybe a race checker can track it down...)

[andydotxyz]

Well, my use case is essentially dumping live logs from a docker run (except my app uses the docker client API instead of shelling out to the CLI), but I suspect you meant logging from fyne

Yes that is what I had meant. Log calls can slow things down a lot, particularly if it is logging an error in the rendering for some reason ;)

I'll try to make a non-proprietary reproducer that I can shar

Thanks for this. I suspect we may be hitting some sort of backlog of rendering that could result in exponential slow-down but the new issue will help a lot.

[mgabeler-lee-6rs]

I suspect we may be hitting some sort of backlog of rendering

An observation from testing on #124 that supports this being part of the problem: When the system is thrashing printing out the logs, ctrl-c does nothing. That led me to believe that it was rendering buffered up in the terminal widget. I further tested this theory by killing the processes / container generating the output, and fyneterm kept going for a good while after. However it also clearly was not all rendering buffering, because when the terminal caught up after a bit, the output generator had clearly not produced all its output, so there clearly also had been some stdio backpressure on the process generating the output.

[andydotxyz]

Right I tracked down that the reason for "AndWait" being needed is that the parse/print and also state management code is all mixed up.

I have some fixes locally but really a large refactor is needed to clean this up properly. Speed will be incredible when that is done ;).

For now I have added some fyne.Do safety around the scroll and other problem areas - I'm now seeing good performance and no logs/crashes.
Happy to get feedback on that.

I don't think that this PR should land tbh as it's just hiding the real race problems and cannot be performant because essentially we are waiting for execution to change thread potentially many times each character change.

[andydotxyz]

Given my summary above, and that @nagylzs reports not actively using the library I expect that a replacement PR should be created with the refactor approach I suggest a little down the road and so I will close this one.