Fix paste not working on non-QWERTY keyboard layouts (Dvorak, Colemak, etc.)#457
Conversation
…, etc.) The text-writer used hardcoded keycode 9 (physical 'V' key on QWERTY) to simulate Cmd+V paste. On Dvorak and other layouts, this physical key produces a different character, so paste never triggered. This fix uses macOS TIS (Text Input Source) APIs to dynamically look up which physical keycode produces 'v' in the current keyboard layout, making the paste command work regardless of the active layout. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughAdds a macOS-only Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)native/**/*.rs📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧬 Code graph analysis (1)native/text-writer/src/keyboard_layout.rs (1)
🔇 Additional comments (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
native/text-writer/src/keyboard_layout.rs (1)
44-99: Consider caching the keycode map for better performance.The function rebuilds the entire keycode map on every call, iterating 128 keycodes with FFI calls. For the current paste use case this is acceptable, but caching the map (e.g., using
std::sync::OnceLockor lazy_static) would improve performance if the function is called frequently or from hot paths in the future.Example with
OnceLock:use std::sync::OnceLock; static KEYCODE_MAP: OnceLock<HashMap<char, u16>> = OnceLock::new(); fn get_keycode_map() -> &'static HashMap<char, u16> { KEYCODE_MAP.get_or_init(|| build_char_to_keycode_map()) } pub fn keycode_for_char(ch: char) -> Option<u16> { get_keycode_map().get(&ch.to_ascii_lowercase()).copied() }Note: This assumes the keyboard layout doesn't change during runtime, which is a reasonable assumption for most use cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
native/text-writer/src/keyboard_layout.rs(1 hunks)native/text-writer/src/macos_writer.rs(2 hunks)native/text-writer/src/main.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
native/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
native/**/*.rs: Use standard Rust formatting with 100 character width and Unix line endings as defined in rustfmt.toml
Follow Rust clippy linter configuration defined in clippy.toml with cognitive complexity threshold settings
Apply Cargo.toml workspace-level lint rules including pedantic and nursery warnings to all native Rust modules
Files:
native/text-writer/src/keyboard_layout.rsnative/text-writer/src/main.rsnative/text-writer/src/macos_writer.rs
🧬 Code graph analysis (2)
native/text-writer/src/keyboard_layout.rs (1)
native/selected-text-reader/src/macos.rs (1)
CFRelease(35-35)
native/text-writer/src/macos_writer.rs (1)
native/text-writer/src/keyboard_layout.rs (1)
get_paste_keycode(110-112)
🔇 Additional comments (5)
native/text-writer/src/macos_writer.rs (2)
10-10: LGTM! Excellent fix for non-QWERTY layouts.The dynamic keycode lookup via
keyboard_layout::get_paste_keycode()properly replaces the hardcoded value and directly addresses the Dvorak/Colemak paste issue.Also applies to: 56-57
60-62: Correct usage of dynamic keycode.Both key down and key up events properly use the layout-aware
v_keycode, ensuring consistent behavior across keyboard layouts.native/text-writer/src/keyboard_layout.rs (2)
68-93: Well-structured keycode iteration and mapping logic.The implementation correctly:
- Iterates through the standard macOS virtual keycode range (0-127)
- Uses UCKeyTranslate with no modifiers to get base characters
- Handles dead keys appropriately
- Validates successful translation (result == 0, single character)
- Prefers lower keycodes for duplicate mappings
101-112: Clean public API design with appropriate fallback.The public functions provide:
keycode_for_char(): General-purpose character-to-keycode lookup returningOption<u16>get_paste_keycode(): Specialized function with safe fallback to QWERTY keycode 9The fallback ensures paste functionality never breaks, even if layout detection fails.
native/text-writer/src/main.rs (1)
9-10: LGTM! Proper platform-specific module wiring.The
keyboard_layoutmodule is correctly gated behind#[cfg(target_os = "macos")], consistent with the existingmacos_writermodule pattern.
The keyboard layout map is now built once on first access and cached using std::sync::OnceLock, avoiding redundant FFI calls on each paste. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
native/text-writer/src/keyboard_layout.rs (1)
14-40: Past review concerns about FFI signatures remain unaddressed.The FFI declarations still use incorrect types that don't match Apple's official API specifications:
- Lines 17, 18-21, 23: Should use
CFTypeReffrom thecore-foundationcrate instead of raw*mut c_void/*const c_void- Line 29: Should be
*const UCKeyboardLayout(a proper opaque type or struct) instead of*const u8- Line 39: Should return
OSStatusinstead ofi32- Lines 36-37: Should use
UniCharCountand*mut UniCharCountinstead ofusize/*mut usizeAs noted in the previous review, these type mismatches create semantic incorrectness in the FFI layer and may cause issues with future compiler or platform changes.
🧹 Nitpick comments (1)
native/text-writer/src/keyboard_layout.rs (1)
107-110: Consider edge case: non-ASCII characters.The use of
to_ascii_lowercase()at lines 94 and 109 means non-ASCII characters will not be lowercased—they'll be returned unchanged. For most keyboard layouts this is fine since typical keys produce ASCII characters, but for completeness you could use.to_lowercase().next()(taking the first char of the lowercased string) to handle Unicode characters correctly.This is a minor edge case and the current implementation is acceptable for the intended use case.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
native/text-writer/src/keyboard_layout.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
native/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
native/**/*.rs: Use standard Rust formatting with 100 character width and Unix line endings as defined in rustfmt.toml
Follow Rust clippy linter configuration defined in clippy.toml with cognitive complexity threshold settings
Apply Cargo.toml workspace-level lint rules including pedantic and nursery warnings to all native Rust modules
Files:
native/text-writer/src/keyboard_layout.rs
🧬 Code graph analysis (1)
native/text-writer/src/keyboard_layout.rs (1)
native/selected-text-reader/src/macos.rs (1)
CFRelease(35-35)
- Add UCKeyboardLayout opaque struct for proper type safety - Add CFTypeRef, OSStatus, UniCharCount type aliases - Update FFI signatures to match Apple documentation - Reference Text Input Sources and Unicode Utilities docs 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The constant value 0 means "no options", not a specific bit flag. Renamed and added doc comment for clarity. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
native/text-writer/src/keyboard_layout.rs (1)
58-58: Misleading constant name persists.The constant
KUC_KEY_TRANSLATE_NO_DEAD_KEYS_BITis still set to0, but the name suggests it's a specific bit flag. A value of0means "no options" rather than a flag that disables dead keys. This was flagged in a previous review but remains unaddressed.
🧹 Nitpick comments (1)
native/text-writer/src/keyboard_layout.rs (1)
12-13: Runtime layout switching won't update the cached keycode map.The
OnceLockcache is initialized once on first access and never refreshed. If a user switches keyboard layouts during runtime (e.g., from QWERTY to Dvorak via System Preferences), paste operations will continue using keycodes from the original layout, potentially breaking the shortcut until the application restarts.If runtime layout switching is a supported use case, consider:
- Exposing an API to invalidate and rebuild the cache, or
- Subscribing to layout-change notifications via
kTISNotifySelectedKeyboardInputSourceChanged.For most users who rarely switch layouts, the current approach is acceptable and avoids repeated FFI overhead.
Also applies to: 124-124
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
native/text-writer/src/keyboard_layout.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
native/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
native/**/*.rs: Use standard Rust formatting with 100 character width and Unix line endings as defined in rustfmt.toml
Follow Rust clippy linter configuration defined in clippy.toml with cognitive complexity threshold settings
Apply Cargo.toml workspace-level lint rules including pedantic and nursery warnings to all native Rust modules
Files:
native/text-writer/src/keyboard_layout.rs
🧬 Code graph analysis (1)
native/text-writer/src/keyboard_layout.rs (1)
native/selected-text-reader/src/macos.rs (1)
CFRelease(35-35)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
native/text-writer/src/keyboard_layout.rs (1)
79-79: Remove unnecessarycast_mut()when callingCFRelease.
CFReleaseacceptsCFTypeRef(a const pointer*const c_void) directly, so thecast_mut()conversion is unnecessary and semantically misleading.Apply this diff:
- CFRelease(input_source.cast_mut()); + CFRelease(input_source);And at line 116:
- CFRelease(input_source.cast_mut()); + CFRelease(input_source);Also applies to: 116-116
🧹 Nitpick comments (1)
native/text-writer/src/keyboard_layout.rs (1)
83-84: Correct misleading comment.
wrap_under_get_ruledoes retain the object (increments refcount). The comment states the opposite, which could confuse future maintainers. The usage is correct—sinceTISGetInputSourcePropertyfollows Get semantics, retaining is appropriate.- // Wrap the CFData without retaining (it's owned by input_source) + // Wrap the CFData with retain (Get rule: we don't own it, so we retain for safe use) let layout_data: CFData = TCFType::wrap_under_get_rule(layout_data_ref.cast());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
native/text-writer/src/keyboard_layout.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
native/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
native/**/*.rs: Use standard Rust formatting with 100 character width and Unix line endings as defined in rustfmt.toml
Follow Rust clippy linter configuration defined in clippy.toml with cognitive complexity threshold settings
Apply Cargo.toml workspace-level lint rules including pedantic and nursery warnings to all native Rust modules
Files:
native/text-writer/src/keyboard_layout.rs
🧬 Code graph analysis (1)
native/text-writer/src/keyboard_layout.rs (1)
native/selected-text-reader/src/macos.rs (1)
CFRelease(35-35)
🔇 Additional comments (6)
native/text-writer/src/keyboard_layout.rs (6)
1-13: LGTM!Module documentation clearly describes the purpose, and imports are appropriately scoped for the FFI work. Using
OnceLockfor the cached keycode map is the correct pattern for lazy, thread-safe initialization.
15-29: LGTM!FFI type definitions correctly model Apple's Carbon/CoreServices types. The opaque
UCKeyboardLayoutstruct pattern is the idiomatic Rust approach for representing opaque C types.
31-55: LGTM!FFI declarations correctly match Apple's Text Input Sources Reference and Unicode Utilities documentation. The type signatures align with the Carbon/CoreServices APIs.
57-62: LGTM!Constants are correctly defined with appropriate values matching Apple's Unicode Utilities headers. The doc comment on
KUC_KEY_TRANSLATE_NO_OPTIONSclarifies that zero means default behavior.
122-127: LGTM!Clean public API with lazy initialization via
OnceLock. The lowercase conversion ensures consistent lookup behavior matching the map population logic.
129-133: LGTM!The fallback to
QWERTY_V_KEYCODEensures paste functionality remains available even when layout detection fails, while correctly returning the layout-specific keycode when available.
CFRelease accepts CFTypeRef (*const c_void) directly, so the cast_mut() was unnecessary. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
wrap_under_get_rule retains the object (Get rule semantics), not the opposite as the comment incorrectly stated. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Use TISCopyCurrentASCIICapableKeyboardLayoutInputSource() instead of TISCopyCurrentKeyboardLayoutInputSource() to always query the user's Latin keyboard layout for shortcut keycodes. This fixes Cmd+V paste when a non-Latin layout like Russian is active, especially for users with alternative Latin layouts like Dvorak. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Summary
Problem
On Dvorak (and other non-QWERTY layouts), the physical key at position 9 produces a different character, so Cmd+V never triggered the paste action. Transcribed text was copied to clipboard but never pasted.
Solution
Added
keyboard_layout.rsmodule that:TISCopyCurrentKeyboardLayoutInputSource()to get the current layoutUCKeyTranslate()to build a character→keycode lookup tableTest plan
cargo buildandcargo clippypass with no warnings🤖 Generated with Claude Code