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

Starting multiple screen sessions (lost key event) #194

Closed
rivo opened this issue Mar 11, 2018 · 30 comments · Fixed by #431
Closed

Starting multiple screen sessions (lost key event) #194

rivo opened this issue Mar 11, 2018 · 30 comments · Fixed by #431

Comments

@rivo
Copy link
Contributor

rivo commented Mar 11, 2018

I need to temporarily exit a Screen session and then come back to it. From I can see, I can't reuse a Screen object on which Fini() has been called already. So I'm creating a new one. However, it seems that by doing that, one key event gets lost. The example below illustrates this. The first event after Init() is always a resize event. Then I hit a key so the second event is a key event. The main() function should thus exit after hitting a key two times. However, when you run it, it takes three key strokes to make it exit. (It's actually the second key stroke that is lost.)

Any idea why this happens? Also, in case starting multiple screen sessions is not a good idea, what would be a good way to go back to the previous terminal session temporarily?

Here's the code:

package main

import (
	"time"

	"github.com/gdamore/tcell"
)

// Program should exit after two key events. However, it takes three key strokes to
// stop it.
func main() {
	// First screen.
	screen, err := tcell.NewScreen()
	if err != nil {
		panic(err)
	}
	err = screen.Init()
	if err != nil {
		panic(err)
	}

	// Wait for input.
	screen.PollEvent() // Resize
	screen.PollEvent() // Key

	// Finish this screen.
	screen.Fini()

	// Go back for one second.
	time.Sleep(time.Second)

	// Second screen.
	screen, err = tcell.NewScreen()
	if err != nil {
		panic(err)
	}
	err = screen.Init()
	if err != nil {
		panic(err)
	}

	// Wait for input.
	screen.PollEvent() // Resize
	screen.PollEvent() // Key

	// Finish this screen.
	screen.Fini()
}
@gdamore
Copy link
Owner

gdamore commented Mar 14, 2018

This generally isn't supported right now. The problem is that not all platforms can do this equally well. I'm not sure at present why this particular error is occurring, but I suspect having multiple screens running is to blame.

A better solution would be for us to figure out how to "suspend" the screen, and then "resume" it later. Suspending should restore the old terminal, and stop eating keystrokes, whereas resumption would reenable all that.

I will think on this some more.

@rivo
Copy link
Contributor Author

rivo commented Mar 14, 2018

A better solution would be for us to figure out how to "suspend" the screen, and then "resume" it later. Suspending should restore the old terminal, and stop eating keystrokes, whereas resumption would reenable all that.

This is exactly what I need. I solved it by finalizing, then recreating the screen, which led to this key event issue. But if there's an "official" way to suspend the screen, that would be great.

@JesseAldridge
Copy link

I will think on this some more.

Was there any outcome from this thinking?
Because this bug is annoying: rivo/tview#165

@gdamore
Copy link
Owner

gdamore commented Sep 11, 2018

I have not made any progress on this, no. Frankly it has not been a priority for me, as it doesn't bother me and the level support that tcell gets at the moment is pretty much as I find cycles and interest. The project isn't earning any income for me, so paying customers (mostly on other projects) tend to get higher priority.

Working through fixing this is likely to take a couple of hours at least. Probably what needs to happen is that we need to catch the SIGTSTP and SIGCONT signals, where the handler for SIGTSTP has to stop the key poller, and the SIGCONT needs to resume it. There are probably some interesting interactions with termio buffering and settings too... for example most folks would want sane cooked tty settings applied when SIGTSTP is caught, but we need to put the term back into raw mode when come back.

There's probably enough hint there that someone else could bake up a solution as well.

If you're interested in sponsoring this work, please let me know and we can talk out of band.

@gdamore
Copy link
Owner

gdamore commented Sep 12, 2018

I was looking at #199 and that code around signal handling was all wrong. I'll see if I can work something up that does the right thing here. Stay tuned.

@gdamore
Copy link
Owner

gdamore commented Sep 12, 2018

I did a bit of poking at this tonight, and I've come to the conclusion that there are several compounding problems here. The separate file descriptors for read and write, the way we use termios, and golang channels are playing a role here.

I believe that to fix this properly, I am going to want to restructure the core input (and possibly the output) loop -- there will probably be a couple of go routines, and separate dup()'d file descriptors between init and fini calls. I have some ideas here.

I might be able to get this done on a Saturday soon -- but no promises.

@tobimensch
Copy link
Contributor

tobimensch commented Nov 13, 2018

@gdamore
The browsh project would also like suspend/resume browsh-org/browsh#239. Did you make any progress? Maybe you can setup bounties for tasks like that. For example I might be willing to chip in a few bucks but not the whole amount you'd like to have. If the other people in this thread also chip in, maybe it's enough. You could use bountysource (which has integration with github) or something like that.

@ddevault
Copy link

Several Saturdays later... any news? 😄

@francop-eb
Copy link

francop-eb commented Mar 30, 2019

Workaround this bug:

// This method avoid tcell bug https://github.com/gdamore/tcell/issues/194
// Aditional EOL event is sent to ensure, consequent events, are correctly handled
func sendExtraEventFix() {
	kb, err := keybd_event.NewKeyBonding()
	if err != nil {
		panic(err)
	}
	kb.SetKeys(keybd_event.VK_ENTER)
	err = kb.Launching()
	if err != nil {
		panic(err)
	}
}

@gdamore
Copy link
Owner

gdamore commented Apr 8, 2019

I've not had any time to work on this lately; sorry.

@taybart
Copy link

taybart commented Oct 25, 2019

@gdamore Do you have a good place to start to fix this? I can PR I just need a general area to look in

@gdamore
Copy link
Owner

gdamore commented Oct 27, 2019

Unfortunately so much time has passed since I last looked at this, that I'll need to re-research it. Its unfortunate, but tcell does get a very limited amount of my time since none of my commercial customers use it (or at least they haven't sponsored any work).

