Conversation
There was a problem hiding this comment.
@FromSi Thanks for taking a run at this. I validated the branch before reviewing — it rebases cleanly, builds, the lint passes, and the added test is a real assertion rather than a compile-level placeholder, which I appreciate. But there are two things I need resolved before this can go in, one of which affects devices other than the MX Master 4.
1. The control ID looks wrong
The descriptor registers 0x01A0 as the Action Ring. Cross-checking against Solaar's canonical CID table (logitech_receiver/special_keys.py:318):
"Haptic": 0x01A0, # Logitech
"Circle": 0x01A3,
"Triangle": 0x01A4,0x01A0 is documented as Haptic, and neither Solaar nor logid defines any "Action Ring" CID. Our CLAUDE.md asks that HID++ facts come from Solaar/logid rather than being derived independently, precisely because a wrong CID silently diverts the wrong control on real hardware.
Could you share where 0x01A0 came from? If it's from a capture on an actual MX Master 4 (solaar show, or a raw HID++ 0x1b04 control enumeration), please post that output — a real capture beats the table and I'll happily take it. If it was inferred, it needs verifying against the device first.
2. ButtonModel::loadFromProfile grows but never shrinks — this affects other devices
To surface the 9th button, loadFromProfile now extends m_buttons when the incoming list is longer. The problem is that ButtonModel is a single shared instance in AppRoot, not per-device, and the change only ever grows:
- MX Anywhere family: 6 controls
- MX Master 2S / 3 / 3S: 8 controls
- MX Master 4: 9 controls
So selecting the MX Master 4 and then switching to any other device in the same session leaves m_buttons at 9. Rows 7–8 keep stale buttonId/controlId values, rowCount() still reports 9, and ButtonsPage.qml renders phantom button rows for a device that doesn't have them.
The model needs to match the active device's control count in both directions — shrink (or rebuild) on device switch, not just grow. Please also add a test for the shrinking case: the current LoadFromProfileUpdatesData only covers 8→9, so the regression above would pass CI today. tests/helpers/AppRootFixture.h has an addMockDevice helper that makes a switch-to-a-smaller-device test straightforward.
3. Minor: the title oversells the scope
With defaultActionType: "default", ButtonActionDispatcher returns early (if (ba.type == ButtonAction::Default) return;), so this adds a remappable button slot rather than the Action Ring behaviour itself — there's no radial-menu/overlay implementation here, unlike gesture-trigger or smartshift-toggle which have dedicated dispatch and UI. That's completely fine as an incremental step, but please reword the title/description so reviewers don't assume the feature works end to end.
Happy to help with (2) if the ownership model isn't obvious — the cleanest fix is probably rebuilding the model per device rather than mutating it in place. Get me a capture for (1) and this becomes an easy merge.
|
CID 0x01A0 (control), TID 0x0109 (its default task) — captured on real hardware.** The CID in my edit is correct; the label was wrong. Here's what the device actually reports and how I got it. Device: MX Master 4, WPID B042, HID++ 4.5, over a Bolt receiver. 1. Control enumeration —
|
FromSi
changed the title
2 participants
1. Where
0x01A0came fromA capture, not the table. Real MX Master 4, WPID B042, HID++ 4.5, Bolt receiver, solaar 1.1.20:
So the CID is right and matches
special_keys.py:318— the label was wrong. The branch conflated two ID spaces:0x01A0— the physical force-sensing thumb pad, Solaar nameHaptic.0x0109— the task bound to it by default, unnamed in both Solaar and logid."Action Ring" is a behaviour, not a control; no canonical table has a CID by that name. The descriptor now reads
Haptic thumb pad— derived from the CID, matching the neighbouring multi-word labels.2. ButtonModel rebuild
loadFromProfilenow clears and rebuildsm_buttonsto match the incoming list inside onebeginResetModel()/endResetModel()pair, so growth, shrink and content changes take the same path. Public API and signals unchanged.One note on the
addMockDevicesuggestion: it hardcodedsession->m_activeDevice = &m_device, so a second device always had the same control count as the first and the shrink couldn't be expressed at all. It now takes an optional descriptor, withnewMockDescriptor()owning the extras so they outlive the sessions pointing at them;MockDevicegainedsetupMx4Controls()(9 controls) andtruncateControls().Tests:
AppRootFixture.CarouselSwitchToFewerControlsShrinksButtonModel— 9→8 and 9→6 through the real carousel-switch path, assertingrowCount()and that no row keeps abuttonId/controlIdfrom the previous device.LoadFewerThanModelSizeButton0Changes→LoadFewerThanModelSizeShrinksModel: the old test asserted the stale-row behaviour being fixed here.LoadFromProfileUpdatesData(8→9) unchanged and still passing.Both shrink tests were confirmed to fail against the pre-fix
loadFromProfileand pass after it.3. Scope
You're right, and the title is reworded. This adds a remappable button slot, nothing more — no radial menu, no overlay, no dedicated dispatch. The control ships with
defaultActionType: "default"andButtonActionDispatcherreturns early onButtonAction::Default(ButtonActionDispatcher.cpp:124), so nothing happens until the user binds an action to it.Closes #125