Fix paste simulation for non-QWERTY keyboard layouts#56
Fix paste simulation for non-QWERTY keyboard layouts#56moona3k merged 7 commits intomoona3k:mainfrom
Conversation
## What Changed - Sources/MacParakeetCore/Services/ClipboardService.swift: Added `import Carbon` for TISCopyCurrentKeyboardInputSource and UCKeyTranslate APIs. Added `virtualKeyCode(for:)` helper that queries the active keyboard layout to find the physical keycode producing a given character. Updated `simulatePaste()` to use dynamic lookup instead of hardcoded 0x09. Falls back to 0x09 (QWERTY 'v') if the layout lookup fails. ## Root Intent The paste simulation hardcoded virtual keycode 0x09, which is the physical 'v' position on QWERTY. On Dvorak that position produces 'k', so Cmd+V was sent as Cmd+K. The same bug affects Colemak, AZERTY, and any non-QWERTY layout. The fix dynamically resolves which physical key produces 'v' on the current keyboard layout using Carbon's UCKeyTranslate API, which handles all layouts universally. ## Prompt That Would Produce This Diff In ClipboardService.swift, the simulatePaste() method hardcodes virtualKey 0x09 for Cmd+V paste simulation. This only works on QWERTY -- on Dvorak, key 0x09 is 'k'. Fix this by: 1. Adding `import Carbon` for keyboard layout APIs 2. Adding a `virtualKeyCode(for:)` helper that uses TISCopyCurrentKeyboardInputSource and UCKeyTranslate to iterate keycodes 0..<128 and find the one producing the target character on the current layout 3. Replacing the hardcoded 0x09 in simulatePaste() with a call to virtualKeyCode(for: "v") 4. Falling back to 0x09 if the lookup fails 5. Removing the now-redundant comment about the keycode ## Files Changed - Sources/MacParakeetCore/Services/ClipboardService.swift (+40, ~5) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces the hardcoded QWERTY virtual key code for Cmd+V with a layout-aware resolver: adds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request replaces the hardcoded virtual key code for the 'v' character with a dynamic lookup mechanism in ClipboardService.swift, ensuring better compatibility with various keyboard layouts. The feedback suggests improving type safety and idiomatic Swift usage by replacing unsafeBitCast with Unmanaged and assumingMemoryBound when interacting with Core Foundation types.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/MacParakeetCore/Services/ClipboardService.swift (1)
168-201: Add regression coverage for non-QWERTY paste mapping and fallback path.This bug fix should include at least one regression test for layout-aware
"v"resolution (plus fallback to0x09) to lock behavior. As per coding guidelines: “When fixing a bug: ... Write test reproducing bug ... Run focused tests ... and full swift test ...”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/MacParakeetCore/Services/ClipboardService.swift` around lines 168 - 201, Add a regression test that exercises ClipboardService.virtualKeyCode(for:) for a non‑QWERTY layout and the fallback path: simulate or stub TISCopyCurrentKeyboardInputSource/TISGetInputSourceProperty/UCKeyTranslate (or inject a test keyboard layout) so that resolving the character "v" returns the expected layout-specific CGKeyCode and, separately, that when no matching key is found the method returns the fallback 0x09; ensure the test asserts both the correct mapped code for a known non‑QWERTY layout and the fallback value to lock behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Sources/MacParakeetCore/Services/ClipboardService.swift`:
- Around line 168-201: Add a regression test that exercises
ClipboardService.virtualKeyCode(for:) for a non‑QWERTY layout and the fallback
path: simulate or stub
TISCopyCurrentKeyboardInputSource/TISGetInputSourceProperty/UCKeyTranslate (or
inject a test keyboard layout) so that resolving the character "v" returns the
expected layout-specific CGKeyCode and, separately, that when no matching key is
found the method returns the fallback 0x09; ensure the test asserts both the
correct mapped code for a known non‑QWERTY layout and the fallback value to lock
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 35feece6-bfbf-4e5d-8ec2-387809a4738d
📒 Files selected for processing (1)
Sources/MacParakeetCore/Services/ClipboardService.swift
The previous PR resolved Cmd+V from TISCopyCurrentKeyboardInputSource(). That works when the active input source is itself a keyboard layout, but it is the wrong abstraction once the user has an IME or input mode selected. In those cases macOS returns the IME/input mode as the current input source, and Apple documents kTISPropertyUnicodeKeyLayoutData as unavailable for non-layout sources. The helper would then silently fall back to the hardcoded QWERTY keycode 0x09, recreating the original failure for users whose underlying layout is Dvorak, Colemak, AZERTY, or another non-QWERTY layout behind the IME. Switch the lookup to TISCopyCurrentKeyboardLayoutInputSource(), which asks macOS for the backing keyboard layout rather than the currently selected IME/input mode wrapper. That keeps the dynamic UCKeyTranslate-based resolution introduced in the PR, but makes it cover both direct layout users and IME-backed layout users. The fallback to 0x09 remains as a defensive last resort, but it is now explicit in the logs so we can see when layout resolution fails instead of silently masking the condition. This keeps the runtime behavior safe while making the shortcut-resolution path materially more correct.
|
Hey there, thank you so much for the PR! This is the first one for MacParakeet :) I made a small update to the key-resolution logic to cover the remaining IME/input-mode path as well. When you get a chance, can you run a sanity check on your setup and let me know if paste works as expected? |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/MacParakeetCore/Services/ClipboardService.swift`:
- Around line 171-177: In virtualKeyCode(for:modifierKeyState:) avoid calling
.takeRetainedValue() on a potentially nil
TISCopyCurrentKeyboardLayoutInputSource(); instead guard-let the returned
Unmanaged value and call takeRetainedValue() only after unwrapping (e.g., guard
let layoutSourceUnmanaged = TISCopyCurrentKeyboardLayoutInputSource() else {
logger.error(...); return fallbackKeyCode }; let layoutSource =
layoutSourceUnmanaged.takeRetainedValue()), then proceed to get
kTISPropertyUnicodeKeyLayoutData – ensure the early-exit uses the same
fallbackKeyCode and logs an error if the input source is nil.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 390fbd7c-0cfb-4e91-b03d-d8d834a8450f
📒 Files selected for processing (1)
Sources/MacParakeetCore/Services/ClipboardService.swift
kUCKeyTranslateNoDeadKeysBit (0) is the bit index, not the bitmask. kUCKeyTranslateNoDeadKeysMask (1) is the correct value for UCKeyTranslate's keyTranslateOptions parameter. No-op for 'v' lookup but fixes the API contract. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/MacParakeetCore/Services/PasteShortcutKeyResolver.swift (1)
12-13: Logger category should match the resolver's identity.The logger category is
"ClipboardService"but this isPasteShortcutKeyResolver. Consider updating for clearer log filtering:- private let logger = Logger(subsystem: "com.macparakeet.core", category: "ClipboardService") + private let logger = Logger(subsystem: "com.macparakeet.core", category: "PasteShortcutKeyResolver")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/MacParakeetCore/Services/PasteShortcutKeyResolver.swift` around lines 12 - 13, The logger on PasteShortcutKeyResolver currently uses category "ClipboardService" which is misleading; update the Logger initialization inside the PasteShortcutKeyResolver struct (the private let logger property) to use a category that matches the resolver's identity (e.g., "PasteShortcutKeyResolver" or similar) so logs are filtered and attributed correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Sources/MacParakeetCore/Services/PasteShortcutKeyResolver.swift`:
- Around line 12-13: The logger on PasteShortcutKeyResolver currently uses
category "ClipboardService" which is misleading; update the Logger
initialization inside the PasteShortcutKeyResolver struct (the private let
logger property) to use a category that matches the resolver's identity (e.g.,
"PasteShortcutKeyResolver" or similar) so logs are filtered and attributed
correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e22d79aa-52d8-4652-83f3-94acdf4bbbd5
📒 Files selected for processing (3)
Sources/MacParakeetCore/Services/ClipboardService.swiftSources/MacParakeetCore/Services/PasteShortcutKeyResolver.swiftTests/MacParakeetTests/Services/PasteShortcutKeyResolverTests.swift
|
Alrighty, the fix looks good, no regression, this will be included in the next release. |
Thanks for improving and merging! Your way is better, and it now helped me improve my own app to keep hint keys on the home row for alternate layouts! http://www.tabikeys.com if you're looking for a better Vimium clone for Safari, msg me if you want a free version once it's ready :) |
2 participants
Fixes #54.
What Changed
Sources/MacParakeetCore/Services/ClipboardService.swift: Addedimport Carbonplus avirtualKeyCode(for:)helper that usesUCKeyTranslateto resolve the keycode forvfrom the active keyboard layout instead of hardcoding QWERTY keycode0x09.Cmd+Vusing the resolved keycode.TISCopyCurrentKeyboardLayoutInputSource()so IME/input-mode users still resolve against the backing keyboard layout.0x09fallback only as a defensive last resort, with explicit logging when layout resolution fails.Root Intent
The original paste simulation hardcoded virtual keycode
0x09, which is the physicalvposition on QWERTY. On Dvorak that position producesk, soCmd+Vwas sent asCmd+K. The same class of bug affects Colemak, AZERTY, and other non-QWERTY layouts.The first pass of this PR replaced the hardcoded keycode with a dynamic lookup, but it queried
TISCopyCurrentKeyboardInputSource(). That works when the active input source is itself a keyboard layout, but it is incomplete when the active source is an IME or input mode. In those cases,kTISPropertyUnicodeKeyLayoutDatacan be unavailable on the current input source, causing the helper to fall back to the old QWERTY keycode.Using
TISCopyCurrentKeyboardLayoutInputSource()fixes the abstraction mismatch by asking macOS for the backing keyboard layout rather than the current IME wrapper. That makes the shortcut resolution work for both direct non-QWERTY layouts and IME-backed non-QWERTY layouts.Validation
swift buildswift testSummary by CodeRabbit