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

Crossterm application receiving keyboard input while separate curses process is in foreground #505

Closed
cleeyv opened this issue Oct 29, 2020 · 12 comments

Comments

@cleeyv
Copy link

cleeyv commented Oct 29, 2020

Describe the bug
While running an application (https://github.com/cleeyv/gurk-rs/tree/attachments) using crossterm, I am launching a separate process in the same terminal (a curses file selection dialog from tinyfiledialogs-rs) that opens over top of the crossterm app. The problem I am seeing is that while the file dialog is open, certain "navigation" keyboard inputs (arrows, pgup/pgdn, delete/home/end, but never backspace, spacebar, or any regular character keys) are not always going to the file dialog but are instead often (I'd say approximately half the time) being received by the crossterm app running behind it. The app interprets these navigation input as characters (which makes it seem that perhaps the input is being received in raw mode). As soon as the crossterm app displays one of these inputs this causes the file dialog running in the foreground to close (which seems appropriate) and return to the crossterm app where the character output of the navigation key can be seen. Once it is back in the foreground the crossterm app functions normally again, navigation keys have no effect unless there is a KeyCode setup for them, etc.

For background, these are the lines in which all of the potentially relevant crossterm configuration should be happening:
https://github.com/cleeyv/gurk-rs/blob/attachments/src/main.rs#L38-L82 ending with the on_attach function that launches the curses file dialog (here https://github.com/cleeyv/gurk-rs/blob/attachments/src/app.rs#L270-L278).

I'm very aware that this could be the result of some detail of how gurk is using crossterm, such as running in an AlternateScreen. I have read through all the documentation and also searched through the existing issues and haven't been able to figure out what might need to be done to address this problem so that a curses process is usable when launched from within crossterm. Thank you for any insight you can provide, and for your work on this library!

To Reproduce
Steps to reproduce the behavior:

  1. Launch a curses process from within a crossterm application. To confirm that this is not just an issue with specific implementation details of tinyfiledialogs-rs I also launched ncdu from within gurk and experienced a variation on the same problem.
  2. Hit a "navigation" key such as down arrow a few times until it gets received by the crossterm application.
  3. See the dialog process close and a return to the crossterm application with the characters corresponding to "down arrow" displayed (ncdu did not close, but remained open while becoming a partially overwritten mess as the crossterm app was partially displayed when it received input in the background).

Expected behavior
That the crossterm application does not receive any keyboard input when it is not in the foreground.

OS

  • Manjaro Linux

Terminal/Console

  • Same behaviour in both a getty virtual console and while running the application through ssh in gnome terminal (for tinyfiledialogs-rs), and directly in a gnome terminal using ncdu.
@sigmaSd
Copy link
Contributor

sigmaSd commented Oct 29, 2020

One workaround in sync code you can put crossterm read in a diffrent thread and use std:: thread ::park after each read, and in the main thread unpark it via the thread handle after the dialog has closed.

Im not sure how to do a similar trick with tokio though.

@cleeyv
Copy link
Author

cleeyv commented Nov 9, 2020

Thanks for the feedback, sigmaSd! Based on your suggestion, I did more research and found tokio::sync::Notify which is described in the docs as being "similar to thread::park and Thread::unpark from std". Tokio uses tasks rather than threads and there are two of them that are relevant in gurk, the main one and a sub-task for receiving updates on keyboard input and resizing. I have notified().await keeping both of these tasks waiting while the file dialog is open: https://github.com/cleeyv/gurk-rs/blob/attachments/src/main.rs

Unfortunately, despite both of these tasks being inactive while waiting, the terminal of the crossterm app is still frequently receiving keyboard input from navigation keys, just as it was before (causing the file dialog to close prematurely). This confirms that the keyboard input is not being received as crossterm events, which are inaccessible while the two tasks are waiting. The keyboard input of navigation keys are being received by the terminal, as managed by crossterm, while the curses process is open in the foreground.

Does anyone have any other interpretations or explanations about what might be going on here, or things I could try for further debugging? Other suggestions for workarounds are also welcome.

@sigmaSd
Copy link
Contributor

sigmaSd commented Nov 9, 2020

I tested and your last code works with ncdu, and also works in my case with tinyfiledialog but that probably because it spawns gtk3 dialog.

But when I close the spawned application, some keys gets printed (maximum 2).

But obviously this still feels hacky.

Also why are you spawning a task here https://github.com/cleeyv/gurk-rs/blob/attachments/src/main.rs#L95
I think since you're blocking the ui thread you can await the process and just block crossterm task

index 4489204..a3846a8 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -91,23 +91,20 @@ async fn main() -> anyhow::Result<()> {
                 KeyCode::Right => app.on_right(),
                 KeyCode::Down => app.on_down(),
                 KeyCode::Char('a') if event.modifiers.contains(KeyModifiers::CONTROL) => {
-                    let events3 = events.clone();
-                    tokio::spawn(async move {
                         //let mut debug = OpenOptions::new().append(true).open("debug.txt").unwrap();
-                        let open_file: String;
+                        // let open_file: String;
                         //writeln!(debug, "File dialog about to run...");
                         // TODO use the open_file_dialog_multi function
-                        match tinyfiledialogs::open_file_dialog("Attach file", "", None) {
-                            Some(file) => open_file = file,
-                            None => open_file = "null".to_string(),
-                        }
-                        println!("Open file {:?}", open_file);
+                        // match tinyfiledialogs::open_file_dialog("Attach file", "", None) {
+                            // Some(file) => open_file = file,
+                            // None => open_file = "null".to_string(),
+                        // }
+                        tokio::process::Command::new("ncdu").spawn().expect("z").await;
+                        //println!("Open file {:?}", open_file);
                         //writeln!(debug, "(2)Notify of events about to run...");
-                        events3.notify();
-                    });
+                       events.notify();
                     //let mut debug = OpenOptions::new().append(true).open("debug.txt").unwrap();
                     //writeln!(debug, "(1)Await of events about to run...")?;
-                    events.notified().await;
                     //writeln!(debug, "(3)Events just woke up from await!")?;
                     let size = terminal.size().unwrap();
                     terminal.resize(size)?;

@sigmaSd
Copy link
Contributor

sigmaSd commented Nov 9, 2020

Also broot https://github.com/Canop/broot/blob/master/src/launchable.rs#L126 leaves the alternate screen + disable raw mode before starting a process, maybe you also need to do that

@cleeyv
Copy link
Author

cleeyv commented Nov 11, 2020

Thanks again for your help with this sigmaSd. You're right that I had unnecessary spawning and notifies leftover from earlier testing and experimentation, I will follow your suggestion for cleaning it up. I was hopeful to see that ncdu works using tokio::process::Command! I had only been trying std::Command before and hadn't tried the tokio version with await. The ncurses program that tinyfiledialogs is calling to generate the file selection interface is called dialog so I tried running dialog directly with tokio::process::Command and it still has the same problem! Furthermore, I tried running various other ncurses programs (nnn, tig, ncspot, broot) using tokio::process::Command and they all worked fine, which leads me to the conclusion that this is specifically a problem with how dialog interacts with crossterm in this context. In an effort to better understand what the problem might be I tried using all of the options of dialog that may change this behaviour, and also tried adjusting various options of Command that seem possibly related without any success.

Based on the example you cited from broot I copied their process of leaving the alternate screen and disabling raw mode before launching the file dialog and this didn't seem to change anything.

I'm going to close this issue because it does not seem to be a general problem with crossterm and is more specifically something related to dialog. This is unfortunate though because both of the existing options that I have found for generating a console file dialog from rust (tinyfiledialogs-rs and another rust crate that is also called dialog) are calling this same external dialog program. If anyone reads this and has any insights or ideas to share, I'm still curious to hear them.

Unfortunately there doesn't seem to be any existing crate/library for generating a pure rust file selection dialog in the console. It can always be custom written, but I haven't yet found examples of this being done with tui/crossterm. I found that it has been done here using the cursive library: https://github.com/chrisvest/xxv/blob/master/src/open_file_dialog.rs but writing an equivalent to this for crossterm isn't the project I want to take on right now.

@cleeyv cleeyv closed this as completed Nov 11, 2020
@sigmaSd
Copy link
Contributor

sigmaSd commented Nov 14, 2020

Here is a minimal example
To reproduce, hit arrow keys a couple of time

fn main() {
    std::thread::spawn(|| loop {
        crossterm::event::read();
    });

    xshell::cmd!("dialog --inputbox name: 10 40").run();
}

The actual problem is ncurses interaction with crossterm.
A workaround is to stop the reader, but even parking the thread wont stop it on my tests, I think this is the same underlying issue as #397

@sigmaSd
Copy link
Contributor

sigmaSd commented Nov 14, 2020

Actually testing

tokio::process::Command::new("dialog")
.args(&["--inputbox", "name:", "10", "40"])
.spawn().expect("z").await;

with your program I still can't reproduce the issue, can you share an example of code that doesn't work?

@cleeyv
Copy link
Author

cleeyv commented Nov 17, 2020

I had only been using the fselect widget for dialog, and hadn't tried any of the other ones. When I use the inputbox widget, as you do in your test, the issue is also not reproduced for me. Here is my updated main.rs for testing the two widgets: https://github.com/cleeyv/gurk-rs/blob/attachments/src/main.rs#L97-L98

What I infer from this is that the issue is not even with all of dialog but it is specifically something with the fselect widget. I have looked through the code for it but my experience with C is limited so I was not able to identify anything there that is clearly relevant to this problem.

@sigmaSd
Copy link
Contributor

sigmaSd commented Nov 18, 2020

fn main() {
    std::thread::spawn(|| loop {
        crossterm::event::read();
        //std::thread::park();
    });
    ncurses::initscr();
    loop {
        dbg!(ncurses::getch());
    }
}

This example shows that the arrows keys are eaten by crossterm so instead of getch() returning [27,67,91] (right arrow) it returns only 27.

Parking the thread actually helps but the issue is that the first press is still eaten by crossterm (which is enough to crash dialog), so to fix this not only you need to park the thread but also for crossterm read to actually stop. (or to somehow flush pending reads?)

@sigmaSd
Copy link
Contributor

sigmaSd commented Nov 18, 2020

This appears to workaround your issue

diff --git a/src/main.rs b/src/main.rs
index 4489204..927f1da 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -48,25 +48,25 @@ async fn main() -> anyhow::Result<()> {
     let mut stdout = std::io::stdout();
     execute!(stdout, EnterAlternateScreen, EnableMouseCapture)?;
 
-    let events = Arc::new(Notify::new());
-    let events2 = events.clone();
+	let (tx2, rx2) = std::sync::mpsc::channel();
 
     let (tx, mut rx) = tokio::sync::mpsc::channel(100);
     tokio::spawn({
         let mut tx = tx.clone();
         async move {
             let mut reader = EventStream::new().fuse();
-            while let Some(event) = reader.next().await {
+			loop {
+			rx2.recv();
+            if let Some(event) = reader.next().await {
                 //let mut debug = OpenOptions::new().append(true).open("debug.txt").unwrap();
                 //writeln!(debug, "(A)Await for event stream about to run...");
-                events2.notified().await;
                 //writeln!(debug, "(B)Await for event stream just ran...");
                 match event {
                     Ok(CEvent::Key(key)) => tx.send(Event::Input(key)).await.unwrap(),
                     Ok(CEvent::Resize(_, _)) => tx.send(Event::Resize).await.unwrap(),
                     _ => (),
                 }
-            }
+            } }
         }
     });
 
@@ -79,8 +79,8 @@ async fn main() -> anyhow::Result<()> {
     terminal.clear()?;
 
     loop {
-        events.notify();
         terminal.draw(|f| ui::draw(f, &mut app))?;
+        tx2.send(());
         match rx.recv().await {
             Some(Event::Input(event)) => match event.code {
                 KeyCode::Char('c') if event.modifiers.contains(KeyModifiers::CONTROL) => {
@@ -91,23 +91,22 @@ async fn main() -> anyhow::Result<()> {
                 KeyCode::Right => app.on_right(),
                 KeyCode::Down => app.on_down(),
                 KeyCode::Char('a') if event.modifiers.contains(KeyModifiers::CONTROL) => {
-                    let events3 = events.clone();
-                    tokio::spawn(async move {
                         //let mut debug = OpenOptions::new().append(true).open("debug.txt").unwrap();
-                        let open_file: String;
+                        // let open_file: String;
                         //writeln!(debug, "File dialog about to run...");
                         // TODO use the open_file_dialog_multi function
-                        match tinyfiledialogs::open_file_dialog("Attach file", "", None) {
-                            Some(file) => open_file = file,
-                            None => open_file = "null".to_string(),
-                        }
-                        println!("Open file {:?}", open_file);
+                        // match tinyfiledialogs::open_file_dialog("Attach file", "", None) {
+                            // Some(file) => open_file = file,
+                            // None => open_file = "null".to_string(),
+                        // }
+                        tokio::process::Command::new("dialog")
+						//.args(&["--inputbox", "name:", "10", "40"])
+						.args(&["--fselect", "./", "0", "60"])
+                        .spawn().expect("z").await;
+                        //println!("Open file {:?}", open_file);
                         //writeln!(debug, "(2)Notify of events about to run...");
-                        events3.notify();
-                    });
                     //let mut debug = OpenOptions::new().append(true).open("debug.txt").unwrap();
                     //writeln!(debug, "(1)Await of events about to run...")?;
-                    events.notified().await;
                     //writeln!(debug, "(3)Events just woke up from await!")?;
                     let size = terminal.size().unwrap();
                     terminal.resize(size)?;

@cleeyv
Copy link
Author

cleeyv commented Nov 18, 2020

Amazing! Thank you sigmaSd 😄 With this workaround I now have the dialog file select working through tinyfiledialogs-rs as I was trying to do all along: cleeyv/gurk-rs@8b64c71

@sigmaSd
Copy link
Contributor

sigmaSd commented Nov 19, 2020

Nice! Also note that you probably should use tokio channel instead of std one

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

No branches or pull requests

2 participants