Conversation
|
| /// key: it sits in the hotkey cluster above/right of the numpad on boards | ||
| /// like the ERGO K860 and MX Keys S, and is absent from the Signature | ||
| /// series. | ||
| /// |
There was a problem hiding this comment.
Configuration schema stays stale
When a KeyCalculator binding is saved and the user subsequently runs an older release, the schema-version-7 file passes the version gate but its new ButtonId key fails deserialization, causing the agent to run with defaults and the desktop to treat the saved configuration as read-only.
There was a problem hiding this comment.
This is real, but it's a pre-existing structural limitation of the whole ButtonId enum, not something this PR introduces: every one of the eight sibling keys this table already models (KeySearch, KeyDictation, KeyEmoji, KeyScreenCapture, KeyMicMute, KeyPlayPause, KeyMute, KeyVolumeDown, KeyVolumeUp) has the exact same forward-compat gap — an older release reading a config.toml with any of those bound would hit the same unknown-variant deserialization failure KeyCalculator would. Config's schema-version gate (mentioned in the config.rs comment about a v1 file still loading) only covers structural/shape migrations, not tolerating an unrecognized enum variant inside an otherwise-valid map.
Fixing that properly means either a custom Deserialize for ButtonId that maps an unrecognized variant name to a catch-all (and a parallel change to how bindings: BTreeMap<ButtonId, Binding> skips rather than fails on one bad entry), or accepting that going back to an older release after binding a newer key is unsupported. That's a real design decision affecting the whole enum family, not specific to adding one more variant to it — out of scope for this PR, which follows the established pattern for this ButtonId family exactly as its predecessors do. Worth a separate issue if a maintainer wants graceful downgrade behavior for the whole family.
There was a problem hiding this comment.
Agreed. The downgrade failure is a systemic consequence of adding any new ButtonId variant to the serde-backed BTreeMap, and the schema-version gate does not claim to provide enum-vocabulary compatibility. KeyCalculator does not introduce a distinct regression beyond the existing keyboard-key family, so this is not actionable for this PR. A separate issue covering the whole ButtonId family is the right scope if graceful downgrade behavior is desired.
dd12ed6 to
be71abe
Compare
Adds `ButtonId::KeyCalculator` and `(0x000a, ButtonId::KeyCalculator)` to `KEYBOARD_KEY_CIDS`, so the Calculator hotkey can be bound like the existing keyboard keys. Closes AprilNEA#1147. Unlike the F-row keys the table was built for, this control sits in the hotkey cluster beside the numpad (ERGO K860, MX Keys S) and is absent from the Signature series. Diversion is the only way to reach it on macOS: with the keyboard in its macOS mode the firmware emits no HID usage at all for this key, so nothing above the HID layer has an event to intercept. In Windows mode the same key natively sends consumer usage 0x0192 (`AL Calculator`). CID and firmware task (`CALCULATOR`) cross-checked against Solaar's `special_keys.py`, the same catalog the existing nine entries cite, and against the Logi asset metadata slot `SLOT_NAME_CALCULATOR`. As for every entry in the table, an unbound key is never diverted, so stock firmware behavior is unchanged unless the user binds it.
be71abe to
bdd0194
Compare
|
Rebased onto current master and opened as #1465 (your branch had drifted into conflict against master's locale/wire changes since this was opened). Authorship preserved via git commit. Thanks for the thorough on-device verification writeup — carried it forward into the new PR body. Closing this one in favor of #1465. |
Adds
ButtonId::KeyCalculatorand(0x000a, ButtonId::KeyCalculator)toKEYBOARD_KEY_CIDS, making the Calculator hotkey bindable like the existing nine keyboard keys.Closes #1147.
Why diversion is the only option here
Unlike the F-row keys the table was built for, this control sits in the hotkey cluster beside the numpad (ERGO K860, MX Keys S) and is absent from the Signature series.
The part that makes this more than a missing table entry: with the keyboard in its macOS mode the firmware emits no HID usage at all for this key, so nothing above the HID layer has an event to intercept. I verified the absence at three levels on an ERGO K860 for Business:
consumer_key_code volume_incrementand the Search keyconsumer_key_code ac_search(0x0221), so the observer was working and that receiver's consumer usages were visible.libinput debug-events --show-keycodes, at raw evdev (evtest, including the receiver's Consumer Control node), and at the raw HID report level — no report arrives, so it is not a missing keycode mapping.0x0192(AL Calculator).Logitech documents this constraint from the other side: their SIGNATURE K650 page lists the "Close browser tab key, Settings key and Calculator key" as requiring Options+ "to work on Mac" while working "out of the box on Windows and Chrome OS", and the K250 page names Calculator as an exception to "Most keys work on Windows, macOS and ChromeOS". Diversion is presumably how Options+ delivers it, and it is the only mechanism available to OpenLogi.
CID and firmware task (
CALCULATOR) cross-checked against Solaar'sspecial_keys.py— the same catalog the existing nine entries cite — and against the Logi asset metadata slotSLOT_NAME_CALCULATOR(slotIdergo-k860-2b364_c10).openlogi diag controlson the K860 reports the control as divertable:As for every entry in the table, an unbound key is never diverted, so stock firmware behavior is unchanged unless the user binds it. In Windows mode, binding it suppresses the native
AL Calculatorusage, which matches how the other keys here behave.KeyCalculatoris appended last in bothButtonIdandKEYBOARD_KEYS, matchingKEYBOARD_KEY_CIDSorder, per the append-only note inbutton.rs. No newActionis needed —RunShellCommand/OpenApplicationalready cover the obvious bindings.Changes
ButtonId::KeyCalculator, growKEYBOARD_KEYSto 10, add the display name and locale key, and default the new key toAction::Noneso it stays native until bound.(0x000a, ButtonId::KeyCalculator)toKEYBOARD_KEY_CIDSand note that this entry is a hotkey beside the numpad rather than an F-row position.PROTOCOL_VERSIONto 31 for the appended variant and update the wire-format pin.calculator_keyto all 23 catalogs, at the same position as inen.toml.Rebased onto master
Rebased from the original base onto
e846e6f4. Conflict resolutions worth flagging for review:PROTOCOL_VERSIONis now 30 → 31. v30 went toAgent::read_wheel/Agent::read_backlightin the meantime, so the v31 line documents this variant instead.calculator_keywere translated on master since the original base. I took master's translated text and supplied best-effort translations forcalculator_keyin all 22 non-English catalogs rather than re-adding the English fill-in, since the Crowdin merge step drops English fill-ins anyway. Native-speaker corrections welcome —zh-HK/zh-TWuse計算機鍵andzh-CN计算器键.openlogi-cli'sprotocol_mismatch_aborts_before_snapshot_or_outputasserts the version numbers as literal strings ("protocol v29"/"requires v30"). That test arrived withe846e6f4, after the original commit, so the bump moved it to v30/v31.Testing
Verified on hardware — ERGO K860 for Business (
ERGO 860B, model id2b364) on a Bolt receiver, macOS on Apple Silicon, keyboard in macOS mode, with:A release build of the patched agent arms the diversion and dispatches the press:
Calculator launches. Before the patch the same keypress produced nothing anywhere in the stack.
Local checks on the rebased tip, with
RUSTFLAGS="-D warnings":cargo fmt --all -- --check— cleancargo clippy --all-targets -- -D warningsforopenlogi-core,openlogi-device,openlogi-ipc,openlogi-agent-core,openlogi-hid,openlogi-hidpp,openlogi-inject,openlogi-hook,openlogi-cli,openlogi-ui,openlogi-agent,openlogi-assets,openlogi-permissions,openlogi-device-registry— cleancargo testfor the same 14 packages — 1225 passed, 0 failedcargo test -p openlogi-ipc --test wire_format— 16 passed, includingprotocol_version_is_pinnedcargo test -p openlogi-ui locale— catalog parity passes;calculator_keypresent at the same position in all 23 catalogsRUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-deps --document-private-itemswith the four GUI/agent exclusions — cleanNot run locally, per the "do not claim a skipped job passed" rule:
cargo test -p openlogi-desktop i18n, and clippy/tests foropenlogi-desktop,openlogi-overlayandopenlogi-camera.gpui_macos's build script needs the Metal shader compiler, which ships only with full Xcode — unavailable on the machine I built on (xcrun: error: unable to find utility "metal"). Those are left to CI. The desktop-side change is limited to the locale catalogs plus the newButtonIdarm, both covered by the portable parity test above.The earlier red macOS-arm64 leg
For reference, since it was red before this force-push and is unrelated to this diff: run 33638862185 attempt 2 failed in
openlogi-agent-core'sruntime::scroll::worker::tests::generation_invalidation_cancels_started_output_out_of_bandwithleft: Ended, right: Changed. Attempt 1 of the same commit was green, and this PR touches no scroll code.That test looks inherently timing-dependent rather than broken by anything here. It receives
Began, then callscancel_hooks(), which bumps the generation atomic and wakes the worker; the worker only cancels at the top of its next loop iteration (worker.rs:425-427).ANIMATION_DURATIONis a fixed 100 ms, so if the animation completes before the worker observes the bump, the engine emitsEnded,cancel_allhas no live motion left to cancel, noCancelledframe is ever emitted, and the assertion that every pre-Cancelledframe isChangedtrips on the queuedEnded. In other words the test gives itself a 100 ms wall-clock budget on a shared runner. I could not reproduce it locally (400/400 idle, 150/150 under saturating CPU load on an M-series host).Happy to open a separate PR threading a controllable clock into
run_worker—ScrollEngine::advance_duealready takes an explicitInstant, so freezing time there would make that test deterministic — but that is unrelated to this change and I did not want to bundle it.Note for review
There is overlap with #1248, which also appends a
ButtonId::KeyCalculatorand maps CID0x000a, alongside Show Desktop, Lock PC and Search plus GUI work. Whichever of the two you prefer, I am happy to rebase this on top of it or close this in its favour — the K860 evidence above applies either way, and it adds one detail #1248 does not cover: on that keyboard there is no OS-level event at all in macOS mode, so diversion is the only possible path rather than a convenience.Happy to add a GUI affordance if wanted — these keyboard slots appear to be config-only today, so this follows the existing pattern.