feat: add keyboard page navigation - #71
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe change replaces page indicator gestures with buttons, adds English and Simplified Chinese shortcut text, and centralizes keyboard handling for screen selection, arrow navigation, and Command-Q termination. ChangesNavigation controls
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant LocalKeyMonitor
participant IslandWindowController
participant PageIndicator
LocalKeyMonitor->>IslandWindowController: forward key event to handleKeyDown
IslandWindowController->>PageIndicator: select or navigate screen
IslandWindowController->>IslandWindowController: terminate on Command-Q
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 376b4adf5a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if modifiers == .command, | ||
| let character = event.charactersIgnoringModifiers, | ||
| let index = ["1", "2", "3"].firstIndex(of: character) { |
There was a problem hiding this comment.
On keyboard layouts where number-row digits require Shift, such as French AZERTY, the key labeled 1 produces either & without Shift or 1 with .shift; the exact modifiers == .command check therefore rejects both ways of entering the advertised ⌘1 shortcut. Numeric-keypad events are similarly rejected because they include .numericPad. Match these shortcuts through layout-aware key-equivalent handling or otherwise account for the additional flags so direct page selection remains usable outside US-style layouts.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Window/IslandWindowController.swift (1)
139-145: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep the key monitor active while the island window is key.
Line 142 removes the only
.keyDownmonitor when the cursor leaves the island. Line 138 makes the window key, but the exit path does not resign it. The expanded island can remain key but cannot receive Command-number or arrow navigation.Install one monitor for the controller or visible-window lifetime. Keep the Line 150 key-window guard to scope handled events.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/Window/IslandWindowController.swift` around lines 139 - 145, Update the key-monitor lifecycle around the controller’s .keyDown monitor setup so it remains installed for the controller or visible-window lifetime instead of being removed when the cursor leaves the island. Remove the exit-path cleanup that clears cmdQMonitor, while preserving the existing key-window guard in the event handler to scope Command-number and arrow navigation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Sources/Window/IslandWindowController.swift`:
- Around line 139-145: Update the key-monitor lifecycle around the controller’s
.keyDown monitor setup so it remains installed for the controller or
visible-window lifetime instead of being removed when the cursor leaves the
island. Remove the exit-path cleanup that clears cmdQMonitor, while preserving
the existing key-window guard in the event handler to scope Command-number and
arrow navigation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5860f327-110d-43e4-b5c6-9721b9774125
📒 Files selected for processing (4)
Resources/en.lproj/Localizable.stringsResources/zh-Hans.lproj/Localizable.stringsSources/Views/PageIndicator.swiftSources/Window/IslandWindowController.swift
The keyboard-navigation change also grew the dots to 7–8pt inside 24pt button slots, roughly doubling the indicator's footprint in the footer. Restore the original geometry, spacing, and opacities; keep the Button wrapper and the ⌘-shortcut tooltip.
376b4ad to
30cc396
Compare
|
Thanks — the keyboard handling here is exactly right: hover-scoped monitor, key-window guard, expanded-only gating, and it applies cleanly on top of v0.1.23. Merging. Two things I did on the branch (rebased onto main + one commit on top): the page-indicator dots were growing from 5pt to 7–8pt inside 24pt button slots, which roughly doubled the indicator's footer footprint — I restored the original geometry/opacities but kept your |
2 participants
What
Adds Command-click cycling and Command-1/2/3 direct page selection for the island page indicator.
The existing one-shot footer hint remains unchanged, keeping persistent chrome quiet. Caps Lock is ignored when matching Command shortcuts so enabled Caps Lock does not break navigation.
Validation
Split from #67 following maintainer feedback.
Summary by CodeRabbit
Release Notes
New Features
UI/UX Improvements
Localization