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

Open /dev/tty in non-blocking mode #454

Closed
wants to merge 1 commit into from

Conversation

tslocum
Copy link
Contributor

@tslocum tslocum commented Apr 25, 2021

Resolves #452.

@tslocum
Copy link
Contributor Author

tslocum commented Apr 25, 2021

See https://code.rocketnine.space/tslocum/cview/issues/58 for additional discussion.

@noborus
Copy link
Contributor

noborus commented Apr 25, 2021

Thank you very much.
It worked fine in my environment.

@hhirtz
Copy link
Contributor

hhirtz commented Apr 27, 2021

Can confirm this fixes the issue introduced by 7694d90

However, it seems to add a slight delay when quitting an application. I'll try and make a minimal example to show this.

for {
select {
case <-stopQ:
return
default:
}

err = t.in.SetReadDeadline(time.Now().Add(250 * time.Millisecond))
Copy link
Contributor

@hhirtz hhirtz Apr 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, 250ms feels like the delay I had.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This causes us to wake up every 250ms. Which is vile on some platforms. I was hoping to avoid period polling.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...Okay? What do you propose instead, considering the issues regarding suspend and resume (#452)?

@@ -102,15 +102,19 @@ func (t *tScreen) disengage() {
// so that it can be restored when the application terminates.
func (t *tScreen) initialize() error {
var err error
t.out = os.Stdout
if t.in, err = os.Open("/dev/tty"); err != nil {
t.inFd, err = syscall.Open("/dev/tty", syscall.O_RDONLY|syscall.O_NONBLOCK, 0644)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be necessary -- I believe os.Open() will do what you need, and you can call SetNonBlocking() if need be.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I already tried this (on Linux) to no effect.

if err == nil {
return nil
t.saved, err = term.GetState(t.inFd)
if err != nil {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a bug in the old code ... thanks for noticing it. :-)

@tslocum
Copy link
Contributor Author

tslocum commented May 11, 2021

In this comment you state:

Argh. There is no way to win. Some want /dev/tty. But that has warts like this. I may go back to stdin and give an alternative api for the few folks who don’t agree with that approach.

Are you planning on returning to STDIN? This PR's only purpose is to work around issues introduced by 7694d90, so it can be closed if that is the case.

@gdamore
Copy link
Owner

gdamore commented May 11, 2021

I think I'll probably return to stdin, and offer an API for people to supply a file descriptor of their own creation. I just haven't done it yet.

@gdamore
Copy link
Owner

gdamore commented May 15, 2021

I've taken a different approach entirely -- doing some more experimentation. Please see my "tty" branch for an example. This still uses /dev/tty, but it solves the blocking on disengage, and provides a pluggable interface for folks who want to build a different I/O layer underneath entirely (e.g. for a websocket terminal or something.)

@gdamore
Copy link
Owner

gdamore commented May 15, 2021

My PR for this is #459 -- would appreciate test feedback.

@gdamore
Copy link
Owner

gdamore commented May 16, 2021

I've merged #459 -- after making some additional fixes for darwin (which has a buggy tty driver in how it handles close()). Those changes make this PR obsolete.

@gdamore gdamore closed this May 16, 2021
hhirtz added a commit to hhirtz/senpai that referenced this pull request Aug 6, 2021
tcell 2.2.1 has a bug [0][1][2] that made senpai need a keypress after
Ctrl-C was hit.

[0] gdamore/tcell#452
[1] gdamore/tcell#454
[2] gdamore/tcell@7694d90
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 this pull request may close these issues.

Lost a key event once when exiting or suspending in v2.2.1.
4 participants