This might sound a bit mercenary, but now that I'm a github sponsor, I'm probably more likely to spend time on this stuff if folks who use it or use projects that use it sponsored me. Otherwise this is relegated to hobby level stuff.

@taybart
Copy link

taybart commented Oct 28, 2019

Ok, I understand. I will look around on my own

@tslocum
Copy link
Contributor

tslocum commented Dec 15, 2020

This seems resolved. I'm unable to reproduce the issue with the above code.

@taybart
Copy link

taybart commented Dec 15, 2020

Just tested with 1.4.0. Seems to be fixed for me as well!

@gdamore
Copy link
Owner

gdamore commented Dec 15, 2020

Ok... not sure what fixed it... but I'm going to close this then.

@gdamore gdamore closed this as completed Dec 15, 2020
@bensadeh
Copy link

It would be great to know a bit more about how this was fixed.

Is the fix for this issue to use @francop-eb's workaround?

As I understand it, the using the keybd_event framework involves running the application with root privileges. For my use case (and perhaps others as well), I would like to be able to suspend and resume without requiring root privileges.

@gdamore
Copy link
Owner

gdamore commented Dec 16, 2020

Wait... maybe I closed prematurely? I thought people were saying that 1.4.0 fixed the issue... if that's not the case, please reopen.

@bensadeh
Copy link

No problem, gdamore, thanks for replying so quickly.

Unfortunately, I this issue is still both present and reproducible for me.

I am using tcell (v2.1.0) indirectly through a suspend function in tslocum’s cview (v1.5.2), and the first keystroke there is still lost. Unless I am overlooking something, maybe this should be reopened?

@tslocum
Copy link
Contributor

tslocum commented Dec 16, 2020

I tested using Gentoo Linux with Alactritty and Kitty, with and without tmux and screen, both tcell v1.4.0 and v2.1.0. Please include your system configuration with your report (as I should have).

Here's a cview test program as well. Press a key to suspend, then enter a line of input, then press another key to exit.

package main

import (
	"bufio"
	"fmt"
	"log"
	"os"

	"github.com/gdamore/tcell/v2"
	"gitlab.com/tslocum/cview"
)

