diff --git a/crates/poltertype-core/src/engine/switcher/correction.rs b/crates/poltertype-core/src/engine/switcher/correction.rs index 8b3a978f..646c3396 100644 --- a/crates/poltertype-core/src/engine/switcher/correction.rs +++ b/crates/poltertype-core/src/engine/switcher/correction.rs @@ -1054,13 +1054,37 @@ impl SwitcherEngine { /// The selection re-rendered under another layout, or `None` when /// it is not wrong-layout text at all. - fn converted(&self, text: &str) -> Option<(String, LayoutId, LayoutId)> { - let from = self.layout_switcher.current().ok()?; - let to = self.next_layout_after(&from)?; - let source = self.layouts.get(&from)?; - let target = self.layouts.get(&to)?; + /// + /// Which of the current layout and its neighbour is the *source* + /// is not something `current()` can answer on its own: the text + /// may have been typed before the very next word moved the layout + /// on, and then a source guess keyed off `current()` names the + /// layout the caret is in now, not the one the selection was + /// typed under. So both directions are tried, current-first, and + /// `transliterate_to`'s own guard — no letter of that layout in + /// the text — is what rejects the one that does not fit; neither + /// is forced through blind. + // `pub(in crate::engine)`, not `pub(super)`: the regression test + // for the direction bug below builds a bare engine directly in + // `engine::tests` rather than driving the run loop, and that + // module sits one level above `switcher`. + pub(in crate::engine) fn converted(&self, text: &str) -> Option<(String, LayoutId, LayoutId)> { + let current = self.layout_switcher.current().ok()?; + let other = self.next_layout_after(¤t)?; + self.transliterated(text, ¤t, &other) + .or_else(|| self.transliterated(text, &other, ¤t)) + } + + fn transliterated( + &self, + text: &str, + from: &LayoutId, + to: &LayoutId, + ) -> Option<(String, LayoutId, LayoutId)> { + let source = self.layouts.get(from)?; + let target = self.layouts.get(to)?; let converted = source.transliterate_to(text, target)?; - Some((converted, from, to)) + Some((converted, from.clone(), to.clone())) } /// Put back what the user had, best effort. A failure here is worth diff --git a/crates/poltertype-core/src/engine/tests.rs b/crates/poltertype-core/src/engine/tests.rs index 202abbbd..0b09c763 100644 --- a/crates/poltertype-core/src/engine/tests.rs +++ b/crates/poltertype-core/src/engine/tests.rs @@ -938,6 +938,101 @@ mod engine_integration_tests { ); } + /// Builds a bare `SwitcherEngine` for calling a private method + /// directly — no thread, no channels driven, unlike `Harness`, + /// which moves the engine into its runner thread and never gives + /// it back. Only what `converted()` touches needs to be real. + fn bare_engine(current: &str, active: &[&str]) -> (SwitcherEngine, Arc) { + let active_ids: Vec = active.iter().map(|s| LayoutId::from(*s)).collect(); + let layouts = Arc::new( + LayoutDb::load(crate::layouts::LoadOptions { + active_filter: Some(&active_ids), + ..Default::default() + }) + .expect("bundled layouts load"), + ); + let settings = Arc::new(SettingsStore::for_tests( + crate::settings::Settings::default(), + )); + let switcher = Arc::new(MockSwitcher::new(current, active)); + let (audio, _audio_rx) = crate::audio::AudioPlayer::for_tests(); + let (out_tx, _out_rx) = crossbeam_channel::unbounded(); + let engine = SwitcherEngine::new(EngineDeps { + settings, + layouts: Arc::clone(&layouts), + detectors: Vec::new(), + layout_switcher: switcher as Arc, + key_emitter: Arc::new(MockEmitter::default()) as Arc, + clipboard: None, + key_gate: poltertype_input::KeyGate::disabled(), + focus_tracker: Arc::new(NoopFocusTracker), + audio: Arc::new(audio), + out_tx, + suggester: None, + }); + (engine, layouts) + } + + /// Regression: force-by-selection trusted `current()` for the + /// *source* layout, not the text. If the layout had moved on since + /// the word was typed — exactly what the very next word does when + /// it triggers auto-correction — `converted()` tried + /// transliterating from the wrong layout, `transliterate_to`'s own + /// guard refused (no letter of that layout in the text), and the + /// whole gesture read as "nothing was selected". Plan and test + /// words are from `plans/backlog.md` (P2, keyboard-switcher track). + #[test] + fn selection_conversion_direction_follows_the_text_not_the_current_layout() { + let (engine, layouts) = bare_engine("uk-UA", &["en-US", "uk-UA"]); + let en = layouts.get(&LayoutId::from("en-US")).expect("en-US loaded"); + let uk = layouts.get(&LayoutId::from("uk-UA")).expect("uk-UA loaded"); + let expected = en + .transliterate_to("ghbdsn", uk) + .expect("en-US -> uk-UA must convert `ghbdsn`"); + + // `current()` says uk-UA, but "ghbdsn" (`привіт` mistyped) was + // typed under en-US — the old code trusted `current()` and + // the force-switch silently did nothing. + assert_eq!( + engine.converted("ghbdsn"), + Some((expected, LayoutId::from("en-US"), LayoutId::from("uk-UA"))), + "must detect en-US as the source from the text, not uk-UA from current()" + ); + } + + /// Mirror of the regression above: `current()` already names the + /// right source, so the first try must still succeed directly — + /// the fallback exists for the case above, not to replace the + /// ordinary one. + #[test] + fn selection_conversion_direction_still_works_when_current_is_right() { + let (engine, layouts) = bare_engine("en-US", &["en-US", "uk-UA"]); + let en = layouts.get(&LayoutId::from("en-US")).expect("en-US loaded"); + let uk = layouts.get(&LayoutId::from("uk-UA")).expect("uk-UA loaded"); + let expected = en + .transliterate_to("ghbdsn", uk) + .expect("en-US -> uk-UA must convert `ghbdsn`"); + + assert_eq!( + engine.converted("ghbdsn"), + Some((expected, LayoutId::from("en-US"), LayoutId::from("uk-UA"))), + "a source that `current()` already gets right must not need the fallback" + ); + } + + /// Text that belongs to neither active layout must not be forced + /// through either direction — `transliterate_to`'s guard has to + /// win both tries, not just the first. + #[test] + fn selection_conversion_direction_gives_up_on_text_neither_layout_typed() { + let (engine, _layouts) = bare_engine("uk-UA", &["en-US", "uk-UA"]); + assert_eq!( + engine.converted("12345"), + None, + "digits belong to no layout's alphabet — nothing to convert" + ); + } + /// A word typed under a latched Caps Lock has to go back out on the /// Shift states the user's fingers actually had. /// diff --git a/crates/poltertype-input/src/windows/listener.rs b/crates/poltertype-input/src/windows/listener.rs index b29972f7..d2528313 100644 --- a/crates/poltertype-input/src/windows/listener.rs +++ b/crates/poltertype-input/src/windows/listener.rs @@ -15,10 +15,6 @@ use std::sync::Arc; use crossbeam_channel::Sender; use tracing::{debug, error, info, warn}; use windows::Win32::Foundation::{HMODULE, LPARAM, LRESULT, WPARAM}; -use windows::Win32::UI::Input::KeyboardAndMouse::{ - GetAsyncKeyState, GetKeyState, VK_CAPITAL, VK_CONTROL, VK_LMENU, VK_LWIN, VK_MENU, VK_RMENU, - VK_RWIN, VK_SHIFT, -}; use windows::Win32::UI::WindowsAndMessaging::{ CallNextHookEx, DispatchMessageW, GetMessageW, HC_ACTION, HHOOK, KBDLLHOOKSTRUCT, MSG, PostThreadMessageW, SetWindowsHookExW, TranslateMessage, UnhookWindowsHookEx, WH_KEYBOARD_LL, @@ -27,8 +23,9 @@ use windows::Win32::UI::WindowsAndMessaging::{ use super::consts::EMITTER_MARKER; use super::gate::WindowsGate; +use super::modifiers::modifiers_for_event; use super::types::WorkerHandle; -use crate::{InputError, InputListener, KeyDirection, KeyEvent, Modifiers}; +use crate::{InputError, InputListener, KeyDirection, KeyEvent}; /// Sender shared with the C-callable hook procedure. There can only be /// one global keyboard hook per process at a time. `parking_lot::RwLock` @@ -211,7 +208,7 @@ unsafe extern "system" fn low_level_keyboard_proc( vk: kb.vkCode, scancode: kb.scanCode, direction, - modifiers: read_modifiers(), + modifiers: modifiers_for_event(kb.vkCode, direction), injected: ours, timestamp_ms: kb.time as u64, }; @@ -246,21 +243,3 @@ unsafe extern "system" fn low_level_keyboard_proc( // ignored for WH_KEYBOARD_LL since Win XP, but we still must call). unsafe { CallNextHookEx(HHOOK(std::ptr::null_mut()), code, wparam, lparam) } } - -fn read_modifiers() -> Modifiers { - fn down(vk: u16) -> bool { - // Safety: GetAsyncKeyState is a trivial Win32 call. - unsafe { (GetAsyncKeyState(vk as i32) as u16) & 0x8000 != 0 } - } - // The low bit of `GetKeyState` is the toggle, not the held-ness — - // the only thing that answers "is Caps Lock on" without guessing. - // Safety: GetKeyState is a trivial Win32 call. - let caps = unsafe { GetKeyState(VK_CAPITAL.0 as i32) } & 1 != 0; - Modifiers { - shift: down(VK_SHIFT.0), - control: down(VK_CONTROL.0), - alt: down(VK_MENU.0) || down(VK_LMENU.0) || down(VK_RMENU.0), - meta: down(VK_LWIN.0) || down(VK_RWIN.0), - caps, - } -} diff --git a/crates/poltertype-input/src/windows/mod.rs b/crates/poltertype-input/src/windows/mod.rs index 2dfc4e6f..2277220a 100644 --- a/crates/poltertype-input/src/windows/mod.rs +++ b/crates/poltertype-input/src/windows/mod.rs @@ -17,6 +17,8 @@ mod gate; #[cfg(windows)] mod listener; #[cfg(windows)] +mod modifiers; +#[cfg(windows)] mod types; #[cfg(windows)] diff --git a/crates/poltertype-input/src/windows/modifiers.rs b/crates/poltertype-input/src/windows/modifiers.rs new file mode 100644 index 00000000..c73a3b88 --- /dev/null +++ b/crates/poltertype-input/src/windows/modifiers.rs @@ -0,0 +1,80 @@ +//! The modifier set a key event carries, read from the OS inside the +//! low-level hook — and corrected for the one thing that read gets +//! wrong there: the event being delivered has not taken effect yet. + +use poltertype_types::ModifierKey; +use windows::Win32::UI::Input::KeyboardAndMouse::{ + GetAsyncKeyState, GetKeyState, VIRTUAL_KEY, VK_CAPITAL, VK_CONTROL, VK_LCONTROL, VK_LMENU, + VK_LSHIFT, VK_LWIN, VK_MENU, VK_RCONTROL, VK_RMENU, VK_RSHIFT, VK_RWIN, VK_SHIFT, +}; + +use crate::{KeyDirection, Modifiers}; + +/// The modifier set *after* the event being delivered. +/// +/// `GetAsyncKeyState` inside `WH_KEYBOARD_LL` describes the keyboard as +/// it was before this event: on the Ctrl release it still says "Ctrl +/// held", and with nothing typed afterwards that reading is the last +/// one the engine gets. It then believes the force-switch chord is +/// still down and waits for a release it has already been handed — +/// measured on Windows Server 2025 over RDP (2026-09-03, 2026-09-08): +/// every manual switch stalled until the next keystroke, which was the +/// user pressing the hotkey again. For a modifier the event is about, +/// the event itself is the truth; the other side of the same modifier +/// is read live, so releasing one Shift while the other is held keeps +/// Shift. +pub(super) fn modifiers_for_event(vk: u32, direction: KeyDirection) -> Modifiers { + let snapshot = read_modifiers(); + let Some((key, other_side)) = modifier_key_of(vk) else { + return snapshot; + }; + let other_side_down = other_side.is_some_and(key_down); + snapshot.after_transition(key, direction == KeyDirection::Press, other_side_down) +} + +/// The modifier set as the OS reports it at the moment of the call. +/// +/// Inside a low-level hook that moment is *before* the event being +/// delivered has taken effect: a Ctrl release still reads "Ctrl held". +/// [`modifiers_for_event`] is what corrects for that; this alone is +/// only right for keys the event is not about. +fn read_modifiers() -> Modifiers { + // The low bit of `GetKeyState` is the toggle, not the held-ness — + // the only thing that answers "is Caps Lock on" without guessing. + // Safety: GetKeyState is a trivial Win32 call. + let caps = unsafe { GetKeyState(VK_CAPITAL.0 as i32) } & 1 != 0; + Modifiers { + shift: key_down(VK_SHIFT), + control: key_down(VK_CONTROL), + alt: key_down(VK_MENU) || key_down(VK_LMENU) || key_down(VK_RMENU), + meta: key_down(VK_LWIN) || key_down(VK_RWIN), + caps, + } +} + +/// Is `vk` physically down right now, as the OS sees it. +fn key_down(vk: VIRTUAL_KEY) -> bool { + // Safety: GetAsyncKeyState is a trivial Win32 call. + unsafe { (GetAsyncKeyState(vk.0 as i32) as u16) & 0x8000 != 0 } +} + +/// Which modifier a virtual key is, and the key on the other side of +/// the keyboard that holds the same modifier — `None` for the generic +/// codes injected input uses, which have no other side. +fn modifier_key_of(vk: u32) -> Option<(ModifierKey, Option)> { + let vk = VIRTUAL_KEY(u16::try_from(vk).ok()?); + Some(match vk { + VK_LSHIFT => (ModifierKey::Shift, Some(VK_RSHIFT)), + VK_RSHIFT => (ModifierKey::Shift, Some(VK_LSHIFT)), + VK_SHIFT => (ModifierKey::Shift, None), + VK_LCONTROL => (ModifierKey::Control, Some(VK_RCONTROL)), + VK_RCONTROL => (ModifierKey::Control, Some(VK_LCONTROL)), + VK_CONTROL => (ModifierKey::Control, None), + VK_LMENU => (ModifierKey::Alt, Some(VK_RMENU)), + VK_RMENU => (ModifierKey::Alt, Some(VK_LMENU)), + VK_MENU => (ModifierKey::Alt, None), + VK_LWIN => (ModifierKey::Meta, Some(VK_RWIN)), + VK_RWIN => (ModifierKey::Meta, Some(VK_LWIN)), + _ => return None, + }) +} diff --git a/crates/poltertype-types/src/enums.rs b/crates/poltertype-types/src/enums.rs index f66fbdb3..edfb82bb 100644 --- a/crates/poltertype-types/src/enums.rs +++ b/crates/poltertype-types/src/enums.rs @@ -8,6 +8,19 @@ pub enum KeyDirection { Release, } +/// One modifier as the physical key an event can be *about* — what a +/// platform names when it delivers "Left Shift went down". Distinct +/// from [`Modifiers`], the set held at a moment: this is how a snapshot +/// taken before an event is carried across it (see +/// [`Modifiers::after_transition`]). +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ModifierKey { + Shift, + Control, + Alt, + Meta, +} + /// What the engine decided to do with the just-completed word. #[derive(Debug, Clone, PartialEq)] pub enum SwitchAction { diff --git a/crates/poltertype-types/src/tests.rs b/crates/poltertype-types/src/tests.rs index 4a5c3bf9..c915f89f 100644 --- a/crates/poltertype-types/src/tests.rs +++ b/crates/poltertype-types/src/tests.rs @@ -1,4 +1,64 @@ use crate::logsafe; +use crate::{ModifierKey, Modifiers}; + +/// The bug this guards: a low-level hook that reads the keyboard as it +/// was *before* the event it is delivering reports "Ctrl held" on the +/// Ctrl release, and nothing corrects that until the next keystroke — +/// so the engine waited for a release it had already been handed. +#[test] +fn a_release_seen_through_a_pre_event_snapshot_clears_the_modifier() { + let before = Modifiers { + control: true, + ..Modifiers::NONE + }; + let after = before.after_transition(ModifierKey::Control, false, false); + assert_eq!(after, Modifiers::NONE); +} + +#[test] +fn a_press_seen_through_a_pre_event_snapshot_sets_the_modifier() { + let after = Modifiers::NONE.after_transition(ModifierKey::Shift, true, false); + assert!(after.shift); + assert!(!after.control && !after.alt && !after.meta); +} + +#[test] +fn releasing_one_side_keeps_the_modifier_while_the_other_side_is_held() { + let before = Modifiers { + shift: true, + ..Modifiers::NONE + }; + assert!( + before + .after_transition(ModifierKey::Shift, false, true) + .shift + ); + assert!( + !before + .after_transition(ModifierKey::Shift, false, false) + .shift + ); +} + +#[test] +fn the_transition_touches_only_its_own_modifier() { + let before = Modifiers { + control: true, + alt: true, + caps: true, + ..Modifiers::NONE + }; + let after = before.after_transition(ModifierKey::Meta, true, false); + assert_eq!( + after, + Modifiers { + meta: true, + ..before + } + ); + // Caps is a latch, not a held key; a modifier event never moves it. + assert!(after.caps); +} #[test] fn redaction_hides_the_word_and_keeps_its_length() { diff --git a/crates/poltertype-types/src/types.rs b/crates/poltertype-types/src/types.rs index 059082c1..8aaf3791 100644 --- a/crates/poltertype-types/src/types.rs +++ b/crates/poltertype-types/src/types.rs @@ -117,6 +117,39 @@ impl Modifiers { pub fn is_command(&self) -> bool { self.control || self.alt || self.meta } + + /// The held set once an event on `key` has taken effect, given a + /// snapshot read *before* it. + /// + /// Some platforms hand a hook the keyboard as it was before the + /// event being delivered — `GetAsyncKeyState` inside a Windows + /// low-level hook does. A Ctrl release then arrives reading "Ctrl + /// held", and with nothing typed afterwards that reading stands + /// until the next key: the engine believes the hotkey's chord is + /// still down and waits for a release it has already been handed + /// (measured on Windows Server 2025 over RDP, 2026-09-03 and + /// 2026-09-08 — every manual switch delayed until the next + /// keystroke). Applying the event's own transition makes the + /// snapshot describe the moment *after* it. + /// + /// A release clears the flag only while the same modifier's other + /// physical key is not held — `other_side_down` is that answer, + /// read by the platform for the opposite side. + pub fn after_transition( + mut self, + key: ModifierKey, + pressed: bool, + other_side_down: bool, + ) -> Self { + let held = pressed || other_side_down; + match key { + ModifierKey::Shift => self.shift = held, + ModifierKey::Control => self.control = held, + ModifierKey::Alt => self.alt = held, + ModifierKey::Meta => self.meta = held, + } + self + } } /// The key combination a desktop binds to "switch to the next keyboard