From 03d8da14c938c8eb5939e9903ec015b2ef727024 Mon Sep 17 00:00:00 2001 From: iga566 Date: Sun, 6 Sep 2026 22:16:35 +0500 Subject: [PATCH 1/2] engine: force-by-selection follows the text, not current() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit converted() trusted layout_switcher.current() for the source layout. If the layout moved on since the word was typed — exactly what the next word does on auto-correction — the guess was wrong, transliterate_to's own guard refused it (no letter of that layout in the text), and the force-switch silently did nothing. Both directions are tried now, current first, falling back to the swap when the text does not belong to it. transliterate_to already refuses a source layout the text carries no letter of, so the wrong direction rejects itself and nothing is forced through blind. Measured on macOS 26 and Windows Server 2025 with the fix: repeated presses on one selection convert it back and forth, each from the first press, whichever layout is active at the time. Co-Authored-By: Claude Fable 5.1 --- .../src/engine/switcher/correction.rs | 36 +++++-- crates/poltertype-core/src/engine/tests.rs | 95 +++++++++++++++++++ 2 files changed, 125 insertions(+), 6 deletions(-) diff --git a/crates/poltertype-core/src/engine/switcher/correction.rs b/crates/poltertype-core/src/engine/switcher/correction.rs index 8b3a978..646c339 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 202abbb..0b09c76 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. /// From 33cab6dbc6dc0e0c806f0260611e15a67fbaa73a Mon Sep 17 00:00:00 2001 From: iga566 Date: Tue, 8 Sep 2026 03:32:11 +0500 Subject: [PATCH 2/2] windows: the hook's modifier snapshot describes the event, not the moment before it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GetAsyncKeyState inside WH_KEYBOARD_LL reports the keyboard as it was before the event being delivered has taken effect. A Ctrl release therefore arrived reading "Ctrl held", and with nothing typed afterwards that reading was the last one the engine got: it believed the force-switch chord was still down and waited for a release it had already been handed — CHORD_RELEASE_WAIT is five seconds, and the wait ended only when the next keystroke refreshed the snapshot. Measured on Windows Server 2025 (over RDP, 0.34.0): six manual switches took 1.65–4.69 s from `applying correction` to the layout change, each exactly as long as the user took to press the hotkey again; automatic corrections on the same path took 0 ms. The second press then landed inside the re-arm window and was swallowed, or undid the first, or started a fresh correction on top of it. convert_selection runs the same wait first, so selection conversion never got as far as a log line. With this change the same presses take 0.13–0.26 s and selection conversion works. For a modifier the event is about, the event is the truth: the platform applies the delivered key's own press/release to the snapshot, reading the other side of the same modifier live so that releasing one Shift while the other is held keeps Shift. The transition is a method on Modifiers in poltertype-types, platform-free and unit-tested; the VK mapping and the live reads sit in a modifiers.rs sibling of the listener, since `xtask style` refuses a seventh free function beside the type. Nothing changes for macOS, whose event tap already hands out post-event flags, nor for Linux. Co-Authored-By: Claude Fable 5.1 --- .../poltertype-input/src/windows/listener.rs | 27 +------ crates/poltertype-input/src/windows/mod.rs | 2 + .../poltertype-input/src/windows/modifiers.rs | 80 +++++++++++++++++++ crates/poltertype-types/src/enums.rs | 13 +++ crates/poltertype-types/src/tests.rs | 60 ++++++++++++++ crates/poltertype-types/src/types.rs | 33 ++++++++ 6 files changed, 191 insertions(+), 24 deletions(-) create mode 100644 crates/poltertype-input/src/windows/modifiers.rs diff --git a/crates/poltertype-input/src/windows/listener.rs b/crates/poltertype-input/src/windows/listener.rs index b29972f..d252831 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 2dfc4e6..2277220 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 0000000..c73a3b8 --- /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 f66fbdb..edfb82b 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 4a5c3bf..c915f89 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 059082c..8aaf379 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