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

Move from termion to termwiz for STDIN handling #1249

Merged
merged 10 commits into from
Mar 23, 2022
Merged

Move from termion to termwiz for STDIN handling #1249

merged 10 commits into from
Mar 23, 2022

Conversation

imsnif
Copy link
Member

@imsnif imsnif commented Mar 21, 2022

This change lays down some groundwork for:

  1. windows support (termwiz is cross platform)
  2. multiple modifier key support
  3. allow navigation with alt+arrow keys
  4. much more!

@imsnif imsnif requested a review from tlinford March 21, 2022 10:25
@imsnif imsnif temporarily deployed to cachix March 21, 2022 10:25 Inactive
@imsnif imsnif temporarily deployed to cachix March 21, 2022 10:41 Inactive
@imsnif
Copy link
Member Author

imsnif commented Mar 22, 2022

@tlinford - I'm still working on the flakiness of the e2e tests that this introduced, but would be happy for your input as I'm doing so (time allowing, ofc).

I had to change the normal_mouse_report functions in cases.rs of the e2e tests to make this work, and I'm honestly not sure what the previous function was doing. If you could also take a look at that to make sure I didn't mess it up, that would be grand.

@tlinford
Copy link
Contributor

Hey @imsnif,
I played around with this a bit last evening, seems to be working great!

I left a comment above about the one thing that seems to be missing, which is the mouse event repeater that makes scrolling up/down while selecting work.

Seems like termwiz is doing a lot of things right, the one thing that has me a bit concerned is the mouse events - but I think the solution you made with InputHandler.holding_mouse should be about equivalent to what we have now in termion, which is fine I guess but can get a bit wonky (try for example holding one mouse button and clicking with another).

I had to change the normal_mouse_report functions in cases.rs of the e2e tests to make this work, and I'm honestly not sure what the previous function was doing. If you could also take a look at that to make sure I didn't mess it up, that would be grand.

This is encoding mouse events using the "Normal" (1000) encoding: https://invisible-island.net/xterm/ctlseqs/ctlseqs.html#h3-Normal-tracking-mode. Not sure why it breaks with termwiz, but might be worth investigating a little further, because if it doesn't work we should probably not try and enable it here either:

const ENABLE_MOUSE_SUPPORT: &str = "\u{1b}[?1000h\u{1b}[?1002h\u{1b}[?1015h\u{1b}[?1006h";
.

// MouseRelease) we need to keep sending this instruction to the app,
// because the app itself doesn't have an event loop in the proper
// place
let mut poller = os_input.stdin_poller();
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to readd this, or figure out a better way to do the same thing?
At the moment we've lost the scrolling while selecting behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

Readded, thanks!

@@ -58,15 +58,16 @@ pub const BRACKETED_PASTE_START: [u8; 6] = [27, 91, 50, 48, 48, 126]; // \u{1b}[
pub const BRACKETED_PASTE_END: [u8; 6] = [27, 91, 50, 48, 49, 126]; // \u{1b}[201
pub const SLEEP: [u8; 0] = [];

// simplified, slighty adapted version of alacritty mouse reporting code
pub fn normal_mouse_report(position: Position, button: u8) -> Vec<u8> {
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpicking, but should probably call this sgr_mouse_report now

@imsnif imsnif temporarily deployed to cachix March 22, 2022 15:16 Inactive
@imsnif imsnif temporarily deployed to cachix March 22, 2022 15:16 Inactive
@imsnif imsnif temporarily deployed to cachix March 22, 2022 15:16 Inactive
@imsnif imsnif temporarily deployed to cachix March 22, 2022 15:59 Inactive
@imsnif imsnif temporarily deployed to cachix March 22, 2022 17:15 Inactive
@imsnif imsnif merged commit 7141779 into main Mar 23, 2022
@har7an har7an deleted the excise-termion branch October 23, 2022 15:07
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