Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using app.Suspend() make application unresponsive #932

Closed
saeedafzal opened this issue Dec 31, 2023 · 7 comments · Fixed by #952
Closed

Using app.Suspend() make application unresponsive #932

saeedafzal opened this issue Dec 31, 2023 · 7 comments · Fixed by #952

Comments

@saeedafzal
Copy link

saeedafzal commented Dec 31, 2023

Hi, I've ran into an issue where after using app.Suspend(...) to start vim, the application stops responding to any keys and mouse. I can't CTRL+C to exit the app either.

From this issue, #244 from what I can tell, some people suggest adding app.Sync() after app.Suspend() which seems to have worked for some, but it didn't seem to work in my case.

I'm using Arch Linux and Kitty terminal emulator. Also happens with Alacritty. And I tested this on my M2 MacBook pro with Kitty and I get the same issue.

Video showing issue (recorded on the Mac):

Screen.Recording.2023-12-31.at.11.33.55.pm.mov

The code I ran in the video above (standalone):

package main

import (
	"os"
	"os/exec"

	"github.com/rivo/tview"
)

func main() {
	app := tview.
		NewApplication().
		EnableMouse(true)

	input := tview.NewInputField()

	// Starts vim using app.Suspend(...)
	startVim := func() {
		filename := "temp"
		file, err := os.Create(filename)
		if err != nil {
			panic(err)
		}
		defer file.Close()
		defer os.Remove(filename)

		app.Suspend(func() {
			cmd := exec.Command("vim", filename)
			cmd.Stdin = os.Stdin
			cmd.Stdout = os.Stdout
			cmd.Stderr = os.Stderr

			if err := cmd.Run(); err != nil {
				panic(err)
			}
		})
		app.Sync()

		b, err := os.ReadFile(filename)
		if err != nil {
			panic(err)
		}

		input.SetText(string(b))
	}

	form := tview.NewForm().
		AddFormItem(input).
		AddButton("Start Vim", startVim).
		AddButton("Button 2", nil)

	if err := app.SetRoot(form, true).Run(); err != nil {
		panic(err)
	}
}
@digitallyserviced
Copy link
Contributor

digitallyserviced commented Jan 1, 2024

@saeedafzal
Just a quick search on github code for the usage of app.suspend seems to have everyone prompting the user to press a key before returning full input capabilities.

https://github.com/sisyphsu/smart-server-selector/blob/66bf4fcb8346d629be1e6fd16f6036f19df8dbec/selector/selector.go#L85-L106

https://github.com/moson-mo/pacseek/blob/36a078a66254a884af95ba367134bd8a58cfba87/internal/pacseek/commands.go#L73-L92

However, your code should be fine the way it is. What you may be experiencing is the fact that the signal handlers have not caught back in on your app yet.

What keys have you tried? When I press tab after it resumes, your app works fine. I can then ctrl-c after I have tabbed once (ie the reason for asking user to press enter).

What may be catching you up is the fact that the form input handler doesnt respond to arrow keys the way you think if you have a button focused...

Peek.2023-12-31.21-35.mp4

oh wezterm btw... although it shouldnt make a difference... this is all backend tty stuff that if your term emulator properly supports shit, should all work exactly the same.

at least you're not on windows... then i'd say you're fucked

@saeedafzal
Copy link
Author

Hi @digitallyserviced, that's quite interesting, I've looked at those examples and I've actually found the issue thanks to it. It seems like the problem is the version of tcell that I was using.

I was running the example code I posted above with tcell v2.7.0, which is the latest version at this time, which is why I got the issue. Probably because I did go get -u github.com/rivo/tview which also updated tcell.

But if I use the version of tcell from tview's go.mod, which is 2.6.x, then everything works fine, including my example above. So the issue seems to be that I was using the latest version of tcell, when I should have been using the version that tview pulls in. Not sure what version of tcell you used to run the example, but I'm guessing it wasn't the latest one.

So for anyone else having this issue, don't upgrade tcell yourself. Only upgrade it with tview, at least that makes it work for me.

@rosmanov
Copy link

rosmanov commented Jan 3, 2024

I can confirm that the app.Suspend() method renders the application unresponsive with github.com/gdamore/tcell/v2 v2.7.0.

Here is how I see the problem with tcell. In a tview app, we often need to refer to the github.com/gdamore/tcell/v2 package (e.g., tcell.KeyF1, func(event *tcell.EventKey), etc.). To do that, we install the tcell package as a separate dependency. When a new version of tcell is released, a dependency monitoring system (such as Dependabot) generates a PR to update the dependency to the latest stable version. However, there is no indication of compatibility between the tcell and tview versions. So we just merge the PR and hope for the best.

I don't know how to solve this problem elegantly. The only idea that comes to mind is wrapping the entire tcell public API so that the user could only refer to tview when using the tview API. This way, the tcell dependency might become an implicit dependency pulled by tview.

