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

upgrade termwiz to 0.7.2 #24

Merged
merged 1 commit into from
Apr 8, 2020

Conversation

quark-zju
Copy link
Contributor

@quark-zju quark-zju commented Apr 7, 2020

termwiz 0.7.2 has SHIFT key normalization to make keys like # work on Windows.

src/screen.rs Outdated
// does not assume US keyboard.
let modifiers = if cfg!(windows)
&& key.modifiers.contains(SHIFT)
&& ":{}()<>#?".chars().any(|ch| key.key == Char(ch))
Copy link
Contributor

@wez wez Apr 7, 2020

Choose a reason for hiding this comment

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

Ah, this stuff is tricky because it technically should know things about the keymap. But, if we consider only SHIFT, I believe this statement is true regardless of the keymap, and the platform:

If the character is ascii alphanumeric or ascii punctuation then the effect of the shift key has already been applied to produce the current character code.

If that is true then we can always just discard the SHIFT modifier from the effective set.

WIth that in mind, I think this code turns into this (which is a superset of the characters you were already testing); we only need to check for punctuation here because normalize_shift_to_upper_case already handled the alpha cases in the core termwiz code:

let modifiers = match key.key {
    Char(ch) if ch.is_ascii_punctuation() => key.modifiers - SHIFT,
    _ => key.modifiers
};

Copy link
Contributor Author

@quark-zju quark-zju Apr 7, 2020

Choose a reason for hiding this comment

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

Nice to know it's is_ascii_punctuation normalized in termwiz! I tested it using termwiz master and it works as expected. However termwiz 0.7.1 still requires manual normalization for some reason.

I bisected it to wez/wezterm@fe89082 (starting from that commit, extra normalization is unnecessary). Maybe it's time for a new point release? Then this PR can then be just upgrading termwiz. (a funny note: I just learned that git bisect refuses to work for bad -> good transition. I now appreciate hg bisect more).

Copy link
Contributor

@wez wez Apr 7, 2020

Choose a reason for hiding this comment

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

I've published termwiz 0.7.2:

> cargo run --example key_tester
   Compiling termwiz v0.7.2 (C:\Users\wez\wez-personal\wezterm\termwiz)
    Finished dev [unoptimized + debuginfo] target(s) in 2.80s
     Running `C:\Users\wez\wez-personal\wezterm\target\debug\examples\key_tester.exe`
Key(KeyEvent { key: Escape, modifiers: NONE })
Key(KeyEvent { key: Char('\''), modifiers: NONE })
Key(KeyEvent { key: Shift, modifiers: SHIFT })
Key(KeyEvent { key: Char('{'), modifiers: NONE })
Key(KeyEvent { key: Char('['), modifiers: NONE })
Key(KeyEvent { key: Char('C'), modifiers: CTRL })

wez added a commit to wez/wezterm that referenced this pull request Apr 7, 2020
This backports the input processing portion of
fe89082
and the follow-on commit
d98fee9
to termwiz 0.7.

This has the side effect normalizing the shift state for keys that have been
recognized as unicode by the terminal processing layer.

refs: markbt/streampager#24

Test Plan: `cd termwiz ; cargo run --example key_tester` and press some
keys:

```
> cargo run --example key_tester
   Compiling termwiz v0.7.1 (C:\Users\wez\wez-personal\wezterm\termwiz)
    Finished dev [unoptimized + debuginfo] target(s) in 2.80s
     Running `C:\Users\wez\wez-personal\wezterm\target\debug\examples\key_tester.exe`
Key(KeyEvent { key: Escape, modifiers: NONE })
Key(KeyEvent { key: Char('\''), modifiers: NONE })
Key(KeyEvent { key: Shift, modifiers: SHIFT })
Key(KeyEvent { key: Char('{'), modifiers: NONE })
Key(KeyEvent { key: Char('['), modifiers: NONE })
Key(KeyEvent { key: Char('C'), modifiers: CTRL })
```
termwiz 0.7.2 has logic to normalize SHIFT key modifiers on
Windows. This ensures keys like '#', ':', '<', '>' are working
properly.
@quark-zju quark-zju force-pushed the normalize-win-key-modifiers branch from 732b67a to 6b4bcf9 Compare April 7, 2020 18:58
@quark-zju quark-zju changed the title input: normalize key modifiers on Windows upgrade termwiz to 0.7.2 Apr 7, 2020
@markbt markbt merged commit b72f993 into markbt:master Apr 8, 2020
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.

None yet

3 participants