Skip to content

Add Nothing Track mod (nothing-track) - #5374

Open
lenorio wants to merge 10 commits into
ramensoftware:mainfrom
lenorio:main
Open

Add Nothing Track mod (nothing-track)#5374
lenorio wants to merge 10 commits into
ramensoftware:mainfrom
lenorio:main

Conversation

@lenorio

@lenorio lenorio commented Sep 5, 2026

Copy link
Copy Markdown

Adds Nothing Track, a native Windows 11 taskbar widget and flyout control for Nothing and CMF earbuds.

It communicates with the earbuds directly over Bluetooth RFCOMM (SPP) profile to display battery status and control ANC, Ultra Bass, EQ presets, Low Latency game mode, and Find My Earbuds without requiring a mobile app or Android emulator.

The PR adds a single file: mods/nothing-track.wh.cpp.

Changelog

If this pull request updates an existing mod, describe the changes below:

  • Initial public release.

Mod authorship

If this pull request introduces a new mod, please complete the section below.

This mod was created by:

  • The submitter, without AI assistance
  • The submitter, with AI assistance
  • Claude
  • ChatGPT
  • Gemini
  • Another AI (please specify):
  • Other (please specify):

Please select the options that best apply. Your selection does not affect the acceptance criteria, but it helps reviewers understand the context of the code and provide relevant feedback.

@windhawk-reviewer windhawk-reviewer Bot added the waiting-for-author The author's turn: request an AI review, or respond to one that was posted. label Sep 5, 2026
@windhawk-reviewer

Copy link
Copy Markdown

Thanks for the pull request! This repository uses a two-stage review: an AI review that you run yourself, followed by a human review.

To get started, comment /ai-review. Once you're happy with the result, comment /ready-for-reviewer to hand it over to a human reviewer.

See the pull request review process for the full details.

@lenorio

lenorio commented Sep 5, 2026

Copy link
Copy Markdown
Author

/ai-review

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-ai-review An AI review was requested and is being prepared. and removed waiting-for-author The author's turn: request an AI review, or respond to one that was posted. labels Sep 5, 2026
@lenorio

lenorio commented Sep 5, 2026

Copy link
Copy Markdown
Author

/ai-review

@windhawk-reviewer

Copy link
Copy Markdown

@lenorio /ai-review can't be applied here: an AI review was already requested, please wait for it to be posted.

@windhawk-reviewer

Copy link
Copy Markdown

Submission review

Note: This review was done by Claude. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding.

Remember: The AI reviewer can be wrong - it may misread code, flag correct code as broken, or suggest changes that make things worse. Treat its findings as suggestions to verify, not instructions to follow blindly. You're responsible for the code you submit, so if a finding doesn't hold up, say so instead of changing working code to satisfy it.

Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them.


Nice, self-contained feature and the taskbar-injection scaffolding follows the established patterns. The main problems are around teardown — several things can outlive Wh_ModUninit or run at process shutdown — plus a couple of concrete bugs in settings handling and widget removal.

1. The BluetoothManager singleton's destructor aborts Explorer at process shutdown. Instance() holds a function-local static BluetoothManager s_instance; whose destructor is registered to run at process exit. Wh_ModUninit does not run when Explorer terminates (restart, sign-out, reboot) — only the CRT destructors do. At that point m_worker is very likely still joinable(), so ~std::thread() calls std::terminate() and takes the host process with it; m_socket / m_service are WinRT objects being released on the shutdown thread after their peers are gone. See https://github.com/ramensoftware/windhawk/wiki/Global-objects-and-process-shutdown (case #1, #3, #9). Suppress the automatic destructor and keep the explicit Stop() in Wh_ModUninit as the only release path:

static BluetoothManager& Instance() {
    [[clang::no_destroy]] static BluetoothManager s_instance;
    return s_instance;
}

Reference: vlc-discord-rpc.wh.cpp#L256.

