Input Reader consumes cursor position LINUX

[sigmaSd]

Hi, first thanks a lot for this awesome project!
While most of time everything is fine typing fast unveils a lot of problems, in particular
https://github.com/TimonPost/crossterm/blob/master/crossterm_cursor/src/sys/unix.rs#L6 returns (0, 0) (when typing fast) so I end up unable to depend on cursor.pos(), also a side effect I noticed
RawScreen::into_raw_mode() gets disabled after the first key stroke for example here

let screen = RawScreen::into_raw_mode();
let mut input = input();
let stdin = input.read_sync();

loop {
    if let Some(key_event) = stdin.next() {
     match key_event {}
    }
}

Currently to workaround that I call raw screen on each cycle of the loop:

loop {
    let screen = RawScreen::into_raw_mode();
    if let Some(key_event) = stdin.next() {
     match key_event {}
    }
}

also if use a hack like

while cursor.pos() == (0, 0) {
 continue
}

rawscreen works normally outside the loop and doesn't get disabled, but a lot of weird issues appear

Hopefully this makes sense, maybe a timeout is needed but I'm not quite sure where the problem lies that's why I'm opening this issue for awareness.

Tested on linux gnome-terminal(wayland)

[TimonPost]

Is this issue related to #122? In that case, it is probably already fixed in master

[sigmaSd]

yeah already tested on master, and its seems related to #122 but the fix isn't enough it seems
I made this patch

diff --git a/crossterm_cursor/src/cursor/ansi_cursor.rs b/crossterm_cursor/src/cursor/ansi_cursor.rs
index c7d7c4a..466a685 100644
--- a/crossterm_cursor/src/cursor/ansi_cursor.rs
+++ b/crossterm_cursor/src/cursor/ansi_cursor.rs
@@ -9,11 +9,15 @@ use std::io::Write;
 use crossterm_utils::Result;
 
 /// This struct is an ANSI implementation for cursor related actions.
-pub struct AnsiCursor;
+pub struct AnsiCursor {
+	pos: (u16, u16),
+}
 
 impl AnsiCursor {
     pub fn new() -> AnsiCursor {
-        AnsiCursor
+        AnsiCursor {
+        	pos: (0, 0)
+        }
     }
 }
 
@@ -23,8 +27,13 @@ impl ITerminalCursor for AnsiCursor {
         Ok(())
     }
 
