fix(termion): Add fallback for failing cursor detection #981
Conversation
erak
requested review from
orhun,
mindoodoo,
sayanarijit,
joshka,
kdheepak and
Valentin271
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #981 +/- ##
=======================================
- Coverage 92.0% 91.9% -0.1%
=======================================
Files 61 61
Lines 15933 15943 +10
=======================================
+ Hits 14660 14663 +3
- Misses 1273 1280 +7 ☔ View full report in Codecov by Sentry. |
erak
force-pushed
the
termion-cursor
branch
2 times, most recently
from
e6b00d2
to
f033297
Compare
src/backend/termion.rs
Outdated
| match termion::cursor::DetectCursorPos::cursor_pos(&mut self.writer) { | ||
| Ok((x, y)) => { | ||
| self.cursor = Some((x - 1, y - 1)); | ||
| Ok((x - 1, y - 1)) | ||
| } | ||
| Err(_) => self | ||
| .cursor | ||
| .ok_or_else(|| io::Error::other("No last known cursor position")), | ||
| } |
There was a problem hiding this comment.
Personally I would keep the map logic and do the match after that. Then there is no duplicate x-1,y-1 logic needed and its only in one place.
There was a problem hiding this comment.
Kept the match structure as it was, but introduced a local variable that holds the mapped cursor.
| } | ||
| Err(_) => self | ||
| .cursor | ||
| .ok_or_else(|| io::Error::other("No last known cursor position")), |
There was a problem hiding this comment.
Can a cursor position of 0,0 just be assumed as a fallback to the fallback? Or could that have issues?
There was a problem hiding this comment.
I think that could work. I discovered this issue with auto-resizing an inline viewport. Can't tell if that would have other side effects then the frame being redrawn at 0,0.
erak
force-pushed
the
termion-cursor
branch
3 times, most recently
from
c9c857e
to
0e2bdbb
Compare
|
I'd need some advice on the unit testing. So currently, both tests that we're added to this PR do fail on terminal let backend = TermionBackend::new(&mut stdout);
let mut terminal = Terminal::new(backend)?;I could change the construction to use terminal options, which would work. But then, retrieving the cursor position seems to block and the test never finishes. let backend = TermionBackend::new(&mut stdout);
let area = Rect::new(0, 0, 3, 1);
let mut terminal = Terminal::with_options(
backend,
TerminalOptions {
viewport: Viewport::Fixed(area),
},
)?; |
|
Are the tests failing locally for you as well, or is it just failing on GitHub Actions? |
They fail with |
|
Reported to |
With
TermionBackend, when the cursor position onstdoutis being read from another thread then the onereading events from
stdin, cursor detection will fail. See: https://gitlab.redox-os.org/redox-os/termion/-/issues/173The workaround is to always store the last known cursor position and use it as fallback.