fix: restore device settings after sleep/wake + home view UI polish - #133
fix: restore device settings after sleep/wake + home view UI polish#133m-wilkowski wants to merge 3 commits into
Conversation
Three related fixes for DPI and other settings not being restored after the laptop wakes from sleep. 1. DeviceSession::enumerateAndSetup() – always reset and recreate m_commandProcessor so it always points to the fresh FeatureDispatcher built at the top of the function. The old guard (if !m_commandProcessor) kept the stale pointer alive after m_features was replaced, causing use-after-free when the CommandProcessor later called m_features->callAsync(). This was latent in the checkSleepWake path and any future caller that re-enumerates without a full disconnect/reconnect cycle. 2. DeviceManager – emit physicalDeviceTransportReady when a new session attaches to an already-existing PhysicalDevice. In that path enumerateAndSetup() emits setupComplete() BEFORE attachTransport() connects the PhysicalDevice signal fan-in, so transportSetupComplete is never emitted and onTransportSetupComplete is never called. The new signal closes that gap (multi-transport setups: Bolt + BT, or rapid udev remove+add where PhysicalDevice is not destroyed). 3. AppRoot – subscribe to the systemd-logind PrepareForSleep DBus signal. On resume (before=false), wait 2 s for HID++ to stabilise and then call onTransportSetupComplete for every known device. This is the primary fix for the common single-transport case where the Bolt receiver does not send a 0x41 reconnect notification after wake, leaving the session connected but with factory-default firmware state.
- HomeView: centered logo + "Logitune" wordmark replaces time-based greeting - logitune-logo.png: new app icon (teal, mouse + L) - logitune-icon.svg: simplified geometry - DeviceCard: 180×280 → 280×420 for better presence at larger window sizes - SideNav: tighten left margin (35→20px) and tab spacing (17→6px) - Theme dark mode: slightly deeper border/input/card/hover colors - SettingsPage: "Active" label uses accent color instead of battery green
There was a problem hiding this comment.
@m-wilkowski Thanks for digging into this — the core of this PR is genuinely valuable, and you've found two real bugs, one of which is a memory-safety issue:
-
Use-after-free in
DeviceSession::enumerateAndSetup. The oldif (!m_commandProcessor && ...)guard meant that on re-enumeration the existingCommandProcessorwas kept while theFeatureDispatcherit points at got rebuilt underneath it — a dangling pointer. Always recreating it is correct. -
The actual sleep/wake root cause. When a transport re-attaches to an existing
PhysicalDevice,physicalDeviceAddedisn't re-emitted, so nothing re-applies the profile. AddingphysicalDeviceTransportReadyis the right shape of fix, and the comment explaining thesetupComplete()-before-attachTransport()ordering is exactly the kind of note that keeps this from regressing.
What I'd like to change: please split this into two PRs.
As it stands the PR bundles those core fixes with unrelated UI/branding work — HomeView.qml, Theme.qml, SideNav.qml margins, DeviceCard.qml, SettingsPage.qml, a replaced logitune-icon.svg and a new logitune-logo.png. That's where nearly all of the 148 deletions live. Two problems with shipping them together:
- The core fixes are objectively verifiable and I want to merge them quickly. The UI/branding changes are subjective and need a different kind of review (and the icon/logo replacement is a branding decision, not just polish).
- If anything regresses, a bisect lands on a commit that changed both device lifecycle handling and the entire home view.
PR 1 — the fix (please open this first, I'll prioritise it):
src/core/DeviceManager.{h,cpp}, src/core/DeviceSession.cpp, and the AppRoot wiring for the new signal.
PR 2 — the UI/branding work: everything under src/app/qml/ plus the asset changes and the tst_SideNav.qml expectation update (that test only changed because of the 17→6 margin change, so it belongs with the UI PR).
One more ask on PR 1: please add tests. Right now the only test touched is the QML nav-margin one, so neither core fix is covered. Both are testable without hardware — tests/services/ProfileOrchestratorFixture.h and tests/helpers/AppRootFixture.h already fake a PhysicalDevice/DeviceSession, so a test can attach a second transport to an existing PhysicalDevice and assert that the profile gets re-applied (i.e. that physicalDeviceTransportReady fires and settings are restored). That's the regression guard that stops this bug coming back after the next refactor.
Heads-up on conflicts: your AppRoot::wireSignals change touches the same region as #148, which is currently open. Whichever lands second will need a rebase — not a problem, just so it isn't a surprise.
Really good find on the dangling pointer especially. Ping me when PR 1 is up and I'll review it straight away.
2 participants
Summary
logitune-logo.pngapp icon (teal, mouse + L)Test plan