2. The strong XAML globals need the same treatment. g_injectedGrid, g_injectionParent, g_trackedElement and g_dispatcherTimer are strong Grid/FrameworkElement/DispatcherTimer references. Their automatic destructors run at process exit, on the shutdown thread, after the XAML core is torn down — the UI-thread-affinity case (#4). These are nullable projected types, so the bare attribute is enough:

[[clang::no_destroy]] static Grid g_injectedGrid{nullptr};
[[clang::no_destroy]] static FrameworkElement g_injectionParent{nullptr};
[[clang::no_destroy]] static FrameworkElement g_trackedElement{nullptr};
[[clang::no_destroy]] static DispatcherTimer g_dispatcherTimer{nullptr};

RemoveWidgetGrid() already nulls all four on the controlled path, so nothing else changes. Reference: neiz-supersmile-audio-visualizer.wh.cpp#L8336-L8338. (g_settings, g_earbudsState, g_stateMutex, g_layoutUpdateToken are fine as-is — heap-only or no-op destructors.)

3. Ten std::thread(...).detach() call sites — no join point. Every flyout action spawns a detached thread that calls into BluetoothManager and blocks on .get() over RFCOMM (which can take seconds, or hang if the buds stop responding). If the mod is disabled/updated while one is in flight, the thread is still executing mod code when Windhawk FreeLibrarys the image — its instruction pointer and return address live in the unmapped mod, which crashes Explorer regardless of what state it touches. Windhawk requires that no mod code is running or scheduled once Wh_ModUninit returns.

The cleanest fix here is to not spawn threads at all: give BluetoothManager a command queue that the existing worker drains (it already owns an MTA apartment, which the detached threads don't initialize), so UI clicks just enqueue. If you'd rather keep the per-action threads, track them and join them in Wh_ModUninit — see the RunAsync / g_asyncTasks pattern in vlc-discord-rpc.wh.cpp#L258-L282 and its join loop at #L2613-L2619.

4. Wh_ModInit starts the worker thread and can then return FALSE. BluetoothManager::Instance().Start() runs before HookTaskbarDllSymbols(), and if hooking fails (Windows 10 — no taskbar.dll; symbol resolution failure on a new build) the mod returns FALSE and gets unloaded without Wh_ModUninit ever running, leaving the worker thread executing in an unmapped image. Do the hooking first and only start the Bluetooth worker once init is guaranteed to succeed — or move Start() to Wh_ModAfterInit.

5. DispatcherTimers and the open flyout are not torn down on unload.

  • ApplySettingsWithRetry chains up to 50 × 100 ms retry timers. g_unloading is checked at the top of the function, but the pending timer itself is never stopped, so its Tick handler (mod code) can fire up to ~5 s after Wh_ModUninit returns.
  • FlyoutContext::ringTimerL / ringTimerR are 5-second DispatcherTimers started by the Find-My-Earbuds buttons. A started DispatcherTimer is rooted by the framework, so it survives the widget being removed from the tree and will fire into unmapped code.
  • If the flyout is open when the mod is disabled, ShowAt has rooted it in the XamlRoot's popup tree — removing the widget button does not release it, so its Click / Toggled / Opened / Closed handlers stay live.

RemoveWidgetGrid() should keep a handle on the pending retry timer and stop it, stop both ring timers, and Hide() the flyout (or Flyout::SetAttachedFlyout / detach it) so the whole subtree is released before the image goes away.

6. RemoveWidgetGrid() decides how the widget was injected from the current setting, not from how it was actually injected. isTrackingPosition is recomputed from g_settings.position, but Wh_ModSettingsChanged calls LoadSettings() before ApplySettings(), so on a settings change the check reflects the new position while the tree still holds the old injection. Two concrete failures:

  • old = a taskbar_* overlay position, new = a tray_* position → isTrackingPosition is false and widgetCol is 0 (the overlay widget had Grid::SetColumn(widgetGrid, 0)), so the code shifts every child of the taskbar RootGrid and deletes its column 0 — the taskbar layout is mangled until Explorer restarts.
  • old = a tray_* position, new = a taskbar_* position → the ColumnDefinition inserted into SystemTrayFrameGrid is never removed, leaving a stray column (and shifted siblings) behind on every such change.

You already record the right thing at injection time — g_injectedColumn is -1 for overlay positions and the column index otherwise — it's just never read. Use it:

auto targetGrid = g_injectionParent.try_as<Grid>();
int widgetCol = g_injectedColumn;
RemoveWidgetGridChildren(targetGrid);
if (widgetCol >= 0 && targetGrid && widgetCol < (int)targetGrid.ColumnDefinitions().Size()) {
    ...
}

7. The paired-device match is far too loose, and the mod sends proprietary command packets to whatever it picks. WorkerLoop accepts any paired Bluetooth device whose name contains nothing, cmf, buds or ear — that matches Galaxy Buds, JBL/Anker "…Earbuds", anything with "Gear"/"Bear"/"Ear" in the name. Worse, the fallback just takes devices.GetAt(0) from every device exposing the SPP UUID (serial adapters, OBD dongles, printers…). It then opens RFCOMM and writes Nothing/CMF vendor frames at it, including the Find-My-Earbuds ring command. Please narrow this: match on the Nothing/CMF Bluetooth vendor/OUI or the exact device names you support, and validate the first response (e.g. the serial/firmware reply and its CRC) before treating the device as supported. Bailing out is much better than talking to a stranger's earbuds.

8. hideDisconnectedBuds is a boolean setting read with Wh_GetStringSetting. The settings block declares - hideDisconnectedBuds: true, so the inferred type is boolean and the read must be Wh_GetIntSetting:

g_settings.hideDisconnectedBuds = Wh_GetIntSetting(L"hideDisconnectedBuds");

The current string read plus wcscmp(hideVal, L"0") != 0 && _wcsicmp(hideVal, L"false") != 0 is not a reliable mapping, and since Wh_GetStringSetting never returns NULL (it returns L"" on error/unset) the else branch is dead — an empty result silently yields true, so unchecking the box may have no effect.

9. Settings UI strings default to Russian. Mod names, descriptions and settings $name/$description should be English by default, with other languages supplied via the localization suffix. Right now displayFormat, pollInterval, language and hideDisconnectedBuds have Russian as the primary $name/$description and English only as :en-US, and marginSide has no English text at all:

- marginSide: "4 4"
  $name: "Отступы виджета (слева справа)"
  $description: "Отступ в пикселях: левый и правый через пробел (по умолчанию 4 4)"

Please flip them so the base strings are English and add $name:ru-RU / $description:ru-RU. Same for the $options of displayFormat and language, which currently pack both languages into one label ("Компактный (Две батареи: L и R) / Compact (Dual battery)") — that should be an English $options plus a separate $options:ru-RU. The position setting already does this correctly and is a good template.

10. Disabling the mod can hang for a long time. Wh_ModUninitStop()join(), but the worker may be parked in a blocking DeviceInformation::FindAllAsync(...).get() or socket.ConnectAsync(...).get(), neither of which Disconnect() can interrupt (the socket doesn't exist yet). A Bluetooth connect attempt to an out-of-range device can take tens of seconds, and Windhawk's unload/update blocks for the whole duration. Keep the in-flight IAsyncOperation in a member and Cancel() it from Stop() before joining, or at minimum use wait_for and bound the wait.

11. The README has no screenshot. This mod's whole value is a visible taskbar widget and a fairly elaborate flyout, so please add at least one image of each (i.imgur.com or raw.githubusercontent.com are the allowed hosts). It also makes the placement options much easier to understand.

Optional improvements

Minor polish — none of this affects users, so it's your call.

  • Unused compiler options and includes. Nothing in the mod calls WinINet, shlwapi, GDI, DWM or Shcore APIs, and <shellapi.h> is unused. -lwininet in particular reads like a leftover — worth confirming it's not a remnant of something that contacted a server, since mods must be fully self-contained. Suggested: -lole32 -loleaut32 -lruntimeobject -luuid -luser32 -lwindowsapp. In the other direction, <memory> (std::make_shared, shared_ptr, weak_ptr) and <cstdint> are used but only reachable transitively.

  • g_settings is written and read concurrently. LoadSettings() runs on the Windhawk engine thread while the 1-second DispatcherTimer reads g_settings.displayFormat on the UI thread and the Bluetooth worker reads g_settings.pollInterval. Reassigning a std::wstring frees the old buffer under the reader. It only happens on a settings change, hence optional, but a std::mutex around g_settings (or a shared_ptr<const ModSettings> swapped atomically) would close it.

  • g_unloading should be std::atomic<bool> — it's written from the engine thread and read from the UI thread and the LayoutUpdated handler.

  • A couple of unlocked reads of g_earbudsState: ProcessPacket reads g_earbudsState.left.present / .right.present at the single-bud check before taking g_stateMutex a few lines below, and the Game Mode Click handler reads g_earbudsState.lowLatencyEnabled outside the lock before taking it.

  • Use WindhawkUtils::StringSetting instead of Wh_GetStringSetting + manual Wh_FreeStringSetting, and drop the if (pos) / if (disp) / if (lang) / if (margins) guards — Wh_GetStringSetting never returns NULL.

  • marginSide as a space-separated string parsed with swscanf_s means a partial input ("8") leaves marginRight at whatever it was previously. Two number settings (marginLeft, marginRight) would be simpler and self-validating.

  • pollInterval isn't clamped to the documented range. The description says 10–120 s, but only the lower bound is enforced (std::max(10, ...)); a typo'd 10000 gives a ~3-hour interval.

  • Dead code: CSecondaryTaskBand_ITaskListWndSite_vftable and CSecondaryTaskBand_GetTaskbarHost_Original are resolved and GetTaskbarXamlRoot handles Shell_SecondaryTrayWnd, but FindCurrentProcessTaskbarWnd() only ever returns Shell_TrayWnd, so the secondary path never runs. Either drop the two hooks or extend the widget to secondary taskbars. g_injectedColumn is likewise only written (see item 6 above for a use).

  • Wh_ModAfterInit calls ApplySettings() directly rather than ApplySettingsWithRetry(). When the mod is enabled mid-session that's usually fine, but a single failed attempt means no widget until the next settings change; reusing the retry path costs nothing.

  • RunFromWindowThread has no timeout on its SendMessageW. If the taskbar thread is wedged, Wh_ModUninit blocks indefinitely. taskbar-ai-quota.wh.cpp takes a timeoutMs parameter for exactly this.

  • The Loc() table hardcodes English and Russian only. That's fine as a starting point, but a std::wstring return for every lookup on a 1-second UI timer is a lot of churn — const wchar_t* would do, since all the strings are literals.

Functionality notes

Non-critical observations and ideas about the feature behavior itself.

  • Battery polling can stall. The poll-interval check sits at the top of the reader loop, but the loop then blocks in reader.LoadAsync(1).get() until a byte arrives. If the buds go quiet (no unsolicited notifications), the loop never returns to the poll check, so QueryBattery() is never sent and the reading freezes at its last value until something else wakes the socket. A separate poll thread/timer that calls QueryBattery() on the interval — or a bounded read — would make the interval setting actually authoritative.

  • Incoming CRC is read but never verified. ProcessPacket consumes crcLo/crcHi and discards them. Since ComputeCrc16 already exists, checking it (and resyncing on mismatch) would guard against desync producing bogus battery/ANC values — and it would double as the "is this really a Nothing device" check from item 7 above.

  • SetEq fires two different commands unconditionally. SendCommand(61469, ...) then ok |= SendCommand(61456, ...) sends both the new and legacy EQ opcodes to every device, and ok is true if either succeeded — so the flyout shows the preset as applied even when the device only understood the other opcode (or neither did anything). Worth gating on the detected model, or at least on the response.

  • Battery lastSeen timeouts only run inside the battery-packet handler, so the "not seen for 35 s" cleanup can't fire while no battery packets are arriving — which is exactly the situation it's meant to detect.

  • single_active display picks the right bud when neither is present: bool showRight = state.right.present || !state.left.present; — with both absent this shows the right group with a dash rather than falling back to the case or to the status text like the compact path does.

  • MessageBeep(MB_ICONWARNING) before ringing is a nice touch for the in-ear warning, but it plays a system sound on every ring, including the harmless case. Consider only beeping on the in-ear path, or replacing it with the existing in-flyout warning badge.

  • flyout.Closed sends stop-ring for both buds on every close, even when nothing was ringing — two RFCOMM writes per flyout dismissal. Gating on isRingingL/isRingingR would avoid the traffic.

  • Overlap note (FYI, not a blocker): bt-battery-monitor already shows generic Bluetooth device battery levels in the tray. Your mod is clearly distinct — vendor-specific protocol, ANC/EQ/bass control, XAML taskbar widget rather than a tray icon — so this is just worth a sentence in the README so users understand which one they want.


Next steps:

  • /ai-review - after pushing fixes, to get a review of the updated code. You can repeat this as many times as you need, but each review is thorough and usually there's no need for more than 2-3 iterations.
  • /ready-for-reviewer - once you're satisfied with the state of the pull request, to hand it over to a human reviewer. If some findings above are left unaddressed, add a short note explaining why.

See the review process for details.

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-author The author's turn: request an AI review, or respond to one that was posted. and removed waiting-for-ai-review An AI review was requested and is being prepared. labels Sep 5, 2026
@lenorio

lenorio commented Sep 5, 2026

Copy link
Copy Markdown
Author

/ai-review

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-ai-review An AI review was requested and is being prepared. and removed waiting-for-author The author's turn: request an AI review, or respond to one that was posted. labels Sep 5, 2026
@windhawk-reviewer

Copy link
Copy Markdown

Submission review

Note: This review was done by Claude. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding.

Remember: The AI reviewer can be wrong - it may misread code, flag correct code as broken, or suggest changes that make things worse. Treat its findings as suggestions to verify, not instructions to follow blindly. You're responsible for the code you submit, so if a finding doesn't hold up, say so instead of changing working code to satisfy it.

Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them.


Nice work overall — the taskbar/XAML integration follows the established pattern (taskbar.dll symbol hooks, TrayUI::StartTaskbar re-injection, RunFromWindowThread, LoadLibraryExW with LOAD_LIBRARY_SEARCH_SYSTEM32), the settings block matches what the code reads, and there's a screenshot on an allowed host. The findings below are mostly in the Bluetooth layer and in teardown.

1. The Refresh button runs blocking Bluetooth I/O on the Explorer UI thread

refreshBtn.Click([](auto const&, auto const&) {
    BluetoothManager::Instance().QueryAll();
});

QueryAll() issues 8 SendCommand() calls — each one takes m_socketMutex and blocks on StoreAsync().get() / FlushAsync().get() — plus 7 × sleep_for(40ms) of hard sleep. That's ≥280 ms of guaranteed UI-thread freeze on every click, and much longer if the worker thread happens to be holding m_socketMutex in a send that's timing out (the whole taskbar hangs until the Bluetooth stack gives up). Every other control in the flyout already does the right thing — route this one through the action queue too:

refreshBtn.Click([](auto const&, auto const&) {
    BluetoothManager::Instance().PostAction([]() {
        BluetoothManager::Instance().QueryAll();
    });
});

2. InputStreamOptions::Partial breaks packet framing

DataReader reader(socket.InputStream());
reader.InputStreamOptions(InputStreamOptions::Partial);
...
if (reader.LoadAsync(7).get() < 7) break;
...
uint32_t toRead = pLen + 2;
if (reader.LoadAsync(toRead).get() < toRead) break;

With Partial, LoadAsync completes as soon as one or more bytes are available, so it routinely returns fewer bytes than requested when a packet is split across RFCOMM segments. Each short read hits the break, which drops out of the reader loop → Disconnect() → a full 5-second reconnect cycle, with battery/ANC state resetting to "disconnected" in the widget. The default InputStreamOptions::None is what you want here — it waits until the requested count has been read. Drop the InputStreamOptions(Partial) line (or set it to None).

3. The Bluetooth stack starts in every explorer.exe process, not just the shell one

Wh_ModAfterInit calls BluetoothManager::Instance().Start() unconditionally, before it checks whether this process even owns a taskbar. @include explorer.exe matches every explorer.exe — and users who enable "Launch folder windows in a separate process" (or start explorer.exe <path> from Run) get several. Each of those processes then spins up two threads, enumerates paired Bluetooth devices every 3 seconds forever, and races the shell process for the single SPP connection the earbuds accept. Gate the start on actually being the taskbar process:

void Wh_ModAfterInit() {
    g_taskbarWnd = FindCurrentProcessTaskbarWnd();
    if (!g_taskbarWnd) {
        return;  // not the shell process, nothing to do here
    }
    BluetoothManager::Instance().Start();
    RunFromWindowThread(g_taskbarWnd, ...);
}

(You'd also want to start it from TrayUI_StartTaskbar_Hook for the case where the mod loads before the taskbar exists.)

4. Unload can block for a long time — the WinRT async operations are never cancelled

Wh_ModUninitStop()Disconnect() + join(). But nothing cancels the operations the worker thread may be sitting in:

  • DeviceInformation::FindAllAsync(...).get() / BluetoothDevice::FromIdAsync(...).get() / GetRfcommServicesForIdAsync(..., Uncached).get() — seconds each.
  • socket.ConnectAsync(...).get()socket is a local at that point and isn't stored in m_socket until after the connect returns, so Disconnect() can't close it. A connect to an unresponsive/out-of-range device can take tens of seconds.
  • Disconnect() itself first has to acquire m_socketMutex, which SendCommand holds across a blocking StoreAsync().get().

So disabling/updating the mod can wedge Windhawk's unload for a long time. Keep the in-flight IAsyncOperation/IAsyncAction in a member and Cancel() it from Stop(), and publish the socket into m_socket before ConnectAsync so Disconnect() can close it out from under the pending connect.

5. Teardown can leave callbacks alive after the mod image is unloaded

Two paths where a callback that lives in the mod image can survive Wh_ModUninit:

  • TrayUI_StartTaskbar_Hook clears g_layoutUpdateToken = {} (along with the other globals) without revoking the handler from the grid it was registered on. If the taskbar is re-created while the XAML root survives, the old LayoutUpdated handler stays registered and is never revoked on unload → it fires into unmapped code on the next layout pass. Call RemoveWidgetGrid() at the top of the hook instead of zeroing the globals by hand — it already revokes the token, stops both timers, and restores the tracked element's margin.
  • Wh_ModUninit ignores the return value of RunFromWindowThread. If it returns false (no taskbar window found, or SetWindowsHookExW fails), RemoveWidgetGrid() never runs, so g_dispatcherTimer keeps ticking into UpdateWidgetUi() and the LayoutUpdated handler stays registered — both crash Explorer once Windhawk FreeLibrarys the mod. At minimum, check the result and log/retry; the timer and the layout handler are the two things that must be gone before Wh_ModUninit returns.

6. Mention how this relates to the existing Bluetooth battery mod

BT Battery Monitor already shows Bluetooth device battery levels in the tray. Your mod is clearly different in substance (vendor RFCOMM protocol, ANC/EQ/bass/latency controls, a taskbar XAML widget rather than a tray icon), but the battery-display half overlaps, and users will hit both when searching. A sentence in the README saying what this does that the generic mod can't would help.

Optional improvements

Minor polish — none of this affects users in normal operation, so it's your call.

  • g_settings is written from one thread and read from others. Wh_ModSettingsChanged calls LoadSettings() directly on the Windhawk thread, while the 1-second DispatcherTimer on the taskbar UI thread reads g_settings.displayFormat / g_settings.position and the Bluetooth worker reads g_settings.pollInterval. Concurrent read/write of a std::wstring is UB and can crash. Easiest fix: move LoadSettings() inside the RunFromWindowThread callback so it runs on the UI thread, and make pollInterval a std::atomic<int>.
  • ProcessPacket reads shared state before taking the lock. In the ANC branch, g_earbudsState.left.present ^ g_earbudsState.right.present is evaluated before std::lock_guard<std::mutex> lock(g_stateMutex) a few lines below. Move the lock up to cover the whole branch.
  • [[clang::no_destroy]] static BluetoothManager s_instance; — the attribute is correct and necessary here (without it, ~std::thread on a still-joinable m_worker/m_actionWorker calls std::terminate() at process shutdown, crashing Explorer on every sign-out), but the bare form is only recommended for nullable/handle-like types. For a plain class the documented shape is the std::optional wrapper, so the release is explicit: [[clang::no_destroy]] static std::optional<BluetoothManager> s_instance;, constructed on first use and reset() in Wh_ModUninit after Stop(). See Global objects and process shutdown. (Your XAML globals — g_injectedGrid, s_currentFlyout, g_dispatcherTimer, … — are correctly using the bare form, since WinRT projected types are nullable.)
  • WindhawkUtils::StringSetting instead of raw get/free. LoadSettings does Wh_GetStringSetting + Wh_FreeStringSetting four times; the RAII wrapper is shorter and exception-safe: g_settings.position = WindhawkUtils::StringSetting::make(L"position").get();
  • The 1-second timer re-walks the visual tree every tick. UpdateWidgetUi() does nine recursive FindChildByName searches and re-applies ToolTipService::SetToolTip on every tick, even when nothing changed. Store the TextBlock/StackPanel references in a struct when you build them in BuildWidgetGrid() (you already do exactly this for the flyout via FlyoutContext), and only set the tooltip when the string differs from the previous one.
  • Dead code. StringId::InEarWarning, ConnectToAdjust, FindBuds, LowLatency and Stop are declared and translated but never displayed. SubBtnInfo is a byte-for-byte duplicate of AncBtnInfo.
  • The #ifndef WH_MOD_ID / #ifndef WH_MOD_VERSION block at the top is dead — Windhawk always defines both on the compiler command line, so the fallbacks never take effect and would silently go stale if the version ever diverged.
  • Missing include: std::abs on a double (in the LayoutUpdated handler) comes from <cmath>, which isn't included — it currently works only transitively.
  • marginSide parse failures leave stale values. swscanf_s(margins, L"%lf %lf", ...) doesn't have its return value checked, so malformed input silently keeps whatever the previous settings load put there rather than falling back to 4 4.
  • The retry timer leaks itself. In ApplySettingsWithRetry, the Tick lambda captures timer by value and is registered on that same timer — a reference cycle. It's broken when the tick fires, but if the mod unloads while a retry is pending, RemoveWidgetGrid() stops it and drops g_retryTimer while the DispatcherTimer object itself is never released. Capture a weak_ref (or just the token) instead.
  • No @license. The mod borrows the GetTaskbarXamlRoot / RunFromWindowThread helpers from existing mods; declaring a compatible SPDX identifier makes the provenance explicit.

Functionality notes

Non-critical observations and ideas about the feature behavior itself.

  • Battery glyph arithmetic looks off by one, and the charging glyphs land in the wrong family. In Segoe MDL2 / Fluent, Battery0Battery10 are E850E85A, BatteryCharging0BatteryCharging9 are E85BE864, and MobBatteryCharging0MobBatteryCharging10 are EBABEBB5 (see the mapping table in taskbar-tray-system-icon-tweaks.wh.cpp). Your GetBatteryGlyph:
    • non-charging returns 0xE850 + (step - 1) for step 1–10, i.e. Battery0Battery9 — so a 100% bud renders the 90% glyph, and Battery10 is never used. 0xE850 + step over step 0–10 gives the right mapping (and makes the step == 0 special case unnecessary).
    • charging returns 0xEBB5 + step, which starts at MobBatteryCharging10 (full) for step == 0 and then walks into MobBatterySaver0MobBatterySaver9 for step 1–10 — inverted and the wrong icon family. 0xEBAB + step (or step == 10 ? 0xEBB5 : 0xE85B + step if you want the desktop family) is what you're after.
  • The packet length field is written as 16-bit but read as 8-bit. BuildPacket emits len & 0xFF followed by a 0x00 high byte, but the reader treats the second byte as a constant (b6) and computes toRead = pLen + 2. Any response with a payload ≥ 256 bytes desyncs the stream permanently (the serial-number CSV is the plausible candidate). Consider uint16_t pLen = b5 | (b6 << 8);.
  • The poll interval is only honored when the earbuds send something. The interval check sits at the top of the reader loop, but the loop then blocks in reader.LoadAsync(...).get(). If the buds go quiet, QueryBattery() is never issued, no matter what pollInterval says. Driving the poll from a separate timer (posting through PostAction) would decouple the two.
  • Ringing continues if the mod is disabled mid-ring. Wh_ModUninit calls Stop() before the UI teardown, and PostAction early-returns once m_stopRequested is set — so the FindBuds(..., false) that RemoveWidgetGrid() queues is silently dropped and the earbud keeps beeping. Sending the stop synchronously (before Stop()) would handle it.
  • The README's in-ear protection claim doesn't match the code. "with in-ear detection to avoid accidental loud sound in your ear" — the ring handler just picks a different MessageBeep tone and rings anyway; StringId::InEarWarning is never shown anywhere. Either wire up the warning (e.g. require a second click when the bud reads as present) or soften the README wording.
  • SetEq fires two commands unconditionally. SendCommand(61469, ...) followed by ok |= SendCommand(61456, ...) means every EQ change also sends the other model's command to the device. Is the shotgun intentional, or would it be better to pick based on the model detected from the serial prefix you already parse?
  • Device selection is a name substring match with no way to choose. "nothing" / "cmf" / "ear (" / "ear(" picks the first paired match, so a user with two Nothing devices can't select which one, and a non-Nothing device that happens to match gets proprietary command packets sent to it. A deviceName setting (empty = auto) would cover both.
  • Only the primary taskbar gets the widget. GetTaskbarXamlRoot has a Shell_SecondaryTrayWnd branch, but FindCurrentProcessTaskbarWnd only ever looks for Shell_TrayWnd, so the secondary-taskbar support is unreachable. Worth either wiring up or removing the dead branch.
  • The LayoutUpdated handler is fairly hot. For the taskbar_*_start / taskbar_after_* positions it runs TransformToVisual + margin writes on every layout pass of the taskbar root grid, and its own margin writes re-trigger layout (the 1px tolerance is what keeps it converging). There isn't an obviously better hook for "track a neighbouring element", so this is just an FYI — but it's worth keeping an eye on with a busy taskbar.


Next steps:

  • /ai-review - after pushing fixes, to get a review of the updated code. You can repeat this as many times as you need, but each review is thorough and usually there's no need for more than 2-3 iterations.
  • /ready-for-reviewer - once you're satisfied with the state of the pull request, to hand it over to a human reviewer. If some findings above are left unaddressed, add a short note explaining why.

See the review process for details.

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-author The author's turn: request an AI review, or respond to one that was posted. and removed waiting-for-ai-review An AI review was requested and is being prepared. labels Sep 5, 2026
@lenorio

lenorio commented Sep 5, 2026

Copy link
Copy Markdown
Author

/ready-for-reviewer

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-reviewer Ready for a human reviewer, and in the queue for one. and removed waiting-for-author The author's turn: request an AI review, or respond to one that was posted. labels Sep 5, 2026
@m417z

m417z commented Sep 7, 2026

Copy link
Copy Markdown
Member

Thanks for the submission.

  • How about implementing suggestion 3 from the last review?
  • How about implementing this to avoid memory leaks: [[clang::no_destroy]] static BluetoothManager s_instance; — the attribute is correct and necessary here (without it, ~std::thread on a still-joinable m_worker/m_actionWorker calls std::terminate() at process shutdown, crashing Explorer on every sign-out), but the bare form is only recommended for nullable/handle-like types. For a plain class the documented shape is the std::optional wrapper, so the release is explicit: [[clang::no_destroy]] static std::optional<BluetoothManager> s_instance;, constructed on first use and reset() in Wh_ModUninit after Stop(). See Global objects and process shutdown. (Your XAML globals — g_injectedGrid, s_currentFlyout, g_dispatcherTimer, … — are correctly using the bare form, since WinRT projected types are nullable.)
  • Why the screenshot doesn't show the taskbar and has an error message?

@m417z m417z added waiting-for-author The author's turn: request an AI review, or respond to one that was posted. and removed waiting-for-reviewer Ready for a human reviewer, and in the queue for one. labels Sep 7, 2026
@lenorio

lenorio commented Sep 11, 2026

Copy link
Copy Markdown
Author

/ai-review

@windhawk-reviewer windhawk-reviewer Bot added the waiting-for-ai-review An AI review was requested and is being prepared. label Sep 11, 2026
@windhawk-reviewer windhawk-reviewer Bot removed the waiting-for-author The author's turn: request an AI review, or respond to one that was posted. label Sep 11, 2026
@windhawk-reviewer

Copy link
Copy Markdown

Submission review

Note: This review was done by Claude. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding.

Remember: The AI reviewer can be wrong - it may misread code, flag correct code as broken, or suggest changes that make things worse. Treat its findings as suggestions to verify, not instructions to follow blindly. You're responsible for the code you submit, so if a finding doesn't hold up, say so instead of changing working code to satisfy it.

Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them.


Good iteration — most of the previous round is genuinely fixed: Start() is now gated on actually being the taskbar process, the async operations are tracked and cancelled on Stop(), the socket is published before ConnectAsync, InputStreamOptions::None fixed the framing, incoming CRC is verified, RemoveWidgetGrid() uses g_injectedColumn, hideDisconnectedBuds is read with Wh_GetIntSetting, pollInterval is clamped, the settings block is English-first with :ru-RU overrides, the manager is a [[clang::no_destroy]] std::optional<> released in Wh_ModUninit, and the screenshots now show the widget in context. Three things left on the required list.

1. BluetoothManager::Instance() and Start() are not thread-safe, and both are reachable from two threads

static BluetoothManager& Instance() {
    auto& instance = InstanceStorage();
    if (!instance) instance.emplace();       // check-then-act, unsynchronized
    return *instance;
}

void Start() {
    if (m_running.load()) return;            // check-then-act, unsynchronized
    ...
    m_worker = std::thread([this]() { WorkerLoop(); });

Start() is called from Wh_ModAfterInit (Windhawk engine thread) and from TrayUI_StartTaskbar_Hook (taskbar UI thread). Those can overlap — enabling the mod while the taskbar is being re-created is exactly that window. If both pass the m_running check, the second one move-assigns over an already-joinable std::thread, which calls std::terminate() and takes Explorer down. The same pattern in Instance() can double-emplace() the optional.

Both are cheap to close:

void Start() {
    if (m_running.exchange(true)) return;   // atomic claim
    m_stopRequested.store(false);
    m_worker = std::thread([this]() { WorkerLoop(); });
    m_actionWorker = std::thread([this]() { ActionLoop(); });
}

and a mutex (or std::call_once) around the emplace() in Instance(). Note Stop() currently sets m_running back to false at the end, so with exchange you'd want it to stay consistent with the "already claimed" meaning.

2. The idle path enumerates every paired Bluetooth device every 3 seconds, forever

WorkerLoop runs DeviceInformation::FindAllAsync(BluetoothDevice::GetDeviceSelectorFromPairingState(true)), walks the whole paired-device list, and — when the buds aren't connected — sleeps 3 s and does it all again, for as long as Explorer lives. For a user who wears the earbuds an hour a day, that's a PnP/Bluetooth enumeration in the shell process roughly 28,000 times a day doing nothing.

There's an event-driven alternative here, so this isn't a "no better option" situation: resolve the BluetoothDevice once and subscribe to BluetoothDevice::ConnectionStatusChanged to learn when it comes back, and/or run a DeviceWatcher over the RFCOMM selector (RfcommDeviceService::GetDeviceSelector(...)) and react to Added/Removed instead of re-enumerating. The worker then parks on an event/condition variable and costs nothing while the buds are off. If you'd rather keep it simple, at minimum cache the matched device across iterations and back the retry interval off (3 s → 15–30 s) after the first few failures.

3. Credit the taskbar-integration code that this mod reuses, and add an @license

The whole taskbar/tray injection layer — the 25 position $options and their labels, RunFromWindowThread, IsReadableMemoryRange, GetTaskbarXamlRoot, FindElementInRepeater / FindNthElementByClassName / kStartButtonNames, ResolveInjectionTarget, and the body of RemoveWidgetGrid — is a near-verbatim copy of taskbar-fluent-media-player.wh.cpp by Salyts (right down to g_trackPosition == L"far_left" at line 2831, a value this mod never assigns — trackSide is only ever "left" or "right"). That's fine as reuse, but please add a credit line in the README saying where it came from, and declare an @license (a valid SPDX identifier) so the provenance is explicit. Windhawk's authorship model is one author per mod, so the extra credit belongs in the README, not in the metadata.

Optional improvements

Minor polish — none of this affects users in normal operation, so it's your call.

  • g_settings is written on the engine thread and read on two others. Wh_ModSettingsChanged calls LoadSettings() directly, while the 1-second DispatcherTimer reads g_settings.displayFormat / hideDisconnectedBuds on the taskbar thread, the LayoutUpdated handler reads marginLeft/marginRight, IsRussian() reads language, and the Bluetooth worker reads pollInterval. Reassigning a std::wstring under a concurrent reader is UB. Simplest fix: move LoadSettings() inside the RunFromWindowThread callback so it runs on the UI thread, and make pollInterval a std::atomic<int>.

  • The retry timer's Tick token is never revoked, so it leaks itself and the XAML root. In ApplySettingsWithRetry, the tick lambda captures timer by value and is registered on that same timer — a reference cycle that's only broken when the tick actually fires (timer.Tick(*tickToken)). RemoveWidgetGrid() only calls Stop() and drops g_retryTimer, so if the mod unloads (or the taskbar restarts) with a retry pending, the DispatcherTimer, the delegate (whose code lives in the mod image), and the captured xamlRootContent — i.e. the entire taskbar XAML root — are leaked permanently, once per occurrence. Keep the token in a global next to g_retryTimer and revoke it where you stop the timer, the way you already do for g_timerToken.

  • ProcessPacket reads shared state before taking the lock. In the ANC branch, g_earbudsState.left.present ^ g_earbudsState.right.present (line 676) is evaluated before std::lock_guard<std::mutex> lock(g_stateMutex) a few lines below. Move the lock up to cover the whole branch.

  • UpdateWidgetUi() re-walks the visual tree every second. Nine recursive FindChildByName calls per tick, plus an unconditional ToolTipService::SetToolTip(btn, box_value(tooltip)) — which replaces the tooltip object every second even when the text is identical, and can make the tooltip flicker while the pointer is resting on the widget. You already do the right thing for the flyout via FlyoutContext; store the TextBlock/StackPanel references the same way when you build the widget in BuildWidgetGrid(), and only set the tooltip when the string changed.

  • WindhawkUtils::StringSetting instead of raw get/free. LoadSettings does Wh_GetStringSetting + Wh_FreeStringSetting four times, each behind an if (ptr) that can never be false — Wh_GetStringSetting never returns NULL, it returns L"". g_settings.position = WindhawkUtils::StringSetting::make(L"position").get(); is shorter and exception-safe.

  • swscanf_s(margins, L"%lf %lf", ...) return value is unchecked, so "8" or garbage silently leaves one or both margins at whatever the previous load put there instead of falling back to 4 4. Two number settings (marginLeft, marginRight) would be simpler and self-validating.

  • InjectWidget() failure isn't retried. ApplySettingsWithRetry only retries while SystemTray.SystemTrayFrame / SystemTrayFrameGrid are missing, but the default tray_before_clock position additionally needs NotificationCenterButton; if ResolveInjectionTarget can't find its anchor, InjectWidget() returns false, the return value is dropped in ApplySettings(), and the widget silently never appears. Folding "did the injection succeed" into the retry condition would make startup more robust.

  • Dead code. StringId::InEarWarning, ConnectToAdjust, FindBuds, LowLatency and Stop are declared and translated but never displayed. SubBtnInfo is a byte-for-byte duplicate of AncBtnInfo. The column-insert else branch in InjectWidget (line 3030) is unreachable — every non-tray target is a taskbar_* position, which always takes the edge/tracking path. GetTaskbarXamlRoot has a full Shell_SecondaryTrayWnd branch (plus two CSecondaryTaskBand symbol hooks) but FindCurrentProcessTaskbarWnd() only ever returns Shell_TrayWnd, so secondary taskbars are never reached — worth either wiring up or removing.

  • The #ifndef WH_MOD_ID / #ifndef WH_MOD_VERSION block at the top is dead — Windhawk always defines both on the compiler command line, so the fallbacks never take effect and would silently go stale if the version diverged.

  • Missing include: std::abs on a double (in the LayoutUpdated handler, line 2994) comes from <cmath>, which isn't included — it only works transitively today.

  • Loc() returns a std::wstring by value for what are all string literals; roughly 20 heap allocations per timer tick. const wchar_t* would do.

Functionality notes

Non-critical observations and ideas about the feature behavior itself.

  • The battery glyph arithmetic is off by one, and the charging glyphs come from the wrong family. Using the mapping table in taskbar-tray-system-icon-tweaks.wh.cpp#L363Battery0Battery10 = E850E85A, MobBatteryCharging0MobBatteryCharging10 = EBABEBB5, MobBatterySaver0… = EBB6…:

    • non-charging returns 0xE850 + (step - 1) for step 1–10, i.e. Battery0Battery9, so a 100% bud renders the 90% glyph and Battery10 is never used. 0xE850 + step over step 0–10 is the right mapping (and makes the step == 0 special case unnecessary).
    • charging returns 0xEBB5 + step, which is MobBatteryCharging10 (full) at 0% and then walks into MobBatterySaver0MobBatterySaver9 for 10–100% — inverted and the wrong icon family. 0xEBAB + step is what you want (or 0xE85B + step / E85A if you'd rather keep the desktop family consistent with the non-charging path).
      This is the most user-visible bug in the list — it shows up every time the buds are in the case charging.
  • The packet length field is written as 16-bit but read as 8-bit. BuildPacket emits len & 0xFF followed by a 0x00 high byte, but the reader treats that second byte as a constant (b6) and computes toRead = pLen + 2. Any response with a payload ≥ 256 bytes desyncs the stream — the serial-number CSV (cmd 16390, which you parse as multi-line CSV) is the plausible candidate. uint16_t pLen = b5 | (b6 << 8); would fix it. The CRC check you added now makes the desync recoverable rather than permanent, which is good, but the packet is still lost.

  • The poll interval is only honored when the earbuds send something. The interval check sits at the top of the reader loop, but the loop then blocks in GetAsync(reader.LoadAsync(1)). If the buds go quiet (no unsolicited notifications), QueryBattery() is never issued and the reading freezes at its last value, no matter what pollInterval says. Driving the poll from a separate timer that goes through PostAction would decouple the two — and it would make the lastSeen timeouts below work too.

  • The lastSeen timeouts only run inside the battery-packet handler, so the "not seen for 35 s / 15 s" cleanup can't fire while no battery packets are arriving — which is exactly the situation it's meant to detect.

  • Ringing continues forever if the mod is disabled mid-ring. Wh_ModUninitRemoveWidgetGrid() queues FindBuds(..., false) via PostAction, then Stop() sets m_stopRequested and ActionLoop breaks out of its wait without draining the queue, so the stop command is silently dropped and the earbud keeps beeping. Either send it synchronously before Stop(), or have ActionLoop drain remaining actions on the way out.

  • icon_only shows a stale glyph for an absent bud. The glyph/opacity updates are inside if (state.left.present) / if (state.right.present), so with hideDisconnectedBuds off, a bud that goes away keeps the last battery glyph it had at full opacity instead of falling back to .

  • single_active picks the right bud when neither is present: bool showRight = state.right.present || !state.left.present; — with both absent this shows the right group with a dash rather than falling back to the case or to the status text the way the compact path does.

  • detailed renders a dangling separator: statusText.Text(deviceName + L" • ") puts a bullet at the end with nothing after it.

  • MessageBeep(...) fires on every ring, including the harmless not-in-ear case. Consider only beeping on the in-ear path. Related: the README says Find My Earbuds has "in-ear detection to avoid accidental loud sound in your ear", but the handler just picks a different MessageBeep tone and rings anyway, and StringId::InEarWarning is never displayed — either wire the warning up (e.g. require a second click when the bud reads as present) or soften the README wording.

  • SetEq fires two commands unconditionally. SendCommand(61469, ...) then ok |= SendCommand(61456, ...) sends both the new and the legacy EQ opcode to every device, and ok is true if either succeeded — so the flyout shows the preset as applied even when the device only understood one of them (or neither did anything). Worth gating on the model you already detect from the serial prefix.

  • Device selection can't be steered. The name match (nothing / cmf / ear ( / ear() is much tighter than before, which is good, but it still takes the first paired match, so a user with two Nothing devices can't choose. An optional deviceName setting (empty = auto) would cover that.

  • The LayoutUpdated handler is fairly hot. For the taskbar_*_start / taskbar_after_* positions it runs TransformToVisual plus margin writes on every layout pass of the taskbar root grid, and its own margin writes re-trigger layout (the 1 px tolerance is what keeps it converging). There's no obviously better hook for "track a neighbouring element", so this is just an FYI — inherited from the mod you based the injection layer on.

  • The case card uses (MusicInfo) as its icon, which reads as a music note rather than a charging case in the screenshot. (EarBuds) or a battery/case-ish glyph might look more intentional.


Next steps:

  • /ai-review - after pushing fixes, to get a review of the updated code. You can repeat this as many times as you need, but each review is thorough and usually there's no need for more than 2-3 iterations.
  • /ready-for-reviewer - once you're satisfied with the state of the pull request, to hand it over to a human reviewer. If some findings above are left unaddressed, add a short note explaining why.

See the review process for details.

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-author The author's turn: request an AI review, or respond to one that was posted. and removed waiting-for-ai-review An AI review was requested and is being prepared. labels Sep 11, 2026
@lenorio

lenorio commented Sep 11, 2026

Copy link
Copy Markdown
Author

/ready-for-reviewer

@windhawk-reviewer

Copy link
Copy Markdown

@lenorio /ready-for-reviewer can't be applied here: the most recent AI review covers 4258b97, but the current head of this pull request is 057d715. Comment /ai-review to get a review of the current code.

@lenorio

lenorio commented Sep 11, 2026

Copy link
Copy Markdown
Author

/ai-review

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-ai-review An AI review was requested and is being prepared. and removed waiting-for-author The author's turn: request an AI review, or respond to one that was posted. labels Sep 11, 2026
@windhawk-reviewer

Copy link
Copy Markdown

Submission review

Note: This review was done by Claude. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding.

Remember: The AI reviewer can be wrong - it may misread code, flag correct code as broken, or suggest changes that make things worse. Treat its findings as suggestions to verify, not instructions to follow blindly. You're responsible for the code you submit, so if a finding doesn't hold up, say so instead of changing working code to satisfy it.

Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them.


Another good round — the previous list is genuinely addressed: Start() / Instance() are now serialized (m_lifecycleMutex + m_running.exchange, InstanceMutex), the paired-device lookup is cached with a 15/30 s backoff instead of a 3-second full enumeration, the battery poll is driven from ActionLoop so pollInterval is authoritative and lastSeen expiry actually runs, the glyph arithmetic and the 16-bit length field are fixed, the retry-timer token is revoked, swscanf_s's result is checked, InjectWidget() failure feeds the retry loop, and the README now credits Salyts and declares an @license. Two things left on the required list, both on the unload path.

1. Wh_ModUninit can return with the widget — and its callbacks — still live in the taskbar

BOOL sent = SendMessageTimeoutW(hWnd, kMsg, 0, (LPARAM)&pay,
                                SMTO_ABORTIFHUNG | SMTO_BLOCK, timeoutMs, &result);
...
for (int attempt = 0; attempt < 3 && !removed; ++attempt) {
    removed = RunFromWindowThread(hWnd, [](void*) { RemoveWidgetGrid(); }, nullptr);
    if (!removed) Sleep(50);
}
if (!removed) Wh_Log(L"Wh_ModUninit: Failed to dispatch taskbar teardown");

SMTO_ABORTIFHUNG fails immediately when the taskbar thread merely looks hung (no message processed for ~5 s — busy shell, a slow shutdown, a modal drag), and the three retries are 50 ms apart, so all three can abort in ~150 ms. When that happens RemoveWidgetGrid() never runs and Wh_ModUninit returns anyway, leaving all of this registered in the XAML tree: the 1-second g_dispatcherTimer (its Tick calls UpdateWidgetUi()), the LayoutUpdated handler on the taskbar RootGrid, the widget Button's Click handler, and the Flyout. Windhawk then FreeLibrarys the mod, and the next tick or layout pass jumps into unmapped memory → Explorer crashes. The same hole exists when hWnd is null: teardown is skipped entirely with no fallback. Windhawk requires that nothing from the mod image is running or scheduled once Wh_ModUninit returns, so this path can't be allowed to give up.

The mod you based the injection layer on uses a plain blocking SendMessageW for exactly this reason — taskbar-fluent-media-player.wh.cpp#L1998. Either use the untimed form on the teardown path, or keep the timeout but retry until it actually succeeds rather than three times.

While you're in RemoveWidgetGrid(): it calls s_currentFlyout.Hide() and drops the reference, but never revokes the flyout's Opened / Closed tokens and never clears flyout.Content(nullptr). Hide() doesn't necessarily tear the popup down synchronously (flyouts have close animations), so revoking the two tokens and clearing the content makes it explicit that nothing from the mod image is left hanging off the popup root.

2. StopRinging() blocks the unload on Bluetooth I/O that nothing can cancel

BluetoothManager::Instance().StopRinging();   // m_stopRequested is still false here
BluetoothManager::Instance().Stop();

StopRinging() runs on the Windhawk engine thread before Stop() sets m_stopRequested, so its SendCommand takes m_socketMutex and blocks in GetAsync(writer.StoreAsync()) / FlushAsync() on operations that CancelPendingOperations() can't reach — it doesn't run until StopRinging() has already returned. If the earbuds drop out of range while a bud is ringing (exactly when a user is likely to disable the mod), disabling or updating it stalls for however long the Bluetooth stack takes to fail the write, with Windhawk's unload blocked behind it.

GetAsync is the natural place to bound this, since every blocking call already goes through it:

if (operation.wait_for(std::chrono::seconds(3)) != winrt::Windows::Foundation::AsyncStatus::Completed) {
    try { asyncInfo.Cancel(); } catch (...) {}
    untrack();
    throw winrt::hresult_canceled();
}

Alternatively, run the stop-ring on the action worker and wait on it with a bounded timeout before calling Stop().

Optional improvements

Minor polish — none of this affects users in normal operation, so it's your call.

  • [[clang::no_destroy]] on the std::mutex is unnecessary. std::mutex has a no-op destructor on Windows, so the attribute on InstanceMutex()'s static is pure noise (and invites cargo-culting). The one on InstanceStorage()'s std::optional<BluetoothManager> is the one that matters and is correctly placed — see Global objects and process shutdown.

  • WindhawkUtils::StringSetting instead of raw get/free. LoadSettings does Wh_GetStringSetting + Wh_FreeStringSetting four times, each behind an if (ptr) that can never be false — Wh_GetStringSetting never returns NULL, it returns L"". g_settings.position = WindhawkUtils::StringSetting::make(L"position").get(); is shorter and exception-safe.

  • The worker threads never call winrt::uninit_apartment(). Both WorkerLoop and ActionLoop call init_apartment(multi_threaded) and return without the matching uninit, so each enable/disable cycle leaks the per-thread OLE state. An RAII guard (or an explicit uninit_apartment() at the end of each loop, after m_cachedDevice / m_socket are released) closes it.

  • The reconnect loop retries at a fixed 5 s with no backoff. If the device is connected in Windows but RFCOMM resolution keeps failing, WorkerLoop issues a GetRfcommServicesForIdAsync(..., BluetoothCacheMode::Uncached) — an over-the-air SDP query — every 5 seconds for as long as Explorer lives, and the widget sits on "Connecting…" the whole time (g_earbudsState.connecting is set to true and only ever cleared by Disconnect(), which that path doesn't reach). Backing off after a few failures, and clearing connecting when no service is found, would fix both.

  • UpdateWidgetUi() re-walks the visual tree every second. Nine recursive FindChildByName calls per tick, plus an unconditional ToolTipService::SetToolTip(btn, box_value(tooltip)) that replaces the tooltip object even when the text is identical (and can make it flicker while the pointer is resting on the widget). You already do the right thing for the flyout via FlyoutContext — store the TextBlock/StackPanel references the same way in BuildWidgetGrid(), and only set the tooltip when the string changed.

  • Loc() returns a std::wstring by value for what are all string literals — roughly 20 heap allocations per timer tick. const wchar_t* would do.

  • The #ifndef WH_MOD_ID / #ifndef WH_MOD_VERSION block at the top is dead — Windhawk always defines both on the compiler command line, so the fallbacks never take effect and would silently go stale if the version diverged.

  • Dead code. StringId::InEarWarning, ConnectToAdjust, FindBuds, LowLatency and Stop are declared and translated but never displayed. SubBtnInfo is a byte-for-byte duplicate of AncBtnInfo. The column-insert else branch in InjectWidget (line 3125) is unreachable — every non-tray target is a taskbar_* position, which always takes the edge/tracking path. RemoveWidgetGridChildren's return value is never used now that g_injectedColumn drives removal. g_trackPosition == L"far_left" (line 2926) is inherited from the source mod — trackSide is only ever "left" or "right". And in ProcessPacket, if (serial.rfind("1351", 0) == 0 && g_earbudsState.deviceName.empty()) can't fire: deviceName is initialized to L"CMF Buds 2" and is only ever overwritten with a non-empty name.

  • Secondary taskbars are still unreachable. GetTaskbarXamlRoot has a full Shell_SecondaryTrayWnd branch and you resolve two CSecondaryTaskBand symbol hooks for it, but FindCurrentProcessTaskbarWnd() only ever returns Shell_TrayWnd. Worth either wiring up or removing.

  • License provenance. You added @license MIT, which is good, but taskbar-fluent-media-player.wh.cpp — the source of the injection layer — declares no @license at all, so there's nothing to be compatible with. A quick confirmation from Salyts that MIT is fine for the reused code would make this airtight.

Functionality notes

Non-critical observations and ideas about the feature behavior itself.

  • The widget margin is applied twice, so the setting is effectively doubled. BuildWidgetGrid() sets btn.Margin({marginLeft, 0, marginRight, 0}) on the inner Button, and InjectWidget() then sets the same margin on the widgetGrid that contains it — nested margins add, so the documented default "4 4" renders as 8 px on each side. The tracking path double-counts too: desiredGap = g_injectedGrid.ActualWidth() + marginLeft + marginRight, but ActualWidth() already includes the button's own margins. Setting it in one place only would make the setting mean what it says.

  • The README overstates single-earbud handling. "automatically dims the inactive bud and disables dual-earbud ANC modes when only one bud is worn" — ancRestrictedSingleBud only drives warnBadge.Visibility; the High/Mid/Low/Adaptive buttons stay enabled and clickable. Either disable them when left.present ^ right.present, or soften the wording.

  • A bogus packet length parks the reader indefinitely. pLen is now read as a full 16-bit value, but it's unbounded, and GetAsync(reader.LoadAsync(pLen + 2)) with InputStreamOptions::None waits until that many bytes arrive. One desynced byte can ask for ~64 KB that will never come, and the connection silently freezes until the socket drops. Rejecting a packet whose pLen exceeds the largest response you actually parse (and resyncing on 0x55) would make the CRC recovery you added effective in that case too.

  • Connection detection can lag up to 15 seconds. WorkerLoop polls targetBtDev.ConnectionStatus() on a 15 s timer. Now that the device is cached, subscribing to BluetoothDevice::ConnectionStatusChanged on it and waking the loop from the handler would make reconnects near-instant and let the loop park indefinitely in between.

  • Clicking the widget while the flyout is open. btn.Click does if (s_flyoutOpen) { flyout.Hide(); return; }, but a Flyout is light-dismiss: the click that reaches the button has usually already dismissed the popup, so flyout.Closed has set s_flyoutOpen = false and the handler rebuilds and reopens it. Worth checking on-device whether the toggle actually feels like a toggle.

  • SetEq fires two commands unconditionally. SendCommand(61469, ...) then ok |= SendCommand(61456, ...) sends both the new and the legacy EQ opcode to every device, and ok is true if either succeeded — so the flyout shows the preset as applied even when the device only understood one of them. Worth gating on the model you already detect from the serial prefix.

  • Device selection still can't be steered. The name match (nothing / cmf / ear ( / ear() takes the first paired hit, so a user with two Nothing devices can't choose. An optional deviceName setting (empty = auto) would cover it.

  • Only the primary taskbar gets the widget (see the dead secondary path above) — users with multiple monitors will only see it on one.

  • The LayoutUpdated handler is fairly hot. For the taskbar_*_start / taskbar_after_* positions it runs TransformToVisual plus margin writes on every layout pass of the taskbar root grid, and its own margin writes re-trigger layout (the 1 px tolerance is what keeps it converging). There's no obviously better hook for "track a neighbouring element", so this is just an FYI — inherited from the mod you based the injection layer on.


Next steps:

  • /ai-review - after pushing fixes, to get a review of the updated code. You can repeat this as many times as you need, but each review is thorough and usually there's no need for more than 2-3 iterations.
  • /ready-for-reviewer - once you're satisfied with the state of the pull request, to hand it over to a human reviewer. If some findings above are left unaddressed, add a short note explaining why.

See the review process for details.

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-author The author's turn: request an AI review, or respond to one that was posted. and removed waiting-for-ai-review An AI review was requested and is being prepared. labels Sep 11, 2026
@lenorio

lenorio commented Sep 12, 2026

Copy link
Copy Markdown
Author

/ai-review

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-ai-review An AI review was requested and is being prepared. and removed waiting-for-author The author's turn: request an AI review, or respond to one that was posted. labels Sep 12, 2026
@windhawk-reviewer

Copy link
Copy Markdown

Submission review

Note: This review was done by Claude. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding.

Remember: The AI reviewer can be wrong - it may misread code, flag correct code as broken, or suggest changes that make things worse. Treat its findings as suggestions to verify, not instructions to follow blindly. You're responsible for the code you submit, so if a finding doesn't hold up, say so instead of changing working code to satisfy it.

Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them.


Both items from the last round are addressed: RunFromWindowThread now uses the plain blocking SendMessageW, the flyout's Opened/Closed tokens are revoked and its content cleared in RemoveWidgetGrid(), teardown has a dispatcher fallback, every blocking WinRT call goes through a bounded GetAsync(..., timeout) so StopRinging() can no longer stall indefinitely, the reader rejects oversized pLen, WindhawkUtils::StringSetting replaced the raw get/free pairs, both worker loops call uninit_apartment(), the reconnect path backs off, the std::mutex lost its unnecessary [[clang::no_destroy]], the tooltip is only rewritten when it changes, the widget margin is applied in one place, and the dead code from the last list is gone. Two things left, both on the unload path.

1. The stop signal is published without the mutex the waiters use, so it can be missed — and then Wh_ModUninit blocks for up to pollInterval

Stop() stores the flag and notifies without ever touching m_actionMutex / m_workerWaitMutex:

m_stopRequested.store(true);
m_actionCv.notify_all();               // m_actionMutex not held
m_workerWakeState->cv.notify_all();    // m_workerWaitMutex not held

Both waiters evaluate their predicate while holding their mutex and then atomically release-and-block. Because the notifier never takes that mutex, a notify_all that lands in the window between the predicate check and the actual block is lost — this is exactly why the state a condition variable's predicate reads must be modified under the mutex even when it's a std::atomic. When it happens:

  • ActionLoop sleeps to nextPoll — up to pollInterval, i.e. 120 s at the maximum setting — before it re-checks m_stopRequested at the top of the loop;
  • WorkerLoop sleeps out its WaitForWake(...) — up to 60 s on the backoff path.

Stop() is inside join() for that whole time, so disabling or updating the mod freezes Windhawk's unload for up to two minutes. CancelPendingOperations() doesn't help: the thread isn't in an async operation, it's parked on the condition variable. The window is narrow, but the fix is a couple of lines. Move the mutex into WorkerWakeState so the ConnectionStatusChanged handler can take it too, and publish every predicate change under the corresponding lock:

struct WorkerWakeState {
    std::mutex mutex;
    std::condition_variable cv;
    std::atomic<uint64_t> connectionStatusVersion{0};
};

void Stop() {
    std::lock_guard<std::mutex> lock(m_lifecycleMutex);
    if (!m_running.exchange(false)) return;
    {
        std::lock_guard<std::mutex> a(m_actionMutex);
        std::lock_guard<std::mutex> w(m_workerWakeState->mutex);
        m_stopRequested.store(true);
    }
    m_actionCv.notify_all();
    m_workerWakeState->cv.notify_all();
    ...

The same applies to the two other notifiers, where the consequence is functional rather than a stall: NotifyPollIntervalChanged() sets m_pollIntervalChanged outside m_actionMutex (a new interval can be ignored for one full cycle), and the ConnectionStatusChanged handler in CacheDevice() bumps connectionStatusVersion outside the wait mutex (a reconnect can be noticed up to 15/30 s late, which is the latency the wake-up was added to remove). Wrapping each state change in std::lock_guard on the matching mutex before notifying closes all three.

2. The dispatcher fallback in Wh_ModUninit can block forever

dispatcher.RunAsync(
    winrt::Windows::UI::Core::CoreDispatcherPriority::Normal,
    []() { RemoveWidgetGrid(); }).get();

This path runs only when RunFromWindowThread failed — which in practice means the taskbar window is gone, i.e. the case where its thread is most likely gone as well. A work item queued on a dispatcher whose thread no longer pumps never completes, so the unbounded .get() parks Wh_ModUninit indefinitely and Windhawk's disable/update never returns. Ironically, that's the one situation where doing nothing would have been safe: with the UI thread dead, g_dispatcherTimer can never tick and there's nothing left to tear down.

Bound the wait so unload can't hang:

auto op = dispatcher.RunAsync(
    winrt::Windows::UI::Core::CoreDispatcherPriority::Normal,
    []() { RemoveWidgetGrid(); });
if (op.wait_for(std::chrono::seconds(5)) ==
    winrt::Windows::Foundation::AsyncStatus::Completed) {
    removed = true;
}

Note the tradeoff: a timed-out wait leaves the delegate queued, so if the thread later drains its queue it would call RemoveWidgetGrid() from an unmapped image. The window route is the reliable one, so make it the primary — re-resolve the taskbar HWND and retry RunFromWindowThread a few times before falling back to the dispatcher. That also matters for the tail of the function: when both routes fail, Wh_ModUninit currently just logs "UI objects remain after teardown" and returns, and Windhawk then FreeLibrarys the mod with the 1-second DispatcherTimer, the LayoutUpdated handler and the widget's Click handler still registered in the taskbar's XAML tree — the next tick jumps into unmapped memory.

Optional improvements

Minor polish — none of this affects users in normal operation, so it's your call.

  • [[clang::no_destroy]] on g_lastTooltip is unnecessary (line 298). std::optional<std::wstring>'s destructor only frees heap, which is safe on the process-shutdown path, so the attribute is pure noise here and invites cargo-culting — see Global objects and process shutdown. The attributes on the XAML/CoreDispatcher globals, on s_currentFlyoutCtx (a shared_ptr owning XAML refs) and on InstanceStorage()'s std::optional<BluetoothManager> are all correct and necessary.

  • Start() should check g_unloading under m_lifecycleMutex. TrayUI_StartTaskbar_Hook checks g_unloading after TrayUI_StartTaskbar_Original returns, so in principle the check can pass just as Wh_ModUninit begins; if the hook then reaches Start() after Stop() has already joined, the mod unloads with two live worker threads. The window is tiny (the engine thread's own teardown normally blocks on a SendMessage to the same UI thread), but if (g_unloading.load()) return; immediately after taking m_lifecycleMutex closes it for free.

  • StopRinging() still adds up to ~6 s to an unload. It's bounded now, but it runs synchronously on the engine thread before Stop(), two SendCommands deep at 3 s each. Queueing it through the action worker and waiting with a short bounded timeout would keep Wh_ModUninit snappy when the buds are out of range.

  • UpdateWidgetUi() re-walks the visual tree every second — nine recursive FindChildByName calls per tick just to find elements the mod created itself. You already do the right thing for the flyout via FlyoutContext; storing the TextBlock/StackPanel references the same way in BuildWidgetGrid() would remove the walk entirely.

  • The battery-expiry logic is duplicated. ProcessPacket (lines 789-798) repeats ExpireStaleBatteryReadings() verbatim. Worth calling the helper instead — also note the standalone copy only runs on the poll tick, so with pollInterval at 120 s a removed bud can read stale for up to two minutes.

  • Loc() returns a std::wstring by value for what are all string literals — roughly 20 heap allocations per timer tick while building the tooltip. const wchar_t* would do.

  • README nit: the EQ list omits the Pop preset that the flyout offers.

Functionality notes

Non-critical observations and ideas about the feature behavior itself.

  • A stale "In case" label can sit next to the case battery readout. In the compact/detailed path with hideDisconnectedBuds on and both buds absent, the branch that shows the case battery (line 2856) doesn't collapse statusText, and the trailing else if (!hideDisconnectedBuds || showLeft || showRight) is false in that state — so if the previous tick took the no-case-battery branch and set statusText to In case, the widget ends up reading Case 56% In case. An explicit statusText.Visibility(Visibility::Collapsed) in that branch fixes it.

  • SetEq fires two commands unconditionally. SendCommand(61469, ...) then ok |= SendCommand(61456, ...) sends both the new and the legacy EQ opcode to every device, and ok is true if either succeeded — so the flyout shows the preset as applied even when the device only understood one of them. Worth gating on the model once you can detect it.

  • Device selection still can't be steered. The name match (nothing / cmf / ear ( / ear() takes the first paired hit, so a user with two Nothing devices can't choose which one the widget follows. An optional deviceName setting (empty = auto) would cover it.

  • Only the primary taskbar gets the widget. FindCurrentProcessTaskbarWnd() matches Shell_TrayWnd only, so on a multi-monitor setup the widget appears on one taskbar. Fine as a limitation — worth a line in the README so it isn't read as a bug.

  • A quiet earbud causes a reconnect cycle. The reader's first LoadAsync(1) is bounded by max(15, pollInterval + 10) seconds; if a poll goes unanswered the socket is torn down and rebuilt rather than retried. That's a reasonable way to detect a dead link, just noting the widget will blink through Connecting… when it happens.

  • The LayoutUpdated handler is fairly hot. For the taskbar_*_start / taskbar_after_* positions it runs TransformToVisual plus margin writes on every layout pass of the taskbar root grid, and its own margin writes re-trigger layout (the 1 px tolerance is what keeps it converging). There's no obviously better hook for "track a neighbouring element", so this is just an FYI — inherited from the mod you based the injection layer on.


Next steps:

  • /ai-review - after pushing fixes, to get a review of the updated code. You can repeat this as many times as you need, but each review is thorough and usually there's no need for more than 2-3 iterations.
  • /ready-for-reviewer - once you're satisfied with the state of the pull request, to hand it over to a human reviewer. If some findings above are left unaddressed, add a short note explaining why.

See the review process for details.

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-author The author's turn: request an AI review, or respond to one that was posted. and removed waiting-for-ai-review An AI review was requested and is being prepared. labels Sep 12, 2026
@lenorio

lenorio commented Sep 12, 2026

Copy link
Copy Markdown
Author

/ai-review

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-ai-review An AI review was requested and is being prepared. and removed waiting-for-author The author's turn: request an AI review, or respond to one that was posted. labels Sep 12, 2026
@windhawk-reviewer

Copy link
Copy Markdown

Submission review

Note: This review was done by Claude. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding.

Remember: The AI reviewer can be wrong - it may misread code, flag correct code as broken, or suggest changes that make things worse. Treat its findings as suggestions to verify, not instructions to follow blindly. You're responsible for the code you submit, so if a finding doesn't hold up, say so instead of changing working code to satisfy it.

Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them.


Both items from the last round are fixed: PublishStopRequested() now sets m_stopRequested under both m_actionMutex and the wake state's mutex (which moved into WorkerWakeState so the ConnectionStatusChanged handler shares it), NotifyPollIntervalChanged()/PostAction() publish under the lock too, and the unload path now retries RunFromWindowThread five times before falling back to a bounded wait_for(5s) on the dispatcher. The optional items from last round are in as well — Start() checks g_unloading under the lifecycle mutex, StopRinging() is bounded at 750 ms per command, the duplicated expiry block is gone, the stale In case label is collapsed, g_lastTooltip lost its unneeded [[clang::no_destroy]], and the README lists Pop. Two things this round, both feature-breaking rather than crashy.

1. LayoutUpdated passes null as sender, so the eight "Taskbar - …" tracking positions never position the widget

The handler registered at line 3243 starts with:

auto layoutGrid = sender.try_as<Grid>();
if (!layoutGrid) return;

FrameworkElement.LayoutUpdated is a layout-manager-wide notification, not an element event — in UWP XAML it is raised with a null sender. With that early return the body never runs, which affects taskbar_left_start, taskbar_right_start, taskbar_after_search_left/right, taskbar_after_taskview_left/right and taskbar_after_widgets_left/right — 8 of the 24 position options. In that branch InjectWidget() deliberately sets no margin on widgetGrid (the widgetGrid.Margin(...) call at line 3285 is only the !targetElem fallback) and relies entirely on the handler for both the widget's Margin.Left and the tracked element's gap, so the widget ends up pinned at the left edge of column 0 of the taskbar root grid, overlapping the Start/Search/Task View buttons, and the tracked element never gets its gap.

Worth noting that every mod in the repo that uses this event ignores sender and captures the element instead — including taskbar-fluent-media-player, which this injection layer is based on and which captures targetGrid by value. Same fix here — layoutGrid is only needed for the TransformToVisual call:

g_layoutUpdateToken = targetGrid.LayoutUpdated(
    [targetGrid](winrt::Windows::Foundation::IInspectable const&,
                 winrt::Windows::Foundation::IInspectable const&) {
        try {
            if (!g_injectedGrid || !g_trackedElement || g_unloading) return;
            ...
            auto transform = g_trackedElement.TransformToVisual(targetGrid);

2. Battery readings expire on fixed 35 s / 15 s timers while pollInterval is user-settable up to 120 s

ExpireStaleBatteryReadings() (lines 689-704) clears left/right after 35 s and caseBattery after 15 s without looking at the poll interval, and since last round it runs every second from ActionLoop instead of only on the poll tick — so the mismatch is now continuously visible:

  • At the default pollInterval of 30 s, the case reading is cleared 15 s after each response and only comes back at the next poll, so the case battery blinks in and out on a 30 s cycle (the README flyout screenshot happens to show the case card reading while both buds report fine).
  • At pollInterval ≥ 40 s the buds themselves expire too — at the maximum 120 s the widget spends 85 s of every 2-minute cycle claiming the buds are disconnected, which with the default hideDisconnectedBuds: true means it flips to In case / hides the buds entirely.

The thresholds need to be derived from the interval, e.g.:

void ExpireStaleBatteryReadings() {
    auto ttl = std::chrono::seconds(g_pollIntervalSeconds.load() * 2 + 5);
    auto now = std::chrono::steady_clock::now();
    std::lock_guard<std::mutex> lock(g_stateMutex);
    if (g_earbudsState.left.present && now - g_earbudsState.left.lastSeen > ttl)
        g_earbudsState.left.present = false;
    ...
}

If the case is meant to expire faster than the buds on purpose, it still needs a floor of at least one poll interval, otherwise it can never survive a cycle.

Optional improvements

Minor polish — none of this affects users in normal operation, so it's your call.

  • operation.Close() after Cancel() throws. At line 3577, Cancel() only requests cancellation — the operation is still in Started when Close() is called on the next line, and IAsyncInfo::Close on a non-terminal async fails with E_ILLEGAL_STATE_CHANGE. The throw is swallowed by the outer catch, so the intended "Dispatcher teardown timed out" log never fires and you get "Dispatcher teardown failed" instead. Just drop the Close(); the Cancel() is the part that matters.

  • Wh_ModSettingsChanged drops the new settings if the dispatch fails. LoadSettings() is called inside the RunFromWindowThread callback (line 3604), so when RunFromWindowThread returns false the mod logs "Failed to dispatch settings update" and keeps running on the old settings. taskbar-fluent-media-player calls LoadSettings() on the engine thread first and then dispatches only the UI rebuild — worth doing the same (it also keeps Wh_Get*Setting on the thread Windhawk called you on).

  • UpdateWidgetUi() re-walks the visual tree every second — nine recursive FindChildByName calls per tick (lines 2771-2779) to find elements the mod created itself. You already do the right thing for the flyout via FlyoutContext; storing the TextBlock/StackPanel references the same way in BuildWidgetGrid() would remove the walk entirely.

  • Loc() returns a std::wstring by value for what are all string literals — roughly 20 heap allocations per timer tick while building the tooltip, even when the tooltip ends up unchanged. const wchar_t* would do.

  • StopRinging() still runs synchronously on the unload path. It's bounded now, but it runs on the engine thread before Stop(), two SendCommands deep, and each one first has to take m_socketMutex — which a worker SendCommand can hold for up to 3 s. Queueing it through the action worker and waiting with a short bounded timeout would keep Wh_ModUninit snappy when the buds are out of range.

Functionality notes

Non-critical observations and ideas about the feature behavior itself.

  • The taskbar screenshot undersells the widget. Both README screenshots show the widget in icon_only mode — two battery glyphs and no numbers — while the mod's headline feature is the per-bud percentage. A shot of the default compact format (L 75% R 85%) as the first image would represent it better. The flyout screenshot is good and addresses the earlier feedback.

  • SetEq fires two commands unconditionally. SendCommand(61469, ...) then ok |= SendCommand(61456, ...) sends both the new and the legacy EQ opcode to every device, and ok is true if either succeeded — so the flyout shows the preset as applied even when the device only understood one of them. Worth gating on the model once you can detect it.

  • Device selection still can't be steered. The name match (nothing / cmf / ear ( / ear() takes the first paired hit, so a user with two Nothing devices can't choose which one the widget follows. An optional deviceName setting (empty = auto) would cover it.

  • Only the primary taskbar gets the widget. FindCurrentProcessTaskbarWnd() matches Shell_TrayWnd only (line 1189), so on a multi-monitor setup the widget appears on one taskbar. Fine as a limitation — worth a line in the README so it isn't read as a bug.


Next steps:

  • /ai-review - after pushing fixes, to get a review of the updated code. You can repeat this as many times as you need, but each review is thorough and usually there's no need for more than 2-3 iterations.
  • /ready-for-reviewer - once you're satisfied with the state of the pull request, to hand it over to a human reviewer. If some findings above are left unaddressed, add a short note explaining why.

See the review process for details.

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-author The author's turn: request an AI review, or respond to one that was posted. and removed waiting-for-ai-review An AI review was requested and is being prepared. labels Sep 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

waiting-for-author The author's turn: request an AI review, or respond to one that was posted.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants