Rename ime_key -> key_char and update behavior (#20953)

As part of the recent changes to keyboard support, ime_key is no longer
populated by the IME; but instead by the keyboard.

As part of #20877 I changed some code to assume that falling back to key
was
ok, but this was not ok; instead we need to populate this more similarly
to how
it was done before #20336.

The alternative fix could be to instead of simulating these events in
our own
code to push a fake native event back to the platform input handler.

Closes #ISSUE

Release Notes:

- Fixed a bug where tapping `shift` coudl type "shift" if you had a
binding on "shift shift"
This commit is contained in:
Conrad Irwin 2024-11-20 20:29:47 -07:00 committed by GitHub
parent 37a59d6b2e
commit e062f30d9e
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
14 changed files with 77 additions and 79 deletions

View file

@ -581,8 +581,8 @@ impl Render for InputExample {
format!( format!(
"{:} {}", "{:} {}",
ks.unparse(), ks.unparse(),
if let Some(ime_key) = ks.ime_key.as_ref() { if let Some(key_char) = ks.key_char.as_ref() {
format!("-> {:?}", ime_key) format!("-> {:?}", key_char)
} else { } else {
"".to_owned() "".to_owned()
} }

View file

@ -12,14 +12,15 @@ pub struct Keystroke {
/// e.g. for option-s, key is "s" /// e.g. for option-s, key is "s"
pub key: String, pub key: String,
/// ime_key is the character inserted by the IME engine when that key was pressed. /// key_char is the character that could have been typed when
/// e.g. for option-s, ime_key is "ß" /// this binding was pressed.
pub ime_key: Option<String>, /// e.g. for s this is "s", for option-s "ß", and cmd-s None
pub key_char: Option<String>,
} }
impl Keystroke { impl Keystroke {
/// When matching a key we cannot know whether the user intended to type /// When matching a key we cannot know whether the user intended to type
/// the ime_key or the key itself. On some non-US keyboards keys we use in our /// the key_char or the key itself. On some non-US keyboards keys we use in our
/// bindings are behind option (for example `$` is typed `alt-ç` on a Czech keyboard), /// bindings are behind option (for example `$` is typed `alt-ç` on a Czech keyboard),
/// and on some keyboards the IME handler converts a sequence of keys into a /// and on some keyboards the IME handler converts a sequence of keys into a
/// specific character (for example `"` is typed as `" space` on a brazilian keyboard). /// specific character (for example `"` is typed as `" space` on a brazilian keyboard).
@ -27,10 +28,10 @@ impl Keystroke {
/// This method assumes that `self` was typed and `target' is in the keymap, and checks /// This method assumes that `self` was typed and `target' is in the keymap, and checks
/// both possibilities for self against the target. /// both possibilities for self against the target.
pub(crate) fn should_match(&self, target: &Keystroke) -> bool { pub(crate) fn should_match(&self, target: &Keystroke) -> bool {
if let Some(ime_key) = self if let Some(key_char) = self
.ime_key .key_char
.as_ref() .as_ref()
.filter(|ime_key| ime_key != &&self.key) .filter(|key_char| key_char != &&self.key)
{ {
let ime_modifiers = Modifiers { let ime_modifiers = Modifiers {
control: self.modifiers.control, control: self.modifiers.control,
@ -38,7 +39,7 @@ impl Keystroke {
..Default::default() ..Default::default()
}; };
if &target.key == ime_key && target.modifiers == ime_modifiers { if &target.key == key_char && target.modifiers == ime_modifiers {
return true; return true;
} }
} }
@ -47,9 +48,9 @@ impl Keystroke {
} }
/// key syntax is: /// key syntax is:
/// [ctrl-][alt-][shift-][cmd-][fn-]key[->ime_key] /// [ctrl-][alt-][shift-][cmd-][fn-]key[->key_char]
/// ime_key syntax is only used for generating test events, /// key_char syntax is only used for generating test events,
/// when matching a key with an ime_key set will be matched without it. /// when matching a key with an key_char set will be matched without it.
pub fn parse(source: &str) -> anyhow::Result<Self> { pub fn parse(source: &str) -> anyhow::Result<Self> {
let mut control = false; let mut control = false;
let mut alt = false; let mut alt = false;
@ -57,7 +58,7 @@ impl Keystroke {
let mut platform = false; let mut platform = false;
let mut function = false; let mut function = false;
let mut key = None; let mut key = None;
let mut ime_key = None; let mut key_char = None;
let mut components = source.split('-').peekable(); let mut components = source.split('-').peekable();
while let Some(component) = components.next() { while let Some(component) = components.next() {
@ -74,7 +75,7 @@ impl Keystroke {
break; break;
} else if next.len() > 1 && next.starts_with('>') { } else if next.len() > 1 && next.starts_with('>') {
key = Some(String::from(component)); key = Some(String::from(component));
ime_key = Some(String::from(&next[1..])); key_char = Some(String::from(&next[1..]));
components.next(); components.next();
} else { } else {
return Err(anyhow!("Invalid keystroke `{}`", source)); return Err(anyhow!("Invalid keystroke `{}`", source));
@ -118,7 +119,7 @@ impl Keystroke {
function, function,
}, },
key, key,
ime_key, key_char: key_char,
}) })
} }
@ -154,7 +155,7 @@ impl Keystroke {
/// Returns true if this keystroke left /// Returns true if this keystroke left
/// the ime system in an incomplete state. /// the ime system in an incomplete state.
pub fn is_ime_in_progress(&self) -> bool { pub fn is_ime_in_progress(&self) -> bool {
self.ime_key.is_none() self.key_char.is_none()
&& (is_printable_key(&self.key) || self.key.is_empty()) && (is_printable_key(&self.key) || self.key.is_empty())
&& !(self.modifiers.platform && !(self.modifiers.platform
|| self.modifiers.control || self.modifiers.control
@ -162,17 +163,17 @@ impl Keystroke {
|| self.modifiers.alt) || self.modifiers.alt)
} }
/// Returns a new keystroke with the ime_key filled. /// Returns a new keystroke with the key_char filled.
/// This is used for dispatch_keystroke where we want users to /// This is used for dispatch_keystroke where we want users to
/// be able to simulate typing "space", etc. /// be able to simulate typing "space", etc.
pub fn with_simulated_ime(mut self) -> Self { pub fn with_simulated_ime(mut self) -> Self {
if self.ime_key.is_none() if self.key_char.is_none()
&& !self.modifiers.platform && !self.modifiers.platform
&& !self.modifiers.control && !self.modifiers.control
&& !self.modifiers.function && !self.modifiers.function
&& !self.modifiers.alt && !self.modifiers.alt
{ {
self.ime_key = match self.key.as_str() { self.key_char = match self.key.as_str() {
"space" => Some(" ".into()), "space" => Some(" ".into()),
"tab" => Some("\t".into()), "tab" => Some("\t".into()),
"enter" => Some("\n".into()), "enter" => Some("\n".into()),

View file

@ -742,14 +742,14 @@ impl Keystroke {
} }
} }
// Ignore control characters (and DEL) for the purposes of ime_key // Ignore control characters (and DEL) for the purposes of key_char
let ime_key = let key_char =
(key_utf32 >= 32 && key_utf32 != 127 && !key_utf8.is_empty()).then_some(key_utf8); (key_utf32 >= 32 && key_utf32 != 127 && !key_utf8.is_empty()).then_some(key_utf8);
Keystroke { Keystroke {
modifiers, modifiers,
key, key,
ime_key, key_char,
} }
} }

View file

@ -1208,7 +1208,7 @@ impl Dispatch<wl_keyboard::WlKeyboard, ()> for WaylandClientStatePtr {
compose.feed(keysym); compose.feed(keysym);
match compose.status() { match compose.status() {
xkb::Status::Composing => { xkb::Status::Composing => {
keystroke.ime_key = None; keystroke.key_char = None;
state.pre_edit_text = state.pre_edit_text =
compose.utf8().or(Keystroke::underlying_dead_key(keysym)); compose.utf8().or(Keystroke::underlying_dead_key(keysym));
let pre_edit = let pre_edit =
@ -1220,7 +1220,7 @@ impl Dispatch<wl_keyboard::WlKeyboard, ()> for WaylandClientStatePtr {
xkb::Status::Composed => { xkb::Status::Composed => {
state.pre_edit_text.take(); state.pre_edit_text.take();
keystroke.ime_key = compose.utf8(); keystroke.key_char = compose.utf8();
if let Some(keysym) = compose.keysym() { if let Some(keysym) = compose.keysym() {
keystroke.key = xkb::keysym_get_name(keysym); keystroke.key = xkb::keysym_get_name(keysym);
} }
@ -1340,7 +1340,7 @@ impl Dispatch<zwp_text_input_v3::ZwpTextInputV3, ()> for WaylandClientStatePtr {
keystroke: Keystroke { keystroke: Keystroke {
modifiers: Modifiers::default(), modifiers: Modifiers::default(),
key: commit_text.clone(), key: commit_text.clone(),
ime_key: Some(commit_text), key_char: Some(commit_text),
}, },
is_held: false, is_held: false,
})); }));

View file

@ -687,11 +687,11 @@ impl WaylandWindowStatePtr {
} }
} }
if let PlatformInput::KeyDown(event) = input { if let PlatformInput::KeyDown(event) = input {
if let Some(ime_key) = &event.keystroke.ime_key { if let Some(key_char) = &event.keystroke.key_char {
let mut state = self.state.borrow_mut(); let mut state = self.state.borrow_mut();
if let Some(mut input_handler) = state.input_handler.take() { if let Some(mut input_handler) = state.input_handler.take() {
drop(state); drop(state);
input_handler.replace_text_in_range(None, ime_key); input_handler.replace_text_in_range(None, key_char);
self.state.borrow_mut().input_handler = Some(input_handler); self.state.borrow_mut().input_handler = Some(input_handler);
} }
} }

View file

@ -178,7 +178,7 @@ pub struct X11ClientState {
pub(crate) compose_state: Option<xkbc::compose::State>, pub(crate) compose_state: Option<xkbc::compose::State>,
pub(crate) pre_edit_text: Option<String>, pub(crate) pre_edit_text: Option<String>,
pub(crate) composing: bool, pub(crate) composing: bool,
pub(crate) pre_ime_key_down: Option<Keystroke>, pub(crate) pre_key_char_down: Option<Keystroke>,
pub(crate) cursor_handle: cursor::Handle, pub(crate) cursor_handle: cursor::Handle,
pub(crate) cursor_styles: HashMap<xproto::Window, CursorStyle>, pub(crate) cursor_styles: HashMap<xproto::Window, CursorStyle>,
pub(crate) cursor_cache: HashMap<CursorStyle, xproto::Cursor>, pub(crate) cursor_cache: HashMap<CursorStyle, xproto::Cursor>,
@ -446,7 +446,7 @@ impl X11Client {
compose_state, compose_state,
pre_edit_text: None, pre_edit_text: None,
pre_ime_key_down: None, pre_key_char_down: None,
composing: false, composing: false,
cursor_handle, cursor_handle,
@ -858,7 +858,7 @@ impl X11Client {
let modifiers = modifiers_from_state(event.state); let modifiers = modifiers_from_state(event.state);
state.modifiers = modifiers; state.modifiers = modifiers;
state.pre_ime_key_down.take(); state.pre_key_char_down.take();
let keystroke = { let keystroke = {
let code = event.detail.into(); let code = event.detail.into();
let xkb_state = state.previous_xkb_state.clone(); let xkb_state = state.previous_xkb_state.clone();
@ -880,13 +880,13 @@ impl X11Client {
match compose_state.status() { match compose_state.status() {
xkbc::Status::Composed => { xkbc::Status::Composed => {
state.pre_edit_text.take(); state.pre_edit_text.take();
keystroke.ime_key = compose_state.utf8(); keystroke.key_char = compose_state.utf8();
if let Some(keysym) = compose_state.keysym() { if let Some(keysym) = compose_state.keysym() {
keystroke.key = xkbc::keysym_get_name(keysym); keystroke.key = xkbc::keysym_get_name(keysym);
} }
} }
xkbc::Status::Composing => { xkbc::Status::Composing => {
keystroke.ime_key = None; keystroke.key_char = None;
state.pre_edit_text = compose_state state.pre_edit_text = compose_state
.utf8() .utf8()
.or(crate::Keystroke::underlying_dead_key(keysym)); .or(crate::Keystroke::underlying_dead_key(keysym));
@ -1156,7 +1156,7 @@ impl X11Client {
match event { match event {
Event::KeyPress(event) | Event::KeyRelease(event) => { Event::KeyPress(event) | Event::KeyRelease(event) => {
let mut state = self.0.borrow_mut(); let mut state = self.0.borrow_mut();
state.pre_ime_key_down = Some(Keystroke::from_xkb( state.pre_key_char_down = Some(Keystroke::from_xkb(
&state.xkb, &state.xkb,
state.modifiers, state.modifiers,
event.detail.into(), event.detail.into(),
@ -1187,11 +1187,11 @@ impl X11Client {
fn xim_handle_commit(&self, window: xproto::Window, text: String) -> Option<()> { fn xim_handle_commit(&self, window: xproto::Window, text: String) -> Option<()> {
let window = self.get_window(window).unwrap(); let window = self.get_window(window).unwrap();
let mut state = self.0.borrow_mut(); let mut state = self.0.borrow_mut();
let keystroke = state.pre_ime_key_down.take(); let keystroke = state.pre_key_char_down.take();
state.composing = false; state.composing = false;
drop(state); drop(state);
if let Some(mut keystroke) = keystroke { if let Some(mut keystroke) = keystroke {
keystroke.ime_key = Some(text.clone()); keystroke.key_char = Some(text.clone());
window.handle_input(PlatformInput::KeyDown(crate::KeyDownEvent { window.handle_input(PlatformInput::KeyDown(crate::KeyDownEvent {
keystroke, keystroke,
is_held: false, is_held: false,

View file

@ -846,9 +846,9 @@ impl X11WindowStatePtr {
if let PlatformInput::KeyDown(event) = input { if let PlatformInput::KeyDown(event) = input {
let mut state = self.state.borrow_mut(); let mut state = self.state.borrow_mut();
if let Some(mut input_handler) = state.input_handler.take() { if let Some(mut input_handler) = state.input_handler.take() {
if let Some(ime_key) = &event.keystroke.ime_key { if let Some(key_char) = &event.keystroke.key_char {
drop(state); drop(state);
input_handler.replace_text_in_range(None, ime_key); input_handler.replace_text_in_range(None, key_char);
state = self.state.borrow_mut(); state = self.state.borrow_mut();
} }
state.input_handler = Some(input_handler); state.input_handler = Some(input_handler);

View file

@ -245,7 +245,7 @@ unsafe fn parse_keystroke(native_event: id) -> Keystroke {
.charactersIgnoringModifiers() .charactersIgnoringModifiers()
.to_str() .to_str()
.to_string(); .to_string();
let mut ime_key = None; let mut key_char = None;
let first_char = characters.chars().next().map(|ch| ch as u16); let first_char = characters.chars().next().map(|ch| ch as u16);
let modifiers = native_event.modifierFlags(); let modifiers = native_event.modifierFlags();
@ -261,13 +261,19 @@ unsafe fn parse_keystroke(native_event: id) -> Keystroke {
#[allow(non_upper_case_globals)] #[allow(non_upper_case_globals)]
let key = match first_char { let key = match first_char {
Some(SPACE_KEY) => { Some(SPACE_KEY) => {
ime_key = Some(" ".to_string()); key_char = Some(" ".to_string());
"space".to_string() "space".to_string()
} }
Some(TAB_KEY) => {
key_char = Some("\t".to_string());
"tab".to_string()
}
Some(ENTER_KEY) | Some(NUMPAD_ENTER_KEY) => {
key_char = Some("\n".to_string());
"enter".to_string()
}
Some(BACKSPACE_KEY) => "backspace".to_string(), Some(BACKSPACE_KEY) => "backspace".to_string(),
Some(ENTER_KEY) | Some(NUMPAD_ENTER_KEY) => "enter".to_string(),
Some(ESCAPE_KEY) => "escape".to_string(), Some(ESCAPE_KEY) => "escape".to_string(),
Some(TAB_KEY) => "tab".to_string(),
Some(SHIFT_TAB_KEY) => "tab".to_string(), Some(SHIFT_TAB_KEY) => "tab".to_string(),
Some(NSUpArrowFunctionKey) => "up".to_string(), Some(NSUpArrowFunctionKey) => "up".to_string(),
Some(NSDownArrowFunctionKey) => "down".to_string(), Some(NSDownArrowFunctionKey) => "down".to_string(),
@ -348,7 +354,7 @@ unsafe fn parse_keystroke(native_event: id) -> Keystroke {
chars_ignoring_modifiers chars_ignoring_modifiers
}; };
if always_use_cmd_layout || alt { if !control && !command && !function {
let mut mods = NO_MOD; let mut mods = NO_MOD;
if shift { if shift {
mods |= SHIFT_MOD; mods |= SHIFT_MOD;
@ -356,11 +362,9 @@ unsafe fn parse_keystroke(native_event: id) -> Keystroke {
if alt { if alt {
mods |= OPTION_MOD; mods |= OPTION_MOD;
} }
let alt_key = chars_for_modified_key(native_event.keyCode(), mods);
if alt_key != key { key_char = Some(chars_for_modified_key(native_event.keyCode(), mods));
ime_key = Some(alt_key); }
}
};
key key
} }
@ -375,7 +379,7 @@ unsafe fn parse_keystroke(native_event: id) -> Keystroke {
function, function,
}, },
key, key,
ime_key, key_char,
} }
} }

View file

@ -1283,18 +1283,17 @@ extern "C" fn handle_key_event(this: &Object, native_event: id, key_equivalent:
} }
if event.is_held { if event.is_held {
let handled = with_input_handler(&this, |input_handler| { if let Some(key_char) = event.keystroke.key_char.as_ref() {
if !input_handler.apple_press_and_hold_enabled() { let handled = with_input_handler(&this, |input_handler| {
input_handler.replace_text_in_range( if !input_handler.apple_press_and_hold_enabled() {
None, input_handler.replace_text_in_range(None, &key_char);
&event.keystroke.ime_key.unwrap_or(event.keystroke.key), return YES;
); }
NO
});
if handled == Some(YES) {
return YES; return YES;
} }
NO
});
if handled == Some(YES) {
return YES;
} }
} }
@ -1437,7 +1436,7 @@ extern "C" fn cancel_operation(this: &Object, _sel: Sel, _sender: id) {
let keystroke = Keystroke { let keystroke = Keystroke {
modifiers: Default::default(), modifiers: Default::default(),
key: ".".into(), key: ".".into(),
ime_key: None, key_char: None,
}; };
let event = PlatformInput::KeyDown(KeyDownEvent { let event = PlatformInput::KeyDown(KeyDownEvent {
keystroke: keystroke.clone(), keystroke: keystroke.clone(),

View file

@ -386,7 +386,7 @@ fn handle_char_msg(
return Some(1); return Some(1);
}; };
drop(lock); drop(lock);
let ime_key = keystroke.ime_key.clone(); let key_char = keystroke.key_char.clone();
let event = KeyDownEvent { let event = KeyDownEvent {
keystroke, keystroke,
is_held: lparam.0 & (0x1 << 30) > 0, is_held: lparam.0 & (0x1 << 30) > 0,
@ -397,7 +397,7 @@ fn handle_char_msg(
if dispatch_event_result.default_prevented || !dispatch_event_result.propagate { if dispatch_event_result.default_prevented || !dispatch_event_result.propagate {
return Some(0); return Some(0);
} }
let Some(ime_char) = ime_key else { let Some(ime_char) = key_char else {
return Some(1); return Some(1);
}; };
with_input_handler(&state_ptr, |input_handler| { with_input_handler(&state_ptr, |input_handler| {
@ -1172,7 +1172,7 @@ fn parse_syskeydown_msg_keystroke(wparam: WPARAM) -> Option<Keystroke> {
Some(Keystroke { Some(Keystroke {
modifiers, modifiers,
key, key,
ime_key: None, key_char: None,
}) })
} }
@ -1220,7 +1220,7 @@ fn parse_keydown_msg_keystroke(wparam: WPARAM) -> Option<KeystrokeOrModifier> {
return Some(KeystrokeOrModifier::Keystroke(Keystroke { return Some(KeystrokeOrModifier::Keystroke(Keystroke {
modifiers, modifiers,
key: format!("f{}", offset + 1), key: format!("f{}", offset + 1),
ime_key: None, key_char: None,
})); }));
}; };
return None; return None;
@ -1231,7 +1231,7 @@ fn parse_keydown_msg_keystroke(wparam: WPARAM) -> Option<KeystrokeOrModifier> {
Some(KeystrokeOrModifier::Keystroke(Keystroke { Some(KeystrokeOrModifier::Keystroke(Keystroke {
modifiers, modifiers,
key, key,
ime_key: None, key_char: None,
})) }))
} }
@ -1253,7 +1253,7 @@ fn parse_char_msg_keystroke(wparam: WPARAM) -> Option<Keystroke> {
Some(Keystroke { Some(Keystroke {
modifiers, modifiers,
key, key,
ime_key: Some(first_char.to_string()), key_char: Some(first_char.to_string()),
}) })
} }
} }
@ -1327,7 +1327,7 @@ fn basic_vkcode_to_string(code: u16, modifiers: Modifiers) -> Option<Keystroke>
Some(Keystroke { Some(Keystroke {
modifiers, modifiers,
key, key,
ime_key: None, key_char: None,
}) })
} }

View file

@ -3038,7 +3038,7 @@ impl<'a> WindowContext<'a> {
return true; return true;
} }
if let Some(input) = keystroke.with_simulated_ime().ime_key { if let Some(input) = keystroke.key_char {
if let Some(mut input_handler) = self.window.platform_window.take_input_handler() { if let Some(mut input_handler) = self.window.platform_window.take_input_handler() {
input_handler.dispatch_input(&input, self); input_handler.dispatch_input(&input, self);
self.window.platform_window.set_input_handler(input_handler); self.window.platform_window.set_input_handler(input_handler);
@ -3267,7 +3267,7 @@ impl<'a> WindowContext<'a> {
if let Some(key) = key { if let Some(key) = key {
keystroke = Some(Keystroke { keystroke = Some(Keystroke {
key: key.to_string(), key: key.to_string(),
ime_key: None, key_char: None,
modifiers: Modifiers::default(), modifiers: Modifiers::default(),
}); });
} }
@ -3482,13 +3482,7 @@ impl<'a> WindowContext<'a> {
if !self.propagate_event { if !self.propagate_event {
continue 'replay; continue 'replay;
} }
if let Some(input) = replay if let Some(input) = replay.keystroke.key_char.as_ref().cloned() {
.keystroke
.with_simulated_ime()
.ime_key
.as_ref()
.cloned()
{
if let Some(mut input_handler) = self.window.platform_window.take_input_handler() { if let Some(mut input_handler) = self.window.platform_window.take_input_handler() {
input_handler.dispatch_input(&input, self); input_handler.dispatch_input(&input, self);
self.window.platform_window.set_input_handler(input_handler) self.window.platform_window.set_input_handler(input_handler)

View file

@ -206,7 +206,7 @@ fn render_markdown_list_item(
let secondary_modifier = Keystroke { let secondary_modifier = Keystroke {
key: "".to_string(), key: "".to_string(),
modifiers: Modifiers::secondary_key(), modifiers: Modifiers::secondary_key(),
ime_key: None, key_char: None,
}; };
Tooltip::text( Tooltip::text(
format!("{}-click to toggle the checkbox", secondary_modifier), format!("{}-click to toggle the checkbox", secondary_modifier),

View file

@ -343,7 +343,7 @@ mod test {
function: false, function: false,
}, },
key: "🖖🏻".to_string(), //2 char string key: "🖖🏻".to_string(), //2 char string
ime_key: None, key_char: None,
}; };
assert_eq!(to_esc_str(&ks, &TermMode::NONE, false), None); assert_eq!(to_esc_str(&ks, &TermMode::NONE, false), None);
} }

View file

@ -83,7 +83,7 @@ impl Vim {
cx: &mut ViewContext<Self>, cx: &mut ViewContext<Self>,
) { ) {
// handled by handle_literal_input // handled by handle_literal_input
if keystroke_event.keystroke.ime_key.is_some() { if keystroke_event.keystroke.key_char.is_some() {
return; return;
}; };