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

Flush terminal output when terminating. #256

Closed
wants to merge 1 commit into from

Conversation

laynor
Copy link
Contributor

@laynor laynor commented Jul 4, 2022

Keyboard input reaches the terminal after the program terminates.
It's possible to type entire commands that get executed once the app quits.

This PR discards the contents of stdin when the app terminates.
There may be cases (crashes?) where this is not enough, but fixes the most common cases (Ctrl-c and flutter app termination via dart:io:exit()).

@ardera
Copy link
Owner

ardera commented Jul 4, 2022

Hey, thanks for the contribution! Though I'm not sure this is the right way to solve this issue.

The flush of stdin at the end of main will only get executed when the program exits sanely, which happens neither when the program is interrupted using Ctrl+C nor when using darts' exit.

The only way to even reach that points is by doing SystemNavigator.pop, which is handled by flutter-pi here. Even then, that codepath is only partially implemented (I wouldn't be surprised if it segfaults on the way) because noone uses that anyway.

Secondly, I'm not really convinced this is even that big of a problem. I tested with other programs and the behaviour is the same, for example try cloning the linux kernel and type commands while it's running, those will get executed afterwards too.

Though if this behaviour is a real problem in your use case we can look into making this work with Ctrl+C and darts exit.

@laynor
Copy link
Contributor Author

laynor commented Jul 4, 2022

@ardera You're right it doesn't solve the issue, we can ditch this PR.
It seems the issue only appears when I exit the app with dart's exit function. Ctrl+C seems not to have the problem. I was probably exiting the app with Ctrl+C assuming they had the same problem, didn't encounter the issue and thought the code I proposed was fixing it. Made a lot of confusion 😅

Re. the similarity with cloning the kernel, git is not an interactive program and is not supposed to eat your input - a flutter app accepting keyboard input is, in my opinion, more akin to vim - when I enter text in vim, I do not expect the input to be executed by the shell once I exit.

The behavior is a problem in any use case where the app self-terminates (or crashes!). In our use case, we terminate the app for auto updates, but I'm not sure the shell would receive the input in that case.

At least in one occasion though, it happened that I inputted sudo reboot in a text field and the PI was rebooted (without exiting the app first!).

EDIT:
As a workaround, I'm flushing stdin in the on_key_event function, as soon as the input is received.

@ardera
Copy link
Owner

ardera commented Jul 4, 2022

Ah I see, you're launching flutter-pi with your physical keyboard. I'm launching via ssh so I didn't see the issue. But in your case it totally makes sense.

I think the issue is actually a bit more complicated. If you launch flutter-pi via ssh, but enter some text inside a flutter app via a physically-attached keyboard, the same keyboard events will still reach the console and possibly be executed. Then even the tcflush won't help because stdin will be the ssh terminal input, not the physical keyboard input.

I actually found a blog post where some people had the exact same problem: https://blog.nanl.de/2011/06/linuxinput-devices-in-qt-embedded-take-care/

I think the solution could be to use EVIOCGRAB on all input devices by default (maybe by adding a ioctl(fd, EVIOCGRAB, 1) inside here)

That EVIOCGRAB ioctl is a bit dumb. So one would have to wait till all keys are released before grabbing the keyboard, otherwise it's possible the terminal will only see the key-down event of you pressing the enter key to launch flutter-pi, not the key-up event, and spam enter characters to stdin.

Furthermore, when the physical keyboard is grabbed, any Ctrl+C entered by that keyboard will no longer be seen by the terminal and won't terminate flutter-pi.

I'm not sure what's the best way to go about this:

  • either we always grab input devices by default, but make it possible to not grab them too via a --nograb cmdline argument
  • or we only grab them in release mode by default (since you're probably not doing Ctrl+C then) (and also add a --nograb and --grab argument)
  • either way, the readme should be updated to inform about the new behaviour

@laynor
Copy link
Contributor Author

laynor commented Jul 5, 2022

or we only grab them in release mode by default (since you're probably not doing Ctrl+C then) (and also add a --nograb and --grab argument)

I like this, it gives the right flexibility :-) Sometimes, in development, it may be useful to test the app in release mode but still have the possibility to kill it by C-c.

I'll prepare a PR for it if I can 👍

@ardera
Copy link
Owner

ardera commented Jul 5, 2022

or we only grab them in release mode by default (since you're probably not doing Ctrl+C then) (and also add a --nograb and --grab argument)

I like this, it gives the right flexibility :-) Sometimes, in development, it may be useful to test the app in release mode but still have the possibility to kill it by C-c.

I'll prepare a PR for it if I can 👍

with release mode I actually meant if flutter-pi is built with -DCMAKE_BUILD_TYPE=Release 😄 but grabbing it if the flutter app is run in release mode might make more sense.

I'll prepare a PR for it if I can 👍

thx!

@ardera ardera mentioned this pull request Oct 26, 2022
@ardera
Copy link
Owner

ardera commented Oct 26, 2022

Closing this for now, tracking it in #298 instead

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.

2 participants