Here is another code demonstrating the issue:

package main

import (
	"os"
	"os/exec"

	"github.com/rivo/tview"
	"github.com/gdamore/tcell/v2"
)

func main() {

	box := tview.NewBox().SetBorder(true).SetTitle("Hello, world!")
	app := tview.NewApplication()

	app.SetInputCapture(func(event *tcell.EventKey) *tcell.EventKey {
		switch event.Key() {
		case tcell.KeyF1, tcell.KeyCtrlH:
			app.Suspend(func() {
				cmd := exec.Command("vim", "/tmp/foo")
				cmd.Stdout = os.Stdout
				cmd.Stdin = os.Stdin
				_ = cmd.Run()
			})
		case tcell.KeyCtrlQ:
			app.Stop()
			return nil
		}
		return event

	})

	if err := app.SetRoot(box, true).Run(); err != nil {
		panic(err)
	}
}

go.mod

module example.com/m

go 1.21.5

require (
        github.com/gdamore/tcell/v2 v2.7.0 // indirect
        github.com/rivo/tview v0.0.0-20240101144852-b3bd1aa5e9f2
)

require (
        github.com/gdamore/encoding v1.0.0 // indirect
        github.com/lucasb-eyer/go-colorful v1.2.0 // indirect
        github.com/mattn/go-runewidth v0.0.15 // indirect
        github.com/rivo/uniseg v0.4.3 // indirect
        golang.org/x/sys v0.15.0 // indirect
        golang.org/x/term v0.15.0 // indirect
        golang.org/x/text v0.14.0 // indirect
)

Steps:

  1. go run main.go
  2. Press the F1 key
  3. Exit vim: :q
  4. Try to invoke vim again by pressing the F1 key or exiting the app by invoking the shortcut Ctrl+Q.

Actual result:

After closing vim, the app doesn't respond to key presses.

Expected result:

The app should resume after the vim process exits so that the user is able to either quit the app with Ctrl+Q or run another vim process by pressing F1.

@ilikeorangutans
Copy link

ilikeorangutans commented Jan 7, 2024

I have the same issue. Calling app.Suspend() freezes (deadlocks?) the app on return.

go 1.21.5, tcell 2.7.0, tview 5f07813.

@digitallyserviced
Copy link
Contributor

digitallyserviced commented Jan 7, 2024

@ilikeorangutans @rosmanov @saeedafzal

b2dec96

That commit seems to be what causes 2.7.0 tcell to not work properly. Essentially the main EventLoop in application.go expects to get a new screen via screen <- a.screenReplacement but this doesn't come it looks like.

Rolling back that commit we can see that tview rather than relying on screenReplacement just creates a tcell.NewScreen and moves on.

package main

import (
	"os"
	"os/exec"

	"github.com/gdamore/tcell/v2"
	"github.com/rivo/tview"
)

func main() {
	scr, _ := tcell.NewScreen()

	app := tview.
		NewApplication()
	// scr.Init()
	app.SetScreen(scr)

	app.
		EnableMouse(true)

	input := tview.NewInputField()

	// Starts vim using app.Suspend(...)
	startVim := func() {
		filename := "temp"
		file, err := os.Create(filename)
		if err != nil {
			panic(err)
		}
		defer file.Close()
		defer os.Remove(filename)

		app.Suspend(func() {
			cmd := exec.Command("nvim", filename)
			cmd.Stdin = os.Stdin
			cmd.Stdout = os.Stdout
			cmd.Stderr = os.Stderr

			if err := cmd.Run(); err != nil {
				panic(err)
			}
		})
                // create and set new screen
		nscr, _ := tcell.NewScreen()
		app.SetScreen(nscr)

		b, err := os.ReadFile(filename)
		if err != nil {
			panic(err)
		}

		input.SetText(string(b))
	}

	form := tview.NewForm().
		AddFormItem(input).
		AddButton("Start Vim", startVim).
		AddButton("Button 2", nil)

	if err := app.SetRoot(form, true).Run(); err != nil {
		panic(err)
	}
}

So rather than trying to fuck with rolling back commits, I tried to replicate this within the app's handler/suspend handlers.

Peek.2024-01-07.13-07.mp4

This seems to work properly, but you still have that "one key swallowed" before the input renegages.

While this is a workaround and doesn't fix the underlying tview. I will see what we can do PR-wise, but at least for the moment, the code I provided should get beyond that blocker.

@rivo
Copy link
Owner

rivo commented Jan 11, 2024

See gdamore/tcell#677.

@dundee
Copy link
Contributor

dundee commented Feb 18, 2024

Looks like this is now fixed and released in https://github.com/gdamore/tcell/releases/tag/v2.7.1

dundee added a commit to dundee/tview that referenced this issue Feb 23, 2024
@rivo rivo closed this as completed in #952 Feb 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants