Add presets — named, switchable configurations - #1819
Open
engineer-myoa wants to merge 9 commits into
Open
Conversation
Also fixes JSONDefault leaving its decoded value in place when a config sets the underlying string to nil, which made footprint colors and snap areas impossible to reset through import.
awakeFromNib runs more than once for this storyboard scene, so the picker row was inserted twice and the configImported observer was registered twice. Everything else in awakeFromNib happens to be idempotent, which is why this had not surfaced before.
Storing the preset library in Defaults.array meant every exported config carried it as an escaped JSON string wedged between the individual settings, which is awkward to read in a file users are expected to share and hand-edit. A dedicated optional field keeps the same data structured. Both directions stay compatible: builds without the field ignore it when decoding, and the field is optional so configs exported before presets existed still load.
Per maintainer feedback on discussion rxhanson#1818: the Shortcuts tab stays as it is, and the picker now sits under a "Presets" header at the end of the Extras popover, reached from the button at the bottom of the General tab. PrefsViewController keeps only its configImported observer, which rebinds the shortcut recorders so the tab reflects a newly applied preset.
Owner
|
Thanks for following through with contributing! There's a decent amount to unpack and test out here, so it'll take me a little bit to walk through this one. I'm planning on cutting a release in the next day or so, and will dive a little deeper into this after the release. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed and discussed first in #1818 — posting the PR now since you said you'd be inclined to move forward.
I switch between a full-size keyboard, a tenkeyless one, and the MacBook's built-in keyboard. Most of my shortcuts sit on the numeric keypad, so on the other two they're unreachable. Import/Export can move a configuration around, but it's two file dialogs each time, there's no notion of a named set, and a config file can't express "this action is deliberately unbound" —
Defaults.encoded()skips the empty-dictionary state, so with #1805 making import authoritative the key is removed and the built-in default comes back instead.What this adds
A preset is a named snapshot of the shortcuts plus the settings that describe window behavior. Settings about the app itself — launch at login, update checks, menu bar icon, per-app exclusions, transient Todo state — stay global. The exclusion list is explicit and tested against
Defaults.arrayso a future rename can't silently pull a key in.Shortcuts are stored in all three states MASShortcut distinguishes (absent / explicitly unbound / assigned), so switching restores unbound bindings as unbound. Snapshots read
persistentDomain(forName:)rather thandictionary(forKey:)to tell an untouched shortcut from an assigned one.The active preset mirrors the live settings, so there's no save button or dirty state. Applying one writes settings first (so
alternateDefaultShortcutsis current beforeregisterDefaults()re-runs), then shortcuts, then postsconfigImportedand lets the existing observers reconcile. Presets can be created from the built-in defaults or by duplicating the current one, and renamed or deleted.UI
Per your feedback, the picker is in the Extras popover under a "Presets" header rather than the Shortcuts tab. No storyboard changes.
PrefsViewControllerkeeps only aconfigImportedobserver so the shortcut recorders reflect a newly applied preset.That observer is registered in
viewDidLoadrather thanawakeFromNib, becauseawakeFromNibruns twice for that scene — everything already in it happens to be idempotent, so it hadn't surfaced before.Export format
Presets ride along in Import/Export through a new optional
Config.presetsfield rather than another entry indefaults, which would have put the whole library into the file as one escaped JSON string. Both directions stay compatible and there are tests for it: builds without the field ignore it when decoding, and the field is optional so older configs still load.Configkeeps a memberwise initializer withpresetsdefaulting to nil, so existing call sites are unchanged.This is the one place the feature steps outside the "every new key goes in
Defaults.array" convention. The convention's purpose — never silently dropping a key from export — is still met by the dedicated field.Along the way
Defaults.load(fileUrl:)'s settings loop is extracted toDefaults.apply(defaults:)so import and preset switching share a path. Pure extraction, no behavior change.JSONDefault.load(from:)now clears its decoded value when a config sets the underlying string to nil; previously footprint colors and snap areas couldn't be reset through import. Different path from theinit(key:defaultValue:)fix in Honor defaults-write customizations for JSONDefault keys #1811.I've kept an eye on #1782 since you mentioned it — the
Defaults.apply(defaults:)seam is small and isolated, so it shouldn't get in the way of reworking configuration management later.Testing
29 new tests, all passing. For reference,
f6279e2runs 223 tests with 22 failing assertions acrossActiveSideSplitRatiosCooperativeTests,CooperativeCornerResizeTestsandHalfSplitCornerCalculationTests; this branch runs 252 with the same 22. I checked an unmodified checkout to confirm they aren't from this work — happy to open a separate issue for them if useful.Happy to adjust anything, including hiding the picker behind a preference if you'd rather it not be visible by default.
🤖 Generated with Claude Code