[shortcuts] Add import and export backups for Keyboard shortcuts#4226
[shortcuts] Add import and export backups for Keyboard shortcuts#4226jensenpat wants to merge 1 commit into
Conversation
![aethersdr-agent[bot]](https://avatars.githubusercontent.com/in/3286087?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thorough, well-tested work — thanks for this, @jensenpat. The RFC-style CSV parser, the batch-atomic import (nothing changes unless the whole file parses and resolves), and the id-primary / name-fallback identity model are all handled carefully, and the cross-release default semantics (persisted → CUSTOMIZED, non-customized rows adopting the importing release's default) round-trip correctly. Conflict resolution — clear colliding non-imported keys, apply, then reuse the startup normalizeDuplicateBindings() so customized wins over a default restored by a later row — is consistent with the existing setBinding() behavior. Both entry points rebuild live QShortcuts (GUI via the post-exec() rebuild in MainWindow_Menus.cpp, automation inline), export is atomic via QSaveFile, and conventions look right (AppSettings, no QSettings, RAII). All seven checks pass, and nothing is out of the stated scope.
I have no blocking findings.
Non-blocking notes
importFromFile()reads the whole file into memory before the 1 MiB guard insideimportBindingsCsv()runs — a pre-readfile.size()check at the boundary would be a touch more defensive. See inline. Since the path is operator/user-selected and the byte guard still catches it, this is purely polish.
🤖 aethersdr-agent · cost: $8.1291 · model: claude-opus-4-8
| .arg(QDir::toNativeSeparators(path)); | ||
| return result; | ||
| } | ||
| return manager.importBindingsCsv(file.readAll()); |
There was a problem hiding this comment.
Minor / non-blocking: readAll() pulls the entire file into memory before the 1 MiB guard in parseCsvRows() gets a chance to reject it, so pointing import at a multi-GB file loads it all first. Since this is a user/operator-selected local path and the byte-size check still fires post-read, it's low severity — but a file.size() check here (mirroring kMaxShortcutCsvBytes) would reject oversized files at the boundary without the allocation. Left as a plain note rather than a suggestion to avoid duplicating the limit constant across the file boundary.
There was a problem hiding this comment.
Really nicely done — this is a clean, self-contained feature with a hand-rolled RFC-4180 CSV parser that handles quoting/UTF-8/size limits, batch-atomic imports that touch no binding unless the whole file resolves, and thoughtful cross-release semantics (stable-id primary key with an exact-name fallback, importing-release defaults for non-customized rows, explicit-clear preservation). The ShortcutFileTransfer seam that lets the Shortcuts Editor buttons and the shortcutConfig bridge verb share one parser/writer/persistence/rebuild path is the right factoring, and it's backed by real tests. All 7 CI checks are green and the commit is signed. Conventions look good — AppSettings (not QSettings), QSaveFile for atomic writes, RAII throughout, no leaks.
I found nothing blocking. A couple of small notes below.
Polish
parseShortcutCsvtreats any non-emptySHORTCUTthatQKeySequence::fromStringcan't decode as valid because unrecognized tokens decode toQt::Key_unknownrather than an empty sequence — see inline. Hand-edited-CSV path only; exports never produce it.
Non-blocking notes
docs/automation-bridge.md— theshortcutConfigexamples show"exported":146/"imported":146, but the registry is ~92 actions (per your own validation). Worth aligning the placeholder so the doc doesn't imply a specific count.- Not this PR, but a heads-up:
shortcutConfig importmutates persisted state + rebuilds live shortcuts, so when the observe-only bridge mode (#4208) lands it'll want to be outside that allowlist.
🤖 aethersdr-agent · cost: $7.9034 · model: claude-opus-4-8
|
|
||
| const QKeySequence shortcut = QKeySequence::fromString( | ||
| shortcutText, QKeySequence::PortableText); | ||
| if (!shortcutText.isEmpty() && shortcut.isEmpty()) { |
There was a problem hiding this comment.
QKeySequence::fromString("Frobnicate", PortableText) doesn't return an empty sequence for un-decodable text — Qt maps unknown tokens to Qt::Key_unknown, so shortcut.isEmpty() is false and a garbage SHORTCUT value in a hand-edited CSV slips through as an unusable binding rather than being rejected. Since the code already reads currentKey[0].key() elsewhere, catching the unknown key here closes the gap:
| if (!shortcutText.isEmpty() && shortcut.isEmpty()) { | |
| if (!shortcutText.isEmpty() | |
| && (shortcut.isEmpty() || shortcut[0].key() == Qt::Key_unknown)) { |
3dca302 to
90c2ae4
Compare
jensenpat
changed the title
jensenpat
changed the title
jensenpat
changed the title
|
Holding for next weeks release. |
|
First-pass triage review (not exhaustive — surfacing blockers to keep this moving): Read the full diff plus Blockers None spotted in a first pass. Import is non-destructive on any parse/resolve failure and the hostile-input surface (size, encoding, quoting, field/row bounds, duplicates) is covered. Secondary notes (non-blocking, for author judgment)
Nice work — reads clean and defensively written; treating the above as polish, not gates. |
2 participants
Summary
Adds portable shortcut backup and cloning to the Shortcuts Editor. Users can now export their complete keyboard mapping to CSV and import it on another AetherSDR installation, including across older and newer releases.
The editor footer now presents the controls in this order:
Import... → Export... → Reset All to Defaults → Close
What changed
FORMAT_VERSIONACTION_IDACTION_NAMECATEGORYSHORTCUTCUSTOMIZEDQKeySequence::PortableText, keeping modifiers portable across platforms (Altfor macOS Option, for example).QSaveFile.Cross-release behavior
The exported file remains useful when moving both forward and backward through AetherSDR releases:
User impact
Operators can back up carefully customized keyboard layouts, clone them between shack computers, and move between macOS, Windows, and Linux without manually recreating every mapping. Import errors are non-destructive: no binding is changed unless the complete file parses and resolves successfully.
Screenshot
Validation
cmake --build build --target AetherSDR shortcut_manager_test -j8ctest --test-dir build -R '^shortcut_manager_test$' --output-on-failurepython3 tools/check_engine_boundary.py --strict— 0 blockers; existing tracked warnings onlypython3 tools/check_a11y.py— existing warning-only findings; no new finding in the changed dialogTest coverage
The shortcut manager test now covers:
👨🏼💻 Generated with OpenAI Codex (GPT-5.6 Sol) and tested by @jensenpat