UI and UX improvements#1
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enhances the cmdcmd macOS app’s configurability and UX by adding an in-app Settings window, introducing new config options (minimal mode, display mode, vim bindings, letter jump), and improving input handling robustness via event taps.
Changes:
- Added a SwiftUI-based Settings window and new configuration fields persisted to
config.json. - Implemented minimal/icon-only overlay mode, letter-jump navigation, and usage-based ordering.
- Refactored overlay/key handling (NSPanel + event taps) and added window fade animations.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| Sources/cmdcmd/main.swift | Introduces @main entrypoint, Settings integration, and display mode application. |
| Sources/cmdcmd/Tile.swift | Adds minimal-mode rendering (icons) and supports tiles without SCWindow. |
| Sources/cmdcmd/SettingsWindow.swift | New SwiftUI Settings UI + model that saves config changes. |
| Sources/cmdcmd/OverlayView.swift | Adds letter input routing and refactors key handling into reusable handlers. |
| Sources/cmdcmd/Overlay.swift | Adds minimal-mode overlay path, usage-based ordering, and key event tap handling. |
| Sources/cmdcmd/NSWindowAnimations.swift | Adds reusable fade-in/out window animation helpers. |
| Sources/cmdcmd/Keymap.swift | Adds optional vim-style default bindings controlled by config. |
| Sources/cmdcmd/Config.swift | Expands config schema (minimal/display/vim/letterJump) and adds save(). |
| Sources/cmdcmd/CmdChord.swift | Adds a session event tap to improve cmd-cmd chord detection robustness. |
| README.md | Updates documentation for new settings, modes, and configuration options. |
| Package.swift | Adds -parse-as-library to support the new @main structure. |
| .pi/semantic-grep.sqlite | Adds a SQLite artifact file (appears tool-generated). |
| .brisk.toml | Adds Brisk build configuration for packaging/signing metadata. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| deinit { | ||
| if let o = workspaceObserver { | ||
| NotificationCenter.default.removeObserver(o) | ||
| } | ||
| if let o = appActivationObserver { | ||
| NSWorkspace.shared.notificationCenter.removeObserver(o) | ||
| } | ||
| } |
There was a problem hiding this comment.
Overlay’s deinit removes observers but doesn’t stop the activity timer or tear down the key event tap/run-loop source. If an Overlay instance were ever deallocated while visible (or if shutdown paths change), this could leave a tap/source installed longer than intended.
Suggestion: call stopActivityTimer() and stopKeyEventTap() (and nil out tap/source) from deinit as a defensive cleanup.
| private let trigger: String? | ||
| @Published var status: String = "" | ||
| var onSave: ((Config) -> Void)? | ||
|
|
||
| init(config: Config) { | ||
| animations = config.animations | ||
| minimalMode = config.minimalMode | ||
| displayMode = config.displayMode | ||
| vimBindings = config.vimBindings | ||
| letterJump = config.letterJump | ||
| trigger = config.trigger | ||
| } | ||
|
|
||
| func save() { | ||
| var config = Config.default | ||
| config.animations = animations | ||
| config.minimalMode = minimalMode | ||
| config.displayMode = displayMode | ||
| config.trigger = trigger |
There was a problem hiding this comment.
SettingsModel.save() rebuilds the file from Config.default and never carries over the existing bindings (or any future config fields). Opening Settings and toggling any switch will overwrite the user’s custom bindings in config.json.
Suggestion: keep a stored Config instance (initialized from the passed-in config) and only mutate the fields controlled by Settings, or explicitly preserve bindings from the initial config when saving.
| private let trigger: String? | |
| @Published var status: String = "" | |
| var onSave: ((Config) -> Void)? | |
| init(config: Config) { | |
| animations = config.animations | |
| minimalMode = config.minimalMode | |
| displayMode = config.displayMode | |
| vimBindings = config.vimBindings | |
| letterJump = config.letterJump | |
| trigger = config.trigger | |
| } | |
| func save() { | |
| var config = Config.default | |
| config.animations = animations | |
| config.minimalMode = minimalMode | |
| config.displayMode = displayMode | |
| config.trigger = trigger | |
| private var config: Config | |
| @Published var status: String = "" | |
| var onSave: ((Config) -> Void)? | |
| init(config: Config) { | |
| self.config = config | |
| animations = config.animations | |
| minimalMode = config.minimalMode | |
| displayMode = config.displayMode | |
| vimBindings = config.vimBindings | |
| letterJump = config.letterJump | |
| } | |
| func save() { | |
| config.animations = animations | |
| config.minimalMode = minimalMode | |
| config.displayMode = displayMode |
| let saved = savedOrder | ||
| let ordered: [Tile] | ||
| if saved.isEmpty { | ||
| ordered = mcTiles | ||
| } else { | ||
| let presentIDs = Set(mcTiles.map { CGWindowID($0.scWindow.windowID) }) | ||
| let knownInOrder = saved.filter { presentIDs.contains($0) } | ||
| let knownIDs = Set(knownInOrder) | ||
| let known = knownInOrder.compactMap { wid in mcTiles.first(where: { CGWindowID($0.scWindow.windowID) == wid }) } | ||
| let unknown = mcTiles.filter { !knownIDs.contains(CGWindowID($0.scWindow.windowID)) } | ||
| ordered = known + unknown | ||
| } | ||
| savedOrder = ordered.map { CGWindowID($0.scWindow.windowID) } | ||
| let usage = Self.usageOrder | ||
| let savedRanks = Dictionary(uniqueKeysWithValues: saved.enumerated().map { ($0.element, $0.offset) }) | ||
| let usageRanks = Dictionary(uniqueKeysWithValues: usage.enumerated().map { ($0.element, $0.offset) }) | ||
| let ordered = mcTiles.sorted { a, b in |
There was a problem hiding this comment.
Dictionary(uniqueKeysWithValues:) will trap at runtime if the input contains duplicate keys. Both saved (from UserDefaults tile order) and usage (from UserDefaults app usage order) can plausibly contain duplicates (e.g., after manual defaults edits or data corruption), which would crash when opening the overlay.
Suggestion: build rank maps in a way that tolerates duplicates (e.g., iterate and only set the first-seen rank, or dedupe the arrays before creating the dictionary).
| Task { | ||
| for t in toStop { await t.stop() } | ||
| for t in toStop { | ||
| t.layer.removeFromSuperlayer() |
There was a problem hiding this comment.
In hide(), the Task removes each tile’s CALayer from its superlayer. That Task is not confined to the main thread/actor, but Core Animation layer tree mutations are expected to be performed on the main thread; doing this off-main can lead to race conditions or crashes.
Suggestion: perform removeFromSuperlayer() on the main actor (e.g., wrap in MainActor.run / DispatchQueue.main.async) and keep the async t.stop() work off-main if desired.
| t.layer.removeFromSuperlayer() | |
| await MainActor.run { | |
| t.layer.removeFromSuperlayer() | |
| } |
| let container = NSView(frame: NSRect(origin: .zero, size: frame.size)) | ||
| container.autoresizingMask = [.width, .height] | ||
| container.wantsLayer = true | ||
| container.layer?.backgroundColor = config.minimalMode ? NSColor.clear.cgColor : NSColor.black.withAlphaComponent(0.18).cgColor | ||
| if config.minimalMode { | ||
| let blur = NSVisualEffectView(frame: container.bounds) | ||
| blur.autoresizingMask = [.width, .height] | ||
| blur.material = .hudWindow | ||
| blur.blendingMode = .withinWindow | ||
| blur.state = .active | ||
| blur.wantsLayer = true | ||
| blur.layer?.cornerRadius = 25 | ||
| blur.layer?.cornerCurve = .continuous | ||
| blur.layer?.masksToBounds = true | ||
| blur.layer?.borderWidth = 1 | ||
| blur.layer?.borderColor = NSColor.white.withAlphaComponent(0.18).cgColor | ||
| container.addSubview(blur) | ||
| } |
There was a problem hiding this comment.
makeWindow bakes config.minimalMode into the window chrome (container background + optional blur view). After the window is created once, toggling minimalMode via Settings updates self.config, but the existing window’s container/blur isn’t updated or rebuilt, so the UI may not actually reflect the new mode until restart or until the window is recreated.
Suggestion: when minimalMode changes, recreate the overlay window (or update the container view hierarchy/background) so the mode switch is visually consistent.
|
This is really awesome, will rebase and see how this runs. |
* Add NSWindow fade animation helpers Adds reusable fadeInAndUp / fadeOutAndDown extensions on NSWindow for smooth window transitions. No callers yet — landed standalone so future window UI (settings, status-item-driven panels) can adopt them. Lifted from #1 by @plyght. Co-Authored-By: plyght <plyght@peril.lol> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Wire window fade into overlay show/hide When animations are on, the overlay now fades in on show and fades out on hide using the new NSWindow helpers. Distance is 0 (alpha-only) since the overlay is fullscreen and any slide reveals desktop edges. Hide defers orderOut and the layer-clear into the fade completion so the fade isn't on an already-emptied window. Co-Authored-By: plyght <plyght@peril.lol> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: plyght <plyght@peril.lol> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Hey @plyght — thanks for this PR. To make it easier to test the moving parts in isolation, I split your changes into six smaller PRs and shipped each one separately:
A few intentional differences from your original PR for the local context:
You're co-credited on every commit ( Cheers! |
This pull request introduces several significant enhancements and refactorings to the
cmdcmdapp, focusing on configurability, user experience, and maintainability. The main improvements include the addition of a built-in settings window, expanded configuration options (including minimal and display modes), improved onboarding and permission handling, and technical upgrades such as event taps for more robust input handling. Below are the most important changes grouped by theme:Configuration and User Experience Enhancements:
minimalMode(for a lightweight, privacy-friendly UI),displayMode(to control Dock/menu-bar/hidden appearance),vimBindings, andletterJump. The config system is now more robust and includes a built-in settings window for immediate visual changes, while trigger edits still require restart. (README.md,Config.swift,Overlay.swift) [1] [2] [3] [4] [5] [6] [7]README.md)README.md)Technical Improvements:
CmdChordandOverlayfor more reliable detection of key events, improving the robustness of global hotkey and overlay interactions. (CmdChord.swift,Overlay.swift) [1] [2] [3] [4] [5] [6]NSWindowAnimations.swift)Key Binding and Customization:
h,j,k,l), controlled by the newvimBindingsconfig option. (Keymap.swift,Config.swift,Overlay.swift) [1] [2] [3] [4]Build and Packaging:
.brisk.tomlfor Brisk build configuration, specifying app metadata, dependencies, build targets, and signing. (.brisk.toml)Package.swift)These changes collectively make the app more flexible, easier to configure, and more user-friendly, while also improving technical reliability and maintainability.