-    fn pos(&self) -> (u16, u16) {
-        get_cursor_position()
+    fn pos(&mut self) -> (u16, u16) {
+        if let Ok(pos) = get_cursor_position() {
+			self.pos = pos;
+        	pos
+        } else {
+        	self.pos
+        }
     }
 
     fn move_up(&self, count: u16) -> Result<()> {
diff --git a/crossterm_cursor/src/cursor/cursor.rs b/crossterm_cursor/src/cursor/cursor.rs
index 184e365..d465e67 100644
--- a/crossterm_cursor/src/cursor/cursor.rs
+++ b/crossterm_cursor/src/cursor/cursor.rs
@@ -57,7 +57,7 @@ impl TerminalCursor {
     ///
     /// # Remarks
     /// position is 0-based, which means we start counting at 0.
-    pub fn pos(&self) -> (u16, u16) {
+    pub fn pos(&mut self) -> (u16, u16) {
         self.cursor.pos()
     }
 
diff --git a/crossterm_cursor/src/cursor/mod.rs b/crossterm_cursor/src/cursor/mod.rs
index c855ab4..8b7ee49 100644
--- a/crossterm_cursor/src/cursor/mod.rs
+++ b/crossterm_cursor/src/cursor/mod.rs
@@ -31,7 +31,7 @@ trait ITerminalCursor: Sync + Send {
     /// Goto some location (x,y) in the context.
     fn goto(&self, x: u16, y: u16) -> Result<()>;
     /// Get the location (x,y) of the current cursor in the context
-    fn pos(&self) -> (u16, u16);
+    fn pos(&mut self) -> (u16, u16);
     /// Move cursor n times up
     fn move_up(&self, count: u16) -> Result<()>;
     /// Move the cursor `n` times to the right.
diff --git a/crossterm_cursor/src/sys/unix.rs b/crossterm_cursor/src/sys/unix.rs
index edd6145..fd8ade7 100644
--- a/crossterm_cursor/src/sys/unix.rs
+++ b/crossterm_cursor/src/sys/unix.rs
@@ -3,19 +3,11 @@ use std::io::{self, Error, ErrorKind, Read, Write};
 
 /// Get the cursor position based on the current platform.
 #[cfg(unix)]
-pub fn get_cursor_position() -> (u16, u16) {
+pub fn get_cursor_position() -> std::io::Result<(u16, u16)> {
     if unsafe { RAW_MODE_ENABLED } {
-        if let Ok(pos) = pos_raw() {
-            pos
-        } else {
-            (0, 0)
-        }
+        pos_raw()
     } else {
-        if let Ok(pos) = pos() {
-            pos
-        } else {
-            (0, 0)
-        }
+        pos()
     }
 }

diff --git a/crossterm_cursor/src/sys/winapi.rs b/crossterm_cursor/src/sys/winapi.rs
index 9787526..ce72e86 100644
--- a/crossterm_cursor/src/sys/winapi.rs
+++ b/crossterm_cursor/src/sys/winapi.rs
@@ -1,11 +1,11 @@
 //! This module handles some logic for cursor interaction in the windows console.
 
 #[cfg(windows)]
-pub fn get_cursor_position() -> (u16, u16) {
+pub fn get_cursor_position() -> Result<(u16, u16), &'static str> {
     if let Ok(cursor) = Cursor::new() {
-        cursor.position().unwrap().into()
+        Ok(cursor.position().unwrap().into())
     } else {
-        (0, 0)
+        Err("test")
     }
 }

this makes things better, I can call intoscreenraw outside of the loop and I get less race conditions when I type fast, but it looks more like a workaround for a problem that I still can't pinpoint

[TimonPost]

What do you mean with race conditions? You aren't using two threads. Also by making pos() mutable it will make the API a bit more difficult to use. I'll try with your code as well to see if I can track down why this is. But feel free to make the PR! Thanks for the research.

[sigmaSd]

I will happily make a pr one I figure out what exactly is the issue.
For the race condition here is a short example of what I mean

use crossterm::*;

pub struct Term {
    cursor: TerminalCursor,
    terminal: Terminal,
    input: TerminalInput,
}

impl Term {
    pub fn new() -> Self {
        let crossterm = Crossterm::new();
        let cursor = crossterm.cursor();
        let terminal = crossterm.terminal();
        let input = crossterm.input();

        Term {
            cursor,
            terminal,
            input,
        }
    }

    pub fn run(&mut self) -> std::io::Result<()> {
        let mut stdin = self.input.read_sync();
        let _screen = crossterm::RawScreen::into_raw_mode()?;
        loop {

            if let Some(key_event) = stdin.next() {
                match key_event {
                	_ => {self.terminal.write(&format!("{} {} || ", self.cursor.pos().0, self.cursor.pos().1));},
                }
            }
        }
    }
}

fn main() {
	let mut term = Term::new();
	term.run().expect("ouch");
}

If I type slowly everything works fine but if I type fast I get a lot of 0 0 || 0 0 || 0 0 || 0 0 || 0 0 || 0 0 ||
I feel like cursor pos is the root of all my current problems, I just need to figure out why.
Also you can take a look at https://github.com/lotabout/tuikit I'm not quite experienced with tui's so maybe you can check how cursor detection is implemented there

[sigmaSd]

new hint,

    pub fn run(&mut self) -> std::io::Result<()> {
        let mut stdin = self.input.read_sync();
        let _screen = crossterm::RawScreen::into_raw_mode()?;
        loop {

            if let Some(key_event) = dbg!(stdin.next()) {
            	let _ = self.cursor.pos();
                match key_event {
                	_ => ()//{self.terminal.write(&format!("{} {} || ", self.cursor.pos().0, self.cursor.pos().1));},
                }
            }
        }
    }

if I use self.cursor.pos() inside the loop and type fast sometimes I get stdin.next() = Some(Unknown)
also tuikit do something that may mean we have to ignore some keys till we get a pos in https://github.com/lotabout/tuikit/blob/master/src/term.rs#L173
Maybe cursor pos should depend on this Unknown event

[sigmaSd]

I think I figured out the issue now, the race condition is acessing stdin

diff --git a/crossterm_cursor/src/sys/unix.rs b/crossterm_cursor/src/sys/unix.rs
index edd6145..84dc0c7 100644
--- a/crossterm_cursor/src/sys/unix.rs
+++ b/crossterm_cursor/src/sys/unix.rs
@@ -30,18 +30,23 @@ pub fn pos_raw() -> io::Result<(u16, u16)> {
     // Where is the cursor?
     // Use `ESC [ 6 n`.
     let mut stdout = io::stdout();
+    let mut stdin = io::stdin();
 
-    // Write command
-    stdout.write_all(b"\x1B[6n")?;
-    stdout.flush()?;
-
+    // Ask for cpr till we get a correct answer
     let mut buf = [0u8; 2];
+    loop {
+    // Write command
+    stdout.write_all(b"\x1B[6n")?;
+    stdout.flush()?;
 
-    // Expect `ESC[`
-    io::stdin().read_exact(&mut buf)?;
-    if buf[0] != 0x1B || buf[1] as char != '[' {
-        return Err(Error::new(ErrorKind::Other, "test"));
-    }
+    // Expect `ESC[`
+    stdin.read_exact(&mut buf)?;
+    if buf[0] != 0x1B || buf[1] as char != '[' {
+ 	continue
+    } else {
+   	break
+       }
+    }

when typing fast stdin will still has some leftovers so this wont work

if buf[0] != 0x1B || buf[1] as char != '[' {
-        return Err(Error::new(ErrorKind::Other, "test"));
- }

Its better with this patch but I wonder if there is a better way than looping.

Also for changes that this means
without this patch multi-line input like terminal.write("hello\nworld") would look like

hello
world

because pos errors and reset to 0
but with this patch since pos is correct it looks like

hello
       world

so when writing multi-line we have to reset pos.x to 0

[sigmaSd]

I finally fixed my program by not relying on cursor.pos() at all and using my own custom cursor struct.
calling cursor.pos() just causes a lot of a problem, I suggest you just remove it from the API and let the users of the library manage it manually.
Looking again at https://github.com/lotabout/tuikit, it also doesn't expose it, calling pos() is just UB xD
Also since I brought the API subject, would you consider changing u16 to usize, its way more easier to deal with instead of having to cast everywhere, in particular in indexing collections.

[kkuehlz]

@sigmaSd This is more than just cursor.pos(). If you are using an AsyncReader (a thread + channel), then any terminal escape sequences can get sucked into your input stream.

[sigmaSd]

@keur I only used sync till now so I wouldn't now, anyhow everything works great now that I'm not using cursor.pos() at all

[sigmaSd]

this patch works perfectly not really xD

--- a/crossterm_cursor/src/sys/unix.rs
+++ b/crossterm_cursor/src/sys/unix.rs
@@ -30,6 +30,7 @@ pub fn pos_raw() -> io::Result<(u16, u16)> {
     // Where is the cursor?
     // Use `ESC [ 6 n`.
     let mut stdout = io::stdout();
+    let mut stdin = io::stdin();
 
     // Write command
     stdout.write_all(b"\x1B[6n")?;
@@ -37,10 +38,14 @@ pub fn pos_raw() -> io::Result<(u16, u16)> {
 
     let mut buf = [0u8; 2];
 
-    // Expect `ESC[`
-    io::stdin().read_exact(&mut buf)?;
-    if buf[0] != 0x1B || buf[1] as char != '[' {
-        return Err(Error::new(ErrorKind::Other, "test"));
+    loop {
+        // Expect `ESC[`
+        stdin.read(&mut buf)?;
+        if buf != [27, 91] {
+            continue;
+        } else {
+            break;
+        }
      }

[TimonPost]

Fixed by #152