From 9faf4b44ff12af00c26763667fdf41bf64a7c060 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Fri, 5 Jan 2024 10:53:14 +0100 Subject: [PATCH] Ignore extra SHIFT and ALT when matching modifiers (#3769) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Closes https://github.com/emilk/egui/issues/3626 Basically, egui now ignores extra SHIFT and ALT pressed when matching keyboard shortcuts. This is because SHIFT and ALT are often requires to produce some logical keys. For instance, typing `+` on an English keyboard requires pressing `SHIFT =`, so the keyboard shortcut looking for `CTRL +` should ignore the SHIFT key. @abey79 You reported problem using `Cmd +` and `Cmd -` to zoom - does this fix it for you? You can run with `RUST_LOG=egui_winit=trace cargo run` to see a printout of how winit reports the logical and physical keys, and how egui interprets them. Weirdly, on Mac winit reports `SHIFT =` as `+`, but `CMD SHIFT =` as `=` (on an English keyboard) so things are… difficult. --- crates/egui-winit/src/lib.rs | 14 ++- crates/egui/src/data/input.rs | 110 ++++++++++++++++--- crates/egui/src/gui_zoom.rs | 21 +++- crates/egui/src/input_state.rs | 21 +++- crates/egui/src/widgets/text_edit/builder.rs | 7 +- 5 files changed, 144 insertions(+), 29 deletions(-) diff --git a/crates/egui-winit/src/lib.rs b/crates/egui-winit/src/lib.rs index 607afdbac2b..c7123648b1b 100644 --- a/crates/egui-winit/src/lib.rs +++ b/crates/egui-winit/src/lib.rs @@ -689,6 +689,15 @@ impl State { let logical_key = key_from_winit_key(logical_key); + // Helpful logging to enable when adding new key support + log::trace!( + "logical {:?} -> {:?}, physical {:?} -> {:?}", + event.logical_key, + logical_key, + event.physical_key, + physical_key + ); + if let Some(logical_key) = logical_key { if pressed { if is_cut_command(self.egui_input.modifiers, logical_key) { @@ -1064,9 +1073,8 @@ fn key_from_key_code(key: winit::keyboard::KeyCode) -> Option { KeyCode::Minus | KeyCode::NumpadSubtract => Key::Minus, - // Using Mac the key with the Plus sign on it is reported as the Equals key - // (with both English and Swedish keyboard). - KeyCode::Equal | KeyCode::NumpadAdd => Key::PlusEquals, + KeyCode::NumpadAdd => Key::Plus, + KeyCode::Equal => Key::Equals, KeyCode::Digit0 | KeyCode::Numpad0 => Key::Num0, KeyCode::Digit1 | KeyCode::Numpad1 => Key::Num1, diff --git a/crates/egui/src/data/input.rs b/crates/egui/src/data/input.rs index 71b77d97020..451e7f4e6da 100644 --- a/crates/egui/src/data/input.rs +++ b/crates/egui/src/data/input.rs @@ -655,13 +655,68 @@ impl Modifiers { !self.alt && !self.shift && self.command } + /// Checks that the `ctrl/cmd` matches, and that the `shift/alt` of the argument is a subset + /// of the pressed ksey (`self`). + /// + /// This means that if the pattern has not set `shift`, then `self` can have `shift` set or not. + /// + /// The reason is that many logical keys require `shift` or `alt` on some keyboard layouts. + /// For instance, in order to press `+` on an English keyboard, you need to press `shift` and `=`, + /// but a Swedish keyboard has dedicated `+` key. + /// So if you want to make a [`KeyboardShortcut`] looking for `Cmd` + `+`, it makes sense + /// to ignore the shift key. + /// Similarly, the `Alt` key is sometimes used to type special characters. + /// + /// However, if the pattern (the argument) explicitly requires the `shift` or `alt` keys + /// to be pressed, then they must be pressed. + /// + /// # Example: + /// ``` + /// # use egui::Modifiers; + /// # let pressed_modifiers = Modifiers::default(); + /// if pressed_modifiers.matches(Modifiers::ALT | Modifiers::SHIFT) { + /// // Alt and Shift are pressed, and nothing else + /// } + /// ``` + /// + /// ## Behavior: + /// ``` + /// # use egui::Modifiers; + /// assert!(Modifiers::CTRL.matches_logically(Modifiers::CTRL)); + /// assert!(!Modifiers::CTRL.matches_logically(Modifiers::CTRL | Modifiers::SHIFT)); + /// assert!((Modifiers::CTRL | Modifiers::SHIFT).matches_logically(Modifiers::CTRL)); + /// assert!((Modifiers::CTRL | Modifiers::COMMAND).matches_logically(Modifiers::CTRL)); + /// assert!((Modifiers::CTRL | Modifiers::COMMAND).matches_logically(Modifiers::COMMAND)); + /// assert!((Modifiers::MAC_CMD | Modifiers::COMMAND).matches_logically(Modifiers::COMMAND)); + /// assert!(!Modifiers::COMMAND.matches_logically(Modifiers::MAC_CMD)); + /// ``` + pub fn matches_logically(&self, pattern: Modifiers) -> bool { + if pattern.alt && !self.alt { + return false; + } + if pattern.shift && !self.shift { + return false; + } + + self.cmd_ctrl_matches(pattern) + } + /// Check for equality but with proper handling of [`Self::command`]. /// + /// `self` here are the currently pressed modifiers, + /// and the argument the pattern we are testing for. + /// + /// Note that this will require the `shift` and `alt` keys match, even though + /// these modifiers are sometimes required to produce some logical keys. + /// For instance, to press `+` on an English keyboard, you need to press `shift` and `=`, + /// but on a Swedish keyboard you can press the dedicated `+` key. + /// Therefore, you often want to use [`Self::matches_logically`] instead. + /// /// # Example: /// ``` /// # use egui::Modifiers; - /// # let current_modifiers = Modifiers::default(); - /// if current_modifiers.matches(Modifiers::ALT | Modifiers::SHIFT) { + /// # let pressed_modifiers = Modifiers::default(); + /// if pressed_modifiers.matches(Modifiers::ALT | Modifiers::SHIFT) { /// // Alt and Shift are pressed, and nothing else /// } /// ``` @@ -677,12 +732,28 @@ impl Modifiers { /// assert!((Modifiers::MAC_CMD | Modifiers::COMMAND).matches(Modifiers::COMMAND)); /// assert!(!Modifiers::COMMAND.matches(Modifiers::MAC_CMD)); /// ``` - pub fn matches(&self, pattern: Modifiers) -> bool { + pub fn matches_exact(&self, pattern: Modifiers) -> bool { // alt and shift must always match the pattern: if pattern.alt != self.alt || pattern.shift != self.shift { return false; } + self.cmd_ctrl_matches(pattern) + } + + #[deprecated = "Renamed `matches_exact`, but maybe you want to use `matches_logically` instead"] + pub fn matches(&self, pattern: Modifiers) -> bool { + self.matches_exact(pattern) + } + + /// Checks only cmd/ctrl, not alt/shift. + /// + /// `self` here are the currently pressed modifiers, + /// and the argument the pattern we are testing for. + /// + /// This takes care to properly handle the difference between + /// [`Self::ctrl`], [`Self::command`] and [`Self::mac_cmd`]. + pub fn cmd_ctrl_matches(&self, pattern: Modifiers) -> bool { if pattern.mac_cmd { // Mac-specific match: if !self.mac_cmd { @@ -897,8 +968,11 @@ pub enum Key { /// `.` Period, - /// The for the Plus/Equals key. - PlusEquals, + /// `+` + Plus, + + /// `=` + Equals, /// `;` Semicolon, @@ -1017,7 +1091,8 @@ impl Key { Self::Comma, Self::Minus, Self::Period, - Self::PlusEquals, + Self::Plus, + Self::Equals, Self::Semicolon, // Digits: Self::Num0, @@ -1117,7 +1192,8 @@ impl Key { "Comma" | "," => Self::Comma, "Minus" | "-" | "−" => Self::Minus, "Period" | "." => Self::Period, - "Plus" | "+" | "Equals" | "=" => Self::PlusEquals, + "Plus" | "+" => Self::Plus, + "Equals" | "=" => Self::Equals, "Semicolon" | ";" => Self::Semicolon, "0" => Self::Num0, @@ -1194,7 +1270,8 @@ impl Key { Key::ArrowRight => "⏵", Key::ArrowUp => "⏶", Key::Minus => crate::MINUS_CHAR_STR, - Key::PlusEquals => "+", + Key::Plus => "+", + Key::Equals => "=", _ => self.name(), } } @@ -1228,7 +1305,8 @@ impl Key { Key::Comma => "Comma", Key::Minus => "Minus", Key::Period => "Period", - Key::PlusEquals => "Plus", + Key::Plus => "Plus", + Key::Equals => "Equals", Key::Semicolon => "Semicolon", Key::Num0 => "0", @@ -1327,12 +1405,16 @@ fn test_key_from_name() { #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] pub struct KeyboardShortcut { pub modifiers: Modifiers, - pub key: Key, + + pub logical_key: Key, } impl KeyboardShortcut { - pub const fn new(modifiers: Modifiers, key: Key) -> Self { - Self { modifiers, key } + pub const fn new(modifiers: Modifiers, logical_key: Key) -> Self { + Self { + modifiers, + logical_key, + } } pub fn format(&self, names: &ModifierNames<'_>, is_mac: bool) -> String { @@ -1341,9 +1423,9 @@ impl KeyboardShortcut { s += names.concat; } if names.is_short { - s += self.key.symbol_or_name(); + s += self.logical_key.symbol_or_name(); } else { - s += self.key.name(); + s += self.logical_key.name(); } s } diff --git a/crates/egui/src/gui_zoom.rs b/crates/egui/src/gui_zoom.rs index fb4f9bd11ed..c4b5deb5ed6 100644 --- a/crates/egui/src/gui_zoom.rs +++ b/crates/egui/src/gui_zoom.rs @@ -6,9 +6,22 @@ use crate::*; pub mod kb_shortcuts { use super::*; - pub const ZOOM_IN: KeyboardShortcut = - KeyboardShortcut::new(Modifiers::COMMAND, Key::PlusEquals); + /// Primary keyboard shortcut for zooming in (`Cmd` + `+`). + pub const ZOOM_IN: KeyboardShortcut = KeyboardShortcut::new(Modifiers::COMMAND, Key::Plus); + + /// Secondary keyboard shortcut for zooming in (`Cmd` + `=`). + /// + /// On an English keyboard `+` is `Shift` + `=`, + /// but it is annoying to have to press shift. + /// So most browsers also allow `Cmd` + `=` for zooming in. + /// We do the same. + pub const ZOOM_IN_SECONDARY: KeyboardShortcut = + KeyboardShortcut::new(Modifiers::COMMAND, Key::Equals); + + /// Keyboard shortcut for zooming in (`Cmd` + `-`). pub const ZOOM_OUT: KeyboardShortcut = KeyboardShortcut::new(Modifiers::COMMAND, Key::Minus); + + /// Keyboard shortcut for resetting zoom in (`Cmd` + `0`). pub const ZOOM_RESET: KeyboardShortcut = KeyboardShortcut::new(Modifiers::COMMAND, Key::Num0); } @@ -21,7 +34,9 @@ pub(crate) fn zoom_with_keyboard(ctx: &Context) { if ctx.input_mut(|i| i.consume_shortcut(&kb_shortcuts::ZOOM_RESET)) { ctx.set_zoom_factor(1.0); } else { - if ctx.input_mut(|i| i.consume_shortcut(&kb_shortcuts::ZOOM_IN)) { + if ctx.input_mut(|i| i.consume_shortcut(&kb_shortcuts::ZOOM_IN)) + || ctx.input_mut(|i| i.consume_shortcut(&kb_shortcuts::ZOOM_IN_SECONDARY)) + { zoom_in(ctx); } if ctx.input_mut(|i| i.consume_shortcut(&kb_shortcuts::ZOOM_OUT)) { diff --git a/crates/egui/src/input_state.rs b/crates/egui/src/input_state.rs index 693f6c56ee2..b95303ec819 100644 --- a/crates/egui/src/input_state.rs +++ b/crates/egui/src/input_state.rs @@ -289,7 +289,13 @@ impl InputState { /// Count presses of a key. If non-zero, the presses are consumed, so that this will only return non-zero once. /// /// Includes key-repeat events. - pub fn count_and_consume_key(&mut self, modifiers: Modifiers, key: Key) -> usize { + /// + /// This uses [`Modifiers::matches_logically`] to match modifiers. + /// This means that e.g. the shortcut `Ctrl` + `Key::Plus` will be matched + /// as long as `Ctrl` and `Plus` are pressed, ignoring if + /// `Shift` or `Alt` are also pressed (because those modifiers might + /// be required to produce the logical `Key::Plus`). + pub fn count_and_consume_key(&mut self, modifiers: Modifiers, logical_key: Key) -> usize { let mut count = 0usize; self.events.retain(|event| { @@ -300,7 +306,7 @@ impl InputState { modifiers: ev_mods, pressed: true, .. - } if *ev_key == key && ev_mods.matches(modifiers) + } if *ev_key == logical_key && ev_mods.matches_logically(modifiers) ); count += is_match as usize; @@ -314,8 +320,8 @@ impl InputState { /// Check for a key press. If found, `true` is returned and the key pressed is consumed, so that this will only return `true` once. /// /// Includes key-repeat events. - pub fn consume_key(&mut self, modifiers: Modifiers, key: Key) -> bool { - self.count_and_consume_key(modifiers, key) > 0 + pub fn consume_key(&mut self, modifiers: Modifiers, logical_key: Key) -> bool { + self.count_and_consume_key(modifiers, logical_key) > 0 } /// Check if the given shortcut has been pressed. @@ -324,8 +330,11 @@ impl InputState { /// /// Includes key-repeat events. pub fn consume_shortcut(&mut self, shortcut: &KeyboardShortcut) -> bool { - let KeyboardShortcut { modifiers, key } = *shortcut; - self.consume_key(modifiers, key) + let KeyboardShortcut { + modifiers, + logical_key, + } = *shortcut; + self.consume_key(modifiers, logical_key) } /// Was the given key pressed this frame? diff --git a/crates/egui/src/widgets/text_edit/builder.rs b/crates/egui/src/widgets/text_edit/builder.rs index 1a2243a92e7..ded1f335d57 100644 --- a/crates/egui/src/widgets/text_edit/builder.rs +++ b/crates/egui/src/widgets/text_edit/builder.rs @@ -990,7 +990,7 @@ fn events( pressed: true, modifiers, .. - } if modifiers.matches(Modifiers::COMMAND) => { + } if modifiers.matches_logically(Modifiers::COMMAND) => { if let Some((undo_ccursor_range, undo_txt)) = state .undoer .lock() @@ -1007,8 +1007,9 @@ fn events( pressed: true, modifiers, .. - } if (modifiers.matches(Modifiers::COMMAND) && *key == Key::Y) - || (modifiers.matches(Modifiers::SHIFT | Modifiers::COMMAND) && *key == Key::Z) => + } if (modifiers.matches_logically(Modifiers::COMMAND) && *key == Key::Y) + || (modifiers.matches_logically(Modifiers::SHIFT | Modifiers::COMMAND) + && *key == Key::Z) => { if let Some((redo_ccursor_range, redo_txt)) = state .undoer