// Program should suspend after first key event, then exit after the second.
func main() {
	app := cview.NewApplication()

	keyPress := make(chan struct{})
	app.SetInputCapture(func(event *tcell.EventKey) *tcell.EventKey {
		keyPress <- struct{}{}
		return nil
	})

	tv := cview.NewTextView()
	tv.SetText("Hello world!")
	app.SetRoot(tv, true)

	go func() {
		<-keyPress
		app.Suspend(func() {
			reader := bufio.NewReader(os.Stdin)
			fmt.Print("Enter text: ")
			reader.ReadString('\n')
		})
		<-keyPress
		app.Stop()
	}()

	if err := app.Run(); err != nil {
		log.Fatal(err)
	}
}

@bensadeh
Copy link

bensadeh commented Dec 16, 2020

Thanks for the code snippet, tslocum.

This might be a macOS specific bug. My setup is macOS Catalina 10.15.7, tested with Terminal, Alacritty and the latest iTerm2.

I am still running into the issue, here is a description of what's happening when running your code above:

  1. When I run the program I see Hello World (So far no key input)
  2. When I press any key after Hello World, I get the "Enter Text: " prompt where I can write text
  3. I write text and hit Enter
  4. Nothing happens
  5. I hit Enter again and I am back at the Hello World screen
  6. I can now hit any key to quit

Edit: clarified that I run the code snippet

@gdamore
Copy link
Owner

gdamore commented Dec 17, 2020

So this doesn't seem to occur with Linux or Windows. I believe its specific to macOS. In the past macOS was known to have a tty driver bug -- this might be related.

@gdamore
Copy link
Owner

gdamore commented Dec 17, 2020

See also #394.

I'll further note that it doesn't matter how many characters you enter -- what matters is that you hit enter.
I believe that the problem is that we need to drain the terminal somewhere that isn't occurring.

@gdamore
Copy link
Owner

gdamore commented Dec 17, 2020

I spent a couple of hours trying different things, and I can't seem to figure out how this works.
Notably, the first line of input is discarded (the second is store by the readline). Also this is restricted to the first line of input.
It's confusing because the prompt prints, then we read a line (which is discarded), then we read another line, which is used.

I've been futzing with not using /dev/tty but just using stdin, as well as avoiding duplicate opens of /dev/tty which I think may be confusing matters.

I'm coming to believe that using /dev/tty directly is a bad idea... and I wonder if cview is doing this too.

@gdamore
Copy link
Owner

gdamore commented Jan 24, 2021

Please try again using master. I integrated the fix for #394 which should hopefully resolve this.

@flowchartsman
Copy link

flowchartsman commented Feb 19, 2021

I just tested with the above code on Catalina with iterm2 and latest tcell, and the issue actually seems to be worse. Now the program simply will not return from suspend:
tcell_194

EDIT: for reference (since the gif is best for invisible keystroke behavior, but poor for reproduction), here's what I did on the command line, more or less:

mkdir tcelltest && cd tcelltest
go mod init tcelltest
<put above code into main.go>
go get ./...
go mod edit -replace=github.com/gdamore/tcell/v2=github.com/gdamore/tcell/v2@19e1709 && go mod tidy
go run main.go

Thanks for your hard work and attention @gdamore, this one is a bear, and I'm sure it's doubly a pain if you don't use the platform. If there's anything I can instrument to help dig into the issue, please let me know. I'm not so hot with tty programming, but I'm happy to help.

@flowchartsman
Copy link

I'll also note that I am not running this in any kind of terminal multiplexer, screen or otherwise. Just raw iterm.

@flowchartsman
Copy link

I can also now confirm that both the prior behavior and the new (doesn't resume) behavior are present in both the built in MacOS terminal as well as alacritty.

@gdamore
Copy link
Owner

gdamore commented Feb 20, 2021

I have very good news. I have fixed this and created a Suspend() and Resume() API. This is in my "suspend" branch, and it seems to work well on Linux, Darwin, and Windows. The secret is setting stdin to nonblocking mode when I need to wake up, along with setting the termios to have VMIN and VTIME both set to 0 when I need to do this.

As this represents a new API, there will be a new minor version to track this.

@flowchartsman
Copy link

Great work!

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.

10 participants