rivo / tview

Terminal UI library with rich, interactive widgets — written in Golang
MIT License
11.13k stars 575 forks source link

Using `app.Suspend()` make application unresponsive #932

Closed saeedafzal closed 9 months ago

saeedafzal commented 11 months ago

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, https://github.com/rivo/tview/issues/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):

https://github.com/rivo/tview/assets/37543494/606cbe3c-14ef-44a5-abfc-df4bff634af8

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 commented 11 months ago

@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...

https://github.com/rivo/tview/assets/1828125/be4fe7a1-997c-4542-a82c-df5ac2d1ac7c

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 commented 11 months ago

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 commented 10 months ago

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 commented 10 months ago

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

go 1.21.5, tcell 2.7.0, tview 5f078138442e.

digitallyserviced commented 10 months ago

@ilikeorangutans @rosmanov @saeedafzal

https://github.com/rivo/tview/commit/b2dec96e1a720d04e17745fbfb8d77214d8bee88

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.

https://github.com/rivo/tview/assets/1828125/cbe49540-950c-41c6-9a96-7096fa1bbdd1

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 commented 10 months ago

See https://github.com/gdamore/tcell/issues/677.

dundee commented 9 months ago

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