fix: Fix some mappings involving shift (#2018)

* Include shift in combination with ascii alpha characters

* Convert shifted ascii alphas to uppercase

* codestyle: fix typos

---------

Co-authored-by: MultisampledNight <contact@multisamplednight.com>

src/window/keyboard_manager.rs

@@ -11,6 +11,10 @@ use winit::{
     keyboard::{Key, KeyCode, KeyLocation},
 };
 
+fn is_ascii_alphabetic_char(text: &str) -> bool {
+    text.len() == 1 && text.chars().next().unwrap().is_ascii_alphabetic()
+}
+
 pub struct KeyboardManager {
     modifiers: Modifiers,
     ime_preedit: (String, Option<(usize, usize)>),
@@ -148,47 +152,58 @@ impl KeyboardManager {
     }
 
     fn format_key_text(&self, text: &str, is_special: bool) -> String {
-        let modifiers = self.format_modifier_string(is_special);
+        // Neovim always converts shifted ascii alpha characters to uppercase, so do it here already
+        // This fixes some bugs where winit does not report the uppercase text as it should
+        let text = if self.modifiers.state().shift_key() && is_ascii_alphabetic_char(text) {
+            text.to_uppercase()
+        } else {
+            text.to_string()
+        };
+
+        let modifiers = self.format_modifier_string(&text, is_special);
         // < needs to be formatted as a special character, but note that it's not treated as a
         // special key for the modifier formatting, so S- and -M are still potentially stripped
         let (text, is_special) = if text == "<" {
-            ("lt", true)
+            ("lt".to_string(), true)
         } else {
             (text, is_special)
         };
         if modifiers.is_empty() {
             if is_special {
                 format!("<{text}>")
             } else {
-                text.to_string()
+                text
             }
         } else {
             format!("<{modifiers}{text}>")
         }
     }
 
-    pub fn format_modifier_string(&self, is_special: bool) -> String {
-        // Is special is used for special keys so that all modifiers are always included
-        // It's also true with alt_is_meta is set to true.
-        // When the key is not special, shift is removed, since the base character is already
-        // shifted. Furthermore on macOS, meta is additionally removed when alt_is_meta is set to false
-        let shift = or_empty(self.modifiers.state().shift_key() && is_special, "S-");
-        let ctrl = or_empty(self.modifiers.state().control_key(), "C-");
-        let alt = or_empty(
-            self.modifiers.state().alt_key() && (use_alt() || is_special),
-            "M-",
-        );
-        let logo = or_empty(self.modifiers.state().super_key(), "D-");
+    pub fn format_modifier_string(&self, text: &str, is_special: bool) -> String {
+        // Shift should always be sent together with special keys (Enter, Space, F keys and so on).
+        // And as a special case togeter with CTRL and standard a-z characters.
+        // In all other cases the resulting character is enough.
+        // Note that, in Neovim <C-a> and <C-A> are the same, but <C-S-A> is different.
+        // Actually, <C-S-a> is the same as <C-S-A>, since Neovim converts all shifted
+        // lowercase alphas to uppercase internally in its mappings.
+        // Also note that mappings that do not include CTRL work differently, they are always
+        // normalized in combination with ascii alphas. For example <M-S-a> is normalized to
+        // uppercase without shift, or <M-A> .
+        // But in combination with other characters, such as <M-S-$> they are not,
+        // so we don't want to send shift when that's the case.
+        let include_shift =
+            is_special || (self.modifiers.state().control_key() && is_ascii_alphabetic_char(text));
 
-        shift.to_owned() + ctrl + alt + logo
-    }
-}
+        // Always send meta (alt) together with special keys, or when alt is meta on macOS
+        let include_alt = use_alt() || is_special;
 
-fn or_empty(condition: bool, text: &str) -> &str {
-    if condition {
-        text
-    } else {
-        ""
+        let state = self.modifiers.state();
+        let mut ret = String::new();
+        (state.shift_key() && include_shift).then(|| ret += "S-");
+        state.control_key().then(|| ret += "C-");
+        (state.alt_key() && include_alt).then(|| ret += "M-");
+        state.super_key().then(|| ret += "D-");
+        ret
     }
 }
 

src/window/mouse_manager.rs

@@ -178,7 +178,7 @@ impl MouseManager {
                     button: self.dragging.as_ref().unwrap().to_owned(),
                     grid_id: relevant_window_details.id,
                     position: self.drag_position.into(),
-                    modifier_string: keyboard_manager.format_modifier_string(true),
+                    modifier_string: keyboard_manager.format_modifier_string("", true),
                 }));
             } else {
                 // otherwise, update the window_id_under_mouse to match the one selected
@@ -191,7 +191,7 @@ impl MouseManager {
                     action: "".into(), // this is ignored by nvim
                     grid_id: relevant_window_details.id,
                     position: self.relative_position.into(),
-                    modifier_string: keyboard_manager.format_modifier_string(true),
+                    modifier_string: keyboard_manager.format_modifier_string("", true),
                 }))
             }
 
@@ -228,7 +228,7 @@ impl MouseManager {
                         action,
                         grid_id: details.id,
                         position: position.into(),
-                        modifier_string: keyboard_manager.format_modifier_string(true),
+                        modifier_string: keyboard_manager.format_modifier_string("", true),
                     }));
                 }
 
@@ -269,7 +269,7 @@ impl MouseManager {
                     .map(|details| details.id)
                     .unwrap_or(0),
                 position: self.drag_position.into(),
-                modifier_string: keyboard_manager.format_modifier_string(true),
+                modifier_string: keyboard_manager.format_modifier_string("", true),
             }
             .into();
             for _ in 0..(new_y - previous_y).abs() {
@@ -296,7 +296,7 @@ impl MouseManager {
                     .map(|details| details.id)
                     .unwrap_or(0),
                 position: self.drag_position.into(),
-                modifier_string: keyboard_manager.format_modifier_string(true),
+                modifier_string: keyboard_manager.format_modifier_string("", true),
             }
             .into();
             for _ in 0..(new_x - previous_x).abs() {