Fix Shift+Space IME toggle inserting a space (#641) - #670
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis pull request adds runtime logic to suppress Shift+Space fallback text during IME (Input Method Editor) handling in Ghostty's terminal view. A private suppression detection method is integrated into key event processing, with a public test helper variant and two new test cases validating the behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/GhosttyTerminalView.swift (1)
3145-3147: Use the original event for suppression checks to avoid modifier-translation coupling.Right now the predicate evaluates
translationEvent. Passing the originaleventkeeps this strictly tied to the physical Shift+Space shortcut and avoids config-dependent false positives/negatives.Suggested diff
- let suppressShiftSpaceFallbackText = - shouldSuppressShiftSpaceFallbackText( - event: translationEvent, - markedTextBefore: markedTextBefore - ) + let suppressShiftSpaceFallbackText = + shouldSuppressShiftSpaceFallbackText( + event: event, + markedTextBefore: markedTextBefore + )Also applies to: 3265-3269
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 3145 - 3147, The suppression predicate should use the original NSEvent rather than the translated/modified translationEvent to avoid coupling modifier checks to input-method translations: update calls to shouldSuppressShiftSpaceFallbackText(...) to pass the original event (the parameter named event) instead of translationEvent so the function evaluates the physical Shift+Space modifier state; adjust both occurrences around shouldSuppressShiftSpaceFallbackText and any related logic that currently forwards translationEvent (also at the second occurrence indicated) to maintain correct suppression 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/GhosttyTerminalView.swift`:
- Around line 3145-3147: The suppression predicate should use the original
NSEvent rather than the translated/modified translationEvent to avoid coupling
modifier checks to input-method translations: update calls to
shouldSuppressShiftSpaceFallbackText(...) to pass the original event (the
parameter named event) instead of translationEvent so the function evaluates the
physical Shift+Space modifier state; adjust both occurrences around
shouldSuppressShiftSpaceFallbackText and any related logic that currently
forwards translationEvent (also at the second occurrence indicated) to maintain
correct suppression behavior.
Greptile SummaryFixed Shift+Space IME toggle inserting unwanted space characters into the terminal. The fix adds targeted suppression logic that detects when AppKit/IME consumes Shift+Space for input-source switching (indicated by no
Confidence Score: 5/5
Important Files Changed
Last reviewed commit: 755831c |
Summary\n- suppress fallback text injection for Shift+Space when AppKit/IME consumes the event for input-source switching\n- keep key event dispatch intact (only avoid synthesized space text)\n- add regression tests for Shift+Space fallback suppression\n\n## Validation\n- ./scripts/setup.sh\n- xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux -configuration Debug -destination 'platform=macOS' build\n- ./scripts/reload.sh --tag fix-issue-641\n- xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux-unit -configuration Debug -destination 'platform=macOS' -only-testing:cmuxTests/CJKIMEShiftSpaceFallbackTests test\n\nCloses #641
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests