Skip to content

Add Mallss Music Overlay - #5449

Open
ItsMeMal wants to merge 5 commits into
ramensoftware:mainfrom
ItsMeMal:main
Open

Add Mallss Music Overlay#5449
ItsMeMal wants to merge 5 commits into
ramensoftware:mainfrom
ItsMeMal:main

Conversation

@ItsMeMal

@ItsMeMal ItsMeMal commented Sep 11, 2026

Copy link
Copy Markdown

Changelog

  • Initial release of Mallss Music Overlay.

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):

@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.

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-author The author's turn: request an AI review, or respond to one that was posted. 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
Added readme documentation for the Mallss Music Overlay mod, detailing features, usage, controls, and supported media.
@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.


The overlay itself is nicely put together, but there are several structural problems to resolve before it can be merged — the process it runs in, two globals that will take down Explorer at shutdown, a window class that is never cleaned up, and an init check that prevents the mod from ever starting after a reboot.

1. This should be a tool mod, not an explorer.exe injection

The mod does not hook a single function. Everything it does — GlobalSystemMediaTransportControlsSessionManager, a WS_POPUP layered window, GDI+ rendering, EnumWindows, GetForegroundWindow — works from any process. The classic tell-tale signs are all present: a home-grown single-instance mutex (AcquireSingleton, line 3910), a GetDesktopShellPid() guard to pick "the real" Explorer (line 3853), and no interaction with the host process's own state anywhere. The cost of injecting anyway is high: a permanent 60 Hz render loop and blocking cross-process WinRT calls now live inside the shell, and a crash or hang in either takes Explorer down with it.

Please convert it per Mods as tools: Running mods in a dedicated process: set @include windhawk.exe, rename Wh_ModInit/Wh_ModUninit to WhTool_ModInit/WhTool_ModUninit, and paste the launcher boilerplate from the wiki verbatim (please don't refactor or trim it — it makes review much easier). Then delete AcquireSingleton, GetDesktopShellPid, FindShellProcessProc, g_singletonMutex and g_activeMod; the framework handles single-instancing for you.

quick-launch-media-panel.wh.cpp is a very close reference — a desktop panel with Windows media controls and GDI+ artwork, running as a tool mod. explorer-folder-hover-menu.wh.cpp has a verbatim copy of the launcher snippet at the bottom of the file.

2. The mod never starts after a reboot or an Explorer restart

Wh_ModInit runs before the target process starts executing. When Explorer launches there is no Progman, no SHELLDLL_DefView and no shell window yet, so GetDesktopShellPid() returns 0 (or, during a restart, the PID of the old dying Explorer), and the check at line 4012 returns FALSE. Windhawk only gives the mod another chance after a settings change — and this mod has no settings — so in practice the overlay silently never appears until the user disables and re-enables the mod by hand.

Converting to a tool mod removes this check entirely. If you keep it for some other reason, it has to be deferred to a point where the shell windows actually exist (e.g. from the worker thread, retrying).

3. g_uiThread / g_mediaThread will crash Explorer on every shutdown

Lines 126–127. Wh_ModUninit is not called when the host process terminates (Explorer restart, sign-out, reboot) — only the global destructors run, after every other thread has already been killed. Both std::thread globals are still joinable() at that point, so ~thread() calls std::terminate() and aborts the process. This fires on every single Explorer restart, not in some rare edge case.

[[clang::no_destroy]] static std::optional<std::thread> g_uiThread;
[[clang::no_destroy]] static std::optional<std::thread> g_mediaThread;

Then g_uiThread.emplace(UIThreadProc); in init, and in Wh_ModUninit keep the existing join and follow it with g_uiThread.reset(); (same for the media thread). See Global objects and process shutdown; win11-home-group-restorer.wh.cpp#L7133 uses exactly this pattern.

4. g_session / g_sessionManager — same teardown path, different failure

Lines 186–190. These are out-of-process WinRT proxies. At process exit their automatic destructors marshal a Release into an apartment whose threads are already gone, which can hang or crash the shutting-down shell. Mark both with the bare attribute (projected WinRT types are nullable, so no std::optional wrapper is needed) and keep the explicit = nullptr you already do in Wh_ModUninit:

[[clang::no_destroy]] static GlobalSystemMediaTransportControlsSessionManager g_sessionManager{nullptr};
[[clang::no_destroy]] static GlobalSystemMediaTransportControlsSession g_session{nullptr};

Same wiki page, section 3; island-media-controls.wh.cpp#L580 does the same for its COM globals.

5. The window class is never unregistered, and ERROR_CLASS_ALREADY_EXISTS is treated as success

Lines 3320–3370. Two related problems:

  • The class is registered with wc.hInstance = GetModuleHandleW(nullptr) — that's explorer.exe's module handle, not the mod's — and it is never unregistered. A window class is not removed when the mod DLL unloads, so after the first disable/reload the registration survives in the process with lpfnWndProc still pointing into the unmapped mod image.
  • Because atom == 0 && GetLastError() == ERROR_CLASS_ALREADY_EXISTS is allowed to continue, the next load happily reuses that stale class, and CreateWindowExW produces a window whose WndProc is a dangling pointer — a crash (or worse) in Explorer on the first message. The 141 suffix in L"MallssMusicOverlay141" only papers this over across version bumps; a plain toggle of the same version hits it directly.

Fix: register with the mod's own module handle, treat ERROR_CLASS_ALREADY_EXISTS as a failure, and unregister on unload.

HINSTANCE GetCurrentModuleHandle() {
    HINSTANCE instance = nullptr;
    GetModuleHandleExW(GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS |
                           GET_MODULE_HANDLE_EX_FLAG_UNCHANGED_REFCOUNT,
                       reinterpret_cast<LPCWSTR>(&GetCurrentModuleHandle),
                       &instance);
    return instance;
}

// registration
wc.hInstance = g_hInst;
if (!RegisterClassExW(&wc)) {
    Wh_Log(L"RegisterClassExW failed: %lu", GetLastError());
    return;  // including ERROR_CLASS_ALREADY_EXISTS
}

// Wh_ModUninit, after the UI thread has been joined:
UnregisterClassW(L"MallssMusicOverlay", g_hInst);

GET_MODULE_HANDLE_EX_FLAG_UNCHANGED_REFCOUNT matters — the mod must not hold an extra reference on its own module. quick-launch-media-panel.wh.cpp#L2053-L2060 and #L2125-L2127 show the full register/unregister pair, and #L2340 the module-handle helper.

6. Media commands run on the UI thread, race with the poll thread, and can hang the unload

MediaPrevious / MediaNext / MediaTogglePlayPause / MediaSeek (lines 747–862) are called straight from WM_LBUTTONDOWN (line 3176) on the UI thread, while QueryMedia reassigns g_session (lines 917 and 926) from the media thread every 500 ms. Two consequences:

  • Data race. Assigning a projected WinRT type is a release-of-old plus addref-of-new, not an atomic store. A click that lands while the poll thread is swapping the session reads a torn or already-released pointer — a use-after-free in Explorer. Nothing guards g_session (g_stateMutex is only used for the playback fields).
  • Unload hang. Each command blocks in .get() on a cross-process call into the media app. If that app is busy or hung, the UI thread is stuck, and g_uiThread.join() in Wh_ModUninit (line 4220) never returns — the mod unload hangs, with no timeout anywhere.

Fix: guard g_session with a mutex and take a local strong copy before use, and move the actual blocking calls off the UI thread — push a command onto the media thread (condition variable + small queue) and return immediately from the window proc. island-media-controls.wh.cpp runs its media work on a dedicated command thread and uses operation.wait_for(timeout) instead of an unbounded .get(); both are worth copying.

7. The UI thread never idles

The loop at lines 3509–3766 is a Sleep(16) spin: it wakes ~62 times a second forever, even when nothing is playing and the window is hidden, and while playing it re-renders the whole overlay through GDI+ and calls UpdateLayeredWindow at 60 FPS. Every frame constructs new Font, SolidBrush, GraphicsPath and StringFormat objects and re-runs MeasureString on an unchanged title (line 2645). In an always-running process this is a permanent CPU/battery cost for a widget that mostly shows a clock and a progress bar.

Suggested changes, roughly in order of payoff:

  • Block in MsgWaitForMultipleObjectsEx(0, nullptr, timeout, QS_ALLINPUT, MWMO_INPUTAVAILABLE) (or a plain GetMessage loop driven by SetTimer) instead of Sleep + PeekMessage, and use a long timeout — or none at all — while the overlay is hidden.
  • Only run the ~60 Hz tick while the marquee is actually scrolling; otherwise a 250–500 ms tick is plenty for the progress bar and clock.
  • Cache the fonts/brushes and the measured title width instead of rebuilding them per frame.

8. No DPI scaling, and the overlay is pinned to the primary monitor

All geometry is hard-coded in pixels (lines 49–93) and PositionOverlay always uses MONITOR_DEFAULTTOPRIMARY (line 1405). Explorer's windows are per-monitor DPI aware, so on a 150% or 200% display the overlay renders at roughly half the intended size with unreadably small text, and there is no WM_DPICHANGED handling. Please scale the layout constants by GetDpiForWindow(g_hwnd) / 96.0, rebuild the back buffer and window region on WM_DPICHANGED, and let the user choose the monitor.

9. Nothing is configurable

There is no ==WindhawkModSettings== block, so the position (bottom-right of the primary monitor), size, colors, poll interval and clock format are all fixed. At minimum, please expose the monitor/corner (or an explicit offset), and the clock and date format — CurrentClock (line 382) hard-codes a 24-hour clock and CurrentDate (line 402) hard-codes DD/MM/YYYY, which is wrong for a large share of users. GetTimeFormatEx/GetDateFormatEx with LOCALE_NAME_USER_DEFAULT gives the user's own format for free, and a format string setting on top of that covers the rest.

10. Please describe how this differs from the existing media mods

The catalog already has quick-launch-media-panel (a desktop panel with album art and media controls, already a tool mod), plus island-media-controls, taskbar-music-lounge and taskbar-fluent-media-player on the taskbar side. A free-floating desktop overlay with a clock is arguably a different thing, but the overlap with the first one in particular is close enough that it's worth stating in the README what this offers that it doesn't — and, if the gap is small, considering an option on the existing mod instead. The maintainer's consistent preference is to extend an existing mod rather than merge a near-duplicate.

11. README

CI is currently red; please fix what it reports. When you add the README block, include a screenshot or a short GIF of the overlay — this is a purely visual mod, and users pick it from the catalog on the strength of that image. Only i.imgur.com and raw.githubusercontent.com are allowed as image hosts.

Optional improvements

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

  • Unused dependencies. #include <shlwapi.h> (line 14) and -lshlwapi are unused — nothing from shlwapi is called. -loleaut32 also looks unnecessary. The #pragma comment(lib, ...) block (lines 32–37) is an MSVC-ism that clang/mingw ignores; @compilerOptions already does the job, so the whole block can go.
  • Log prefixes. Wh_Log(L"[UI] ..."), [Guard], [Init] — Windhawk already prefixes log lines with the mod name, so the tags are noise. (Wh_Log itself is cheap and disabled by default, so the call sites are fine.)
  • Dead code.
    • g_cachedMinuteKey / clockChanged (lines 3626–3637) feed if (shouldShow || clockChanged) { if (shouldShow) { ... } } at line 3750 — the outer condition can never matter, so the minute tracking has no effect.
    • g_lastZOrderQpc (line 238) is written at line 1503 and never read.
    • The CreateWindowExW retry without WS_EX_NOREDIRECTIONBITMAP (lines 3398–3421) is unreachable: that style doesn't make window creation fail. WS_EX_NOREDIRECTIONBITMAP is meant for windows rendered through DirectComposition, not UpdateLayeredWindow — I'd drop both the style and the retry.
    • DrawArtist's if (artist.empty()) artist = L"Unknown artist"; (line 2034) can't trigger, since QueryMedia already substitutes it at line 987.
  • g_hwnd after WM_CLOSE. The window proc destroys the window (line 3272) but g_hwnd stays non-null until UIThreadProc exits, so the loop runs one more iteration calling IsWindowVisible/SetWindowPos on a destroyed handle, and DestroyWindow is then called a second time at line 3774. Clear g_hwnd in WM_NCDESTROY instead.
  • Redundant lock. DrawControls (line 2578) takes g_stateMutex just to read g_playing, which is already std::atomic<bool>.
  • SecondsSince calls QueryPerformanceFrequency on every invocation (line 263) — it's constant for the lifetime of the system, so query it once.
  • Formatting. The repo has a .clang-format; running it would cut this file down considerably (one argument per line plus a blank line between every statement is what turns ~1500 lines of logic into 4281) and make future diffs much easier to read.

Functionality notes

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

  • The overlay disappears the moment you pause. ApplyMedia sets g_visible = snapshot.playing (line 1188), so pausing from the overlay immediately hides it — which means the Play half of the play/pause button can never be used. Consider staying visible while a session exists (optionally fading out after a delay), so the control is actually round-trippable.
  • The second marquee copy is never visible. DrawTitle draws a duplicate at offset + g_cachedTitleWidth + MARQUEE_GAP (line 1996), but scrolling stops at -(g_cachedTitleWidth - TITLE_WIDTH) (line 3700), so the duplicate always sits to the right of the clip rect. Either drop it, or switch to a truly continuous loop by scrolling to -(g_cachedTitleWidth + MARQUEE_GAP) and wrapping.
  • Stale duration on tracks that don't report one. g_duration is only updated when snapshot.duration > 0.0 (line 1094), so switching to a live stream or a track with no timeline keeps the previous track's duration — the progress bar then shows a bogus position and click-to-seek maps to the wrong range. Reset g_duration to 0 on a track change instead.
  • The seek hit area overlaps the album line. HitTest treats y in [52, 80] as the progress bar (line 3104), while the album text is drawn at y 49–63 (line 2123) — clicking the album name seeks the track. Tightening the band to around the bar (y 60–78) would avoid the surprise.
  • Z-order flicker. The overlay is pushed between HWND_TOP and HWND_BOTTOM based on a foreground-window poll every 120 ms (line 1458), so there's a visible lag when alt-tabbing to and from the desktop. Not obviously avoidable with this design, just worth knowing; a SetWinEventHook on EVENT_SYSTEM_FOREGROUND would make it event-driven.
  • Fixed-width text areas. Artist and album are ellipsized to 178 px and the title marquee assumes the same, so long metadata is cut fairly aggressively at the default size. Worth revisiting if you add a size setting.


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. waiting-for-ai-review An AI review was requested and is being prepared. and removed waiting-for-ai-review An AI review was requested and is being prepared. 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.


A nicely self-contained overlay, but as an explorer.exe mod it has a few issues that will crash or hang the shell, plus an architectural mismatch — it installs no hooks at all, so it shouldn't be injecting into Explorer in the first place.

1. Global std::thread objects abort Explorer on every sign-out / reboot / Explorer restart.

static std::thread g_mediaThread;   // line 176
static std::thread g_uiThread;      // line 177

Wh_ModUninit runs on a normal mod unload, but not when the host process terminates. On that path the CRT still runs global destructors, and both threads are still joinable() — so ~std::thread() calls std::terminate() and aborts explorer.exe. Every sign-out, shutdown and Explorer restart hits this. See https://github.com/ramensoftware/windhawk/wiki/Global-objects-and-process-shutdown (#1-worker-thread-stdthread):

[[clang::no_destroy]] static std::optional<std::thread> g_mediaThread;
[[clang::no_destroy]] static std::optional<std::thread> g_uiThread;
// Wh_ModInit:   g_uiThread.emplace(UIThreadProc);
// Wh_ModUninit: if (g_uiThread->joinable()) g_uiThread->join(); g_uiThread.reset();

Example: taskbar-fluent-media-player.wh.cpp#L4399, taskbar-system-info.wh.cpp#L478.

2. The WinRT media-session globals have the same problem, and are released on the wrong thread.

static GlobalSystemMediaTransportControlsSessionManager g_sessionManager{nullptr};  // 236
static GlobalSystemMediaTransportControlsSession        g_session{nullptr};         // 239

These are out-of-process proxies. At process exit their automatic Release() marshals into an apartment whose thread is already dead — the wiki page uses GlobalSystemMediaTransportControlsSession g_session{nullptr} as its literal broken example (#3-winrt-or-com-object-especially-out-of-process). Both are nullable projected types, so the bare attribute is the right form:

[[clang::no_destroy]] static GlobalSystemMediaTransportControlsSessionManager g_sessionManager{nullptr};
[[clang::no_destroy]] static GlobalSystemMediaTransportControlsSession        g_session{nullptr};

Precedent: taskbar-fluent-media-player.wh.cpp#L2067.

Separately, the explicit release ordering in Wh_ModUninit is wrong. Both objects are created on the media thread's MTA, but MediaThreadProc calls winrt::uninit_apartment() (line 1354) before Wh_ModUninit gets to g_session = nullptr; (lines 4289-4293) — so the release happens after the apartment that owns them may already be gone. Release them at the end of MediaThreadProc, before uninit_apartment().

3. The window class is never unregistered, and ERROR_CLASS_ALREADY_EXISTS is deliberately tolerated — that combination can execute a dangling WndProc.

wc.hInstance    = GetModuleHandleW(nullptr);       // 3378
wc.lpszClassName = L"MallssMusicOverlay141";       // 3383
ATOM atom = RegisterClassExW(&wc);
if (atom == 0 && GetLastError() != ERROR_CLASS_ALREADY_EXISTS) { ... return; }   // 3400

There is no UnregisterClass anywhere, so when the mod unloads the registration survives in explorer.exe with lpfnWndProc still pointing into the unmapped mod image. On the next load (toggle, update, settings reload) RegisterClassExW fails with ERROR_CLASS_ALREADY_EXISTS, the code continues anyway, and CreateWindowExW creates a window from the stale class — messages then dispatch into freed memory. Version-suffixing the class name (...141) only postpones it until someone toggles the same version.

Two changes: register with the mod's own module handle (the WndProc lives there — GetModuleHandleW(nullptr) is explorer.exe), and unregister on unload. Drop the ERROR_CLASS_ALREADY_EXISTS tolerance — a fresh registration must succeed.

static HMODULE GetCurrentModuleHandle() {
    HMODULE mod = nullptr;
    GetModuleHandleExW(GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS |
                       GET_MODULE_HANDLE_EX_FLAG_UNCHANGED_REFCOUNT,
                       L"", &mod);
    return mod;
}
// ... wc.hInstance = GetCurrentModuleHandle();
// on teardown, after DestroyWindow:
UnregisterClassW(kClassName, GetCurrentModuleHandle());

References: autoscroll-win32.wh.cpp#L984, mic-mute-hotkey-overlay.wh.cpp#L1093, desktop-draggable-widgets.wh.cpp#L1975.

4. This should be a tool mod, not an explorer.exe mod.

The mod installs zero function hooks and zero symbol hooks. It creates its own window, its own UI thread and its own polling thread, and never touches Explorer's state — it just needs a process to live in. It also hand-rolls the two things the tool-mod framework gives you for free: a single-instance mutex (AcquireSingleton, lines 3960-4003) and a "which Explorer is the real shell?" PID probe (GetDesktopShellPid, lines 3903-3954). Those are textbook signals from https://github.com/ramensoftware/windhawk/wiki/Mods-as-tools:-Running-mods-in-a-dedicated-process. On top of that, a GDI+ or WinRT fault in this code currently takes down the shell.

The rewrite: @include windhawk.exe, rename Wh_ModInit/Wh_ModUninitWhTool_ModInit/WhTool_ModUninit, paste the launcher boilerplate from the wiki verbatim, and delete AcquireSingleton, GetDesktopShellPid, FindShellProcessProc, ShellSearch and g_activeMod entirely. Closely comparable mods already do exactly this: quick-launch-media-panel.wh.cpp#L2450 (desktop panel + GSMTC media card, same UI-thread/media-thread split), dynamic-island-for-windows.wh.cpp, explorer-folder-hover-menu.wh.cpp.

5. The shell-PID gate makes the mod fail to start after a reboot or an Explorer restart.

const DWORD desktopShellPid = GetDesktopShellPid();
if (desktopShellPid == 0 || currentPid != desktopShellPid) { return FALSE; }   // 4062

Wh_ModInit runs before the target process starts executing. At that point the new explorer.exe has not created Progman, SHELLDLL_DefView or the shell window yet, so EnumWindows finds nothing, GetShellWindow() and FindWindowW(L"Progman") return NULL (or a stale process), GetDesktopShellPid() returns 0, and the mod bails out. It then stays dead until the user happens to change a setting. So on every boot and every Explorer restart the overlay simply never appears. The tool-mod rewrite in item 4 removes this check entirely; if you keep the Explorer-hosted design, the detection has to be deferred (e.g. to Wh_ModAfterInit plus a EVENT_OBJECT_CREATE/retry path) rather than run at init time.

6. g_session / g_sessionManager are read and written from two threads with no synchronization.

QueryMedia (media thread) writes them at lines 967 and 976, while MediaPrevious/MediaNext/MediaTogglePlayPause/MediaSeek (UI thread, from WM_LBUTTONDOWN) read and call through them at lines 801, 817, 833 and 855. Assigning a WinRT projected type releases the old interface and add-refs the new one; doing that concurrently with another thread dereferencing the same variable is a use-after-free. Clicking next/previous while the poll tick lands is enough to hit it. Guard the pair with a mutex and take a local strong copy before the call — taskbar-fluent-media-player.wh.cpp#L2069 does this with a dedicated g_sessionMtx.

7. The UI thread spins at ~62 Hz forever and re-renders the whole overlay every frame.

while (g_running.load()) {
    while (PeekMessageW(&msg, nullptr, 0, 0, PM_REMOVE)) { ... }
    ...
    if (shouldShow) { RenderFrame(); PresentFrame(); }
    Sleep(FRAME_MS);   // 3813, FRAME_MS = 16
}

Two costs. First, the loop never blocks — even with no media playing and the window hidden it wakes 62 times a second to call PeekMessage, IsWindowVisible, GetLocalTime and QueryPerformanceCounter, which keeps the CPU out of deep idle for the whole uptime of the session. Second, when the overlay is visible it runs a full GDI+ repaint every 16 ms — rounded-rect paths, six Font constructions, MeasureString, six DrawStrings, a bicubic rescale of the album art, and an UpdateLayeredWindow — regardless of whether anything changed.

Use a blocking wait and only redraw when there's something to redraw: MsgWaitForMultipleObjectsEx with a computed timeout, or a plain GetMessageW loop driven by a SetTimer that you only arm while the overlay is visible (and at ~16 ms only while the marquee is actually scrolling — otherwise once per second is enough for the progress readout). quick-launch-media-panel.wh.cpp#L868 arms its 16 ms timer only when an animation is running.

8. The overlay ignores DPI and is hard-wired to the primary monitor.

Every dimension is a fixed physical pixel count (OVERLAY_W = 420, OVERLAY_H = 164, all the *_X/*_Y constants) and every font is UnitPixel at a fixed size. Explorer is Per-Monitor-V2 aware, so nothing scales this for you — on a 150% or 200% display the overlay renders at roughly half or a third of its intended size, with unreadable text. There's also no WM_DPICHANGED handling. On top of that, PositionOverlay uses MonitorFromPoint({0,0}, MONITOR_DEFAULTTOPRIMARY) (line 1455), so on a multi-monitor setup the overlay is pinned to the primary monitor with no way to move it.

Scale all geometry and font sizes by GetDpiForWindow(g_hwnd) / 96.0, recompute on WM_DPICHANGED, and pick the monitor from a setting (or at least from the window's current monitor). See quick-launch-media-panel.wh.cpp#L1942.

9. GSMTC is polled twice a second instead of using its events.

MediaThreadProc calls QueryMedia every MEDIA_POLL_MS = 500, and each pass does GetCurrentSession(), GetPlaybackInfo(), TryGetMediaPropertiesAsync().get() and GetTimelineProperties() — cross-process RPC, forever, even when nothing is playing. The API is event-driven: SessionManager::CurrentSessionChanged, Session::MediaPropertiesChanged, PlaybackInfoChanged, TimelinePropertiesChanged. Subscribing removes the polling entirely and makes track changes instant instead of up to 500 ms late. See island-media-controls.wh.cpp#L10851 and taskbar-fluent-media-player.wh.cpp#L4000. Remember to unregister the tokens on teardown.

10. The mod has no settings block at all.

Everything is a compile-time constant: overlay size and position, every color, the marquee speed, the poll interval, and whether the clock is shown. For a desktop overlay, position (corner / monitor / offset) and the option to hide the clock are the minimum users will ask for. Adding a ==WindhawkModSettings== block with a handful of entries would go a long way — and it pairs naturally with the DPI/monitor fix above.

11. The clock and date formats are hard-coded and ignore the user's regional settings.

swprintf_s(buffer, L"%02d:%02d", st.wHour, st.wMinute);                 // 442
swprintf_s(buffer, L"%02d/%02d/%04d", st.wDay, st.wMonth, st.wYear);    // 462

That's a fixed 24-hour clock and a fixed DD/MM/YYYY date — wrong for every user whose locale uses 12-hour time or MM/DD/YYYY, which is a large share of the audience. Use GetTimeFormatEx / GetDateFormatEx with LOCALE_NAME_USER_DEFAULT (or expose the format string as a setting, as taskbar-clock-customization.wh.cpp does).

12. The README has no screenshot.

This is a purely visual mod — a desktop card with album art, a clock and controls — and the README describes it entirely in prose. Please add a screenshot or a short GIF; only i.imgur.com and raw.githubusercontent.com are allowed as image hosts.

13. Please state how this differs from the existing media mods.

There are already several mods in this space — quick-launch-media-panel (desktop panel with a GSMTC media card), dynamic-island-for-windows, island-media-controls, taskbar-fluent-media-player and taskbar-music-lounge. A standalone floating desktop card that isn't attached to the taskbar does look like a distinct niche, so this isn't a blocker — but the README should say so explicitly so users can tell them apart. quick-launch-media-panel's README has a "How it differs from existing media mods" section that's a good model.

Optional improvements

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

  • Dead code / unused state. g_lastZOrderQpc (lines 288, 1553) is written but never read. UpdateOverlayZOrder(bool force) is only ever called with false. clockChanged and g_cachedMinuteKey are computed every frame but have no effect, because the outer if (shouldShow || clockChanged) immediately re-tests if (shouldShow) inside (lines 3800-3811) — either drop them or make the clock actually repaint on a minute tick while hidden. IsDesktopActive's foreground == g_hwnd branch (lines 1377-1382) is unreachable: the window is WS_EX_NOACTIVATE and returns MA_NOACTIVATE, so it never becomes foreground.

  • Unused dependencies. <shlwapi.h> (line 64) and -lshlwapi are never used — no Str*/Path* call appears in the file. The #pragma comment(lib, ...) block (lines 82-87) is an MSVC-ism that clang/mingw ignores; @compilerOptions already carries the -l flags, so it's dead weight.

  • SecondsSince calls QueryPerformanceFrequency on every invocation (lines 311-313), and it's called several times per frame. The frequency is fixed for the lifetime of the system — read it once into a static const.

  • Media commands block the UI thread. MediaPrevious/MediaNext/MediaTogglePlayPause/MediaSeek all do .get() on a WinRT async operation directly inside WM_LBUTTONDOWN, so the overlay freezes (no repaint, no marquee) for the duration of the cross-process call. Queue the command to the media thread and return immediately.

  • g_hwnd is a plain non-atomic HWND written on the UI thread (line 3429) and read from Wh_ModUninit on the engine thread (line 4241). std::atomic<HWND> costs nothing here.

  • Double DestroyWindow. The WM_CLOSE handler destroys the window (line 3322) but doesn't clear g_hwnd, so the post-loop cleanup (line 3822) calls DestroyWindow again on a dead handle. Clear g_hwnd in WM_NCDESTROY (or WM_DESTROY) instead.

  • GDI+ Font objects are constructed from scratch on every frameDrawTitle, DrawArtist, DrawAlbum, DrawClock (×2), DrawDuration and UpdateTitleMetrics each build one, so ~360 font constructions per second at 60 fps. Create them once (they only change when the DPI changes) and reuse.

  • DrawCover holds g_stateMutex across graphics.DrawImage (line 1749), which is the most expensive call in the frame. That stalls the media thread on every frame. Copying out a Bitmap* under the lock isn't safe either since the media thread may delete it — a shared_ptr<Bitmap> swapped under the lock would let you drop the mutex before drawing.

  • Log prefixes. [UI], [Guard], [Init] in the Wh_Log strings — Windhawk already prefixes the mod name and the function, so these are partly redundant.

  • Class name carries the version (MallssMusicOverlay141). Once UnregisterClass is in place (item 3), drop the suffix — otherwise every version bump orphans another registration.

  • SafeString(hstring value) (line 360) takes its argument by value, copying an hstring per call; const hstring& is free. Also, the whole function is equivalent to std::wstring{value} since an empty hstring already yields an empty wstring.

  • Wh_ModInit starts both threads. For a mod that doesn't hook anything, Wh_ModAfterInit is the more conventional place — it's less constrained, and it's where the process is actually up.

Functionality notes

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

  • The marquee's second title copy is never visible. DrawTitle draws a second string at x + g_cachedTitleWidth + MARQUEE_GAP (lines 2046-2059), but the scroll stops at g_marqueeOffset = -(g_cachedTitleWidth - TITLE_WIDTH) (line 3761). At maximum scroll the second copy sits at area.X + TITLE_WIDTH + 42, which is past the right edge of the clip rect — so it's an extra DrawString per frame that never shows a pixel. Either scroll far enough to wrap (offset down to -(width + GAP)) or delete the second draw.

  • Album art is rescaled every frame. DrawCover runs DrawImage with InterpolationModeHighQualityBicubic from the full-resolution thumbnail down to 92×92, 60 times a second. Scale it once into a 92×92 Bitmap when the track changes and blit that instead — a large cover art image makes this the single most expensive thing in the frame.

  • The progress hit area overlaps the album text. HitTest accepts y in [52, 80] (lines 3154-3155) for a bar that's drawn at y = 66..71, while the album title is drawn at y = 49..63. Clicking the album name seeks the track, which is surprising. A tighter band (say y = 60..78) would still be comfortably clickable.

  • No hover or press feedback. DrawControls passes active as a hard-coded false/true/false (lines 2636-2664), so the play button is permanently highlighted and the prev/next buttons never react to the pointer. Tracking WM_MOUSEMOVE + WM_MOUSELEAVE and feeding the hovered HitArea into DrawMediaButton would make the controls feel a lot more responsive — and you already have the hit-testing for it.

  • Inconsistent "no media" state. With no session, the title shows "No music" but the artist line still shows "Unknown artist", because DrawArtist substitutes on empty (lines 2084-2086) even though HandleNoMedia clears g_artist. Leaving the artist line blank in that state would read better.

  • HWND_BOTTOM hides the overlay completely. UpdateOverlayZOrder drops the window to the very bottom of the z-order when the desktop isn't focused, which puts it below Progman/WorkerW — so it isn't just "behind the active application" as the README says, it's behind the wallpaper. That's probably the intent, but the README wording suggests it would still be visible next to a non-maximized window.

  • Foreground tracking is polled. IsDesktopActive is called every 120 ms via GetForegroundWindow + GetClassNameW. SetWinEventHook(EVENT_SYSTEM_FOREGROUND, ...) gives you the same information as an event and lets the UI loop sleep, which pairs well with item 7 above.


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. waiting-for-ai-review An AI review was requested and is being prepared. and removed waiting-for-ai-review An AI review was requested and is being prepared. 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

@ItsMeMal /ready-for-reviewer can't be applied here: an AI review is being prepared, 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.


The tool-mod structure is the right call here (no hooks, only SetWinEventHook + its own window), the WinRT/thread globals use [[clang::no_destroy]] std::optional<std::thread> correctly, and teardown joins both threads before freeing anything. The following need attention:

1. None of the controls work — HitTest is fed client coordinates but expects screen coordinates.

HitTest converts its arguments to client space itself:

static HitArea HitTest(int screenX, int screenY) {
    RECT rect{};
    GetWindowRect(g_hwnd, &rect);
    const int x = static_cast<int>((screenX - rect.left) / scale);
    const int y = static_cast<int>((screenY - rect.top) / scale);

WM_NCHITTEST's lParam is in screen coordinates, so that call is correct. But WM_LBUTTONDOWN's lParam is in client coordinates, and it's passed to the same function. With the overlay pinned to the bottom-right (rect.left ≈ screen width − 438), x comes out several hundred pixels negative, so every button press falls through to HitArea::None and the whole switch does nothing — previous / play-pause / next / click-to-seek are all dead.

Note that the seek math a few lines below already treats GET_X_LPARAM(lParam) as client-relative (logicalX = GET_X_LPARAM(lParam) / scale), which is the correct reading — so the two are inconsistent with each other. Make HitTest take client coordinates and convert in WM_NCHITTEST:

static HitArea HitTest(int clientX, int clientY) {
    const float scale = g_scale <= 0.0f ? 1.0f : g_scale;
    const int x = static_cast<int>(clientX / scale);
    const int y = static_cast<int>(clientY / scale);
    ...
}

case WM_NCHITTEST: {
    POINT pt{GET_X_LPARAM(lParam), GET_Y_LPARAM(lParam)};
    ScreenToClient(hwnd, &pt);
    return HitTest(pt.x, pt.y) != HitArea::None ? HTCLIENT : HTTRANSPARENT;
}

2. The overlay re-renders at 30–60 fps permanently, including while it is completely hidden.

The UI loop renders whenever g_visible is true, and g_visible is true for as long as a media session exists (playing or paused) — it has nothing to do with whether the overlay is actually on screen:

if (visible) {
    RenderFrame();
    PresentFrame();
}
...
sleepMs = (visible && playing && titleOverflow) ? 16 : (visible && playing) ? 33 : ...

When another app is in the foreground the overlay is moved to HWND_BOTTOM, i.e. underneath everything, but IsWindowVisible is still TRUE, so PresentFrame keeps doing a full UpdateLayeredWindow of a 420×164 32-bit layered surface 30 times a second, plus a complete GDI+ repaint (rounded-rect paths, antialiased text, bicubic album-art scaling) — forever, for nothing. That's constant CPU and battery cost for every user who leaves music playing while they work.

Gate rendering on the overlay actually being on top, and only use the 16 ms cadence when the marquee is actually animating:

const bool onScreen = visible && g_lastDesktopActive;
if (onScreen) { RenderFrame(); PresentFrame(); }

and fall back to a long sleep (or no redraw at all) otherwise, redrawing once when UpdateOverlayZOrder flips the state back to desktop-active.

3. The tool-mod launcher boilerplate deviates from the wiki snippet.

The launcher must be a verbatim copy of the snippet from Mods as tools: Running mods in a dedicated process so it stays maintainable. Two functional pieces are missing:

  • The session 0 guard at the top of Wh_ModInit, which stops the tool process from being spawned in the service session where it can never be seen:

    DWORD sessionId;
    if (ProcessIdToSessionId(GetCurrentProcessId(), &sessionId) && sessionId == 0) {
        return FALSE;
    }
  • The exclusion list only checks -service; the snippet also excludes -service-start and -service-stop.

See mods/explorer-folder-hover-menu.wh.cpp for a verbatim copy of the current snippet. (The extra CloseHandle(g_toolModProcessMutex) in Wh_ModUninit and the dosHeader null check are harmless, but they're also drift — easier to just paste the snippet as-is.)

4. The clock and date ignore the user's locale.

swprintf_s(buffer, L"%02d:%02d", st.wHour, st.wMinute);          // CurrentClock
swprintf_s(buffer, L"%02d/%02d/%04d", st.wDay, st.wMonth, st.wYear); // CurrentDate

This hard-codes a 24-hour clock and DD/MM/YYYY, so users on a 12-hour clock see the wrong format and US users read 09/11/2026 as the wrong date entirely. Use GetTimeFormatEx / GetDateFormatEx with LOCALE_NAME_USER_DEFAULT (and/or expose a format string as a setting) — taskbar-clock-customization does exactly this, see its GetTimeFormatEx/GetDateFormatEx usage.

5. Nothing is configurable, and the position is hard-pinned to the primary monitor.

There's no ==WindhawkModSettings== block at all, and PositionOverlay always uses MonitorFromPoint({0,0}, MONITOR_DEFAULTTOPRIMARY) with fixed RIGHT_MARGIN / BOTTOM_MARGIN. On a multi-monitor setup the user can't move the overlay to the monitor they actually watch, and they can't change the size, the corner, the opacity or the colors either. For a permanently-visible desktop widget that's a big gap — please add at least monitor selection, corner/offset, and the clock format. quick-launch-media-panel is a good reference for what a desktop-panel settings block looks like.

6. Please add a screenshot or GIF to the README.

The README currently says a screenshot "should be added once hosted at an allowed location". For a mod whose entire purpose is a visual overlay, the catalog entry really needs one — users pick these mods by the picture. Allowed hosts are i.imgur.com and raw.githubusercontent.com.

7. Spell out how this differs from the existing desktop media overlays.

The README's "Difference from taskbar media mods" section only compares against taskbar mods, but the two closest neighbours are also desktop overlays running as windhawk.exe tool mods with GSMTC media controls, title/artist/art and a timeline:

The fixed bottom-right card with an integrated clock is a defensible difference, but please say so explicitly in the README, and consider whether an option on one of those mods would serve users better than a fourth entry in this cluster.

Optional improvements

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

  • Dead code. g_forceRedraw is only ever stored, never loaded. minuteChanged / lastMinute in the UI loop are computed and then unused (the render happens every iteration anyway). MarqueePhase::HoldEnd is never assigned, so that case and MARQUEE_END_HOLD are unreachable. g_backBits receives the CreateDIBSection bits pointer that's never read. This is the kind of leftover that AI-assisted drafts tend to accumulate — worth a pass over the file for more of it.

  • [[clang::no_destroy]] on trivially-destructible globals. The attribute is correctly applied to g_uiThread / g_mediaThread (std::optional<std::thread>) and to the WinRT g_session / g_sessionManager — those genuinely need it. But it's also on g_gdiplusToken (ULONG_PTR), g_backDC / g_backBitmap / g_backOldBitmap (handles), g_backBits (void*), g_coverBitmap (raw pointer), g_commandMutex (std::mutex), g_commandCv and g_commandQueue (a deque of plain values). None of those have a destructor that can misbehave at process shutdown, so the attribute is a no-op there and just invites cargo-culting (note g_stateMutex doesn't have it, while g_commandMutex does). Drop it from those. Background: Global objects and process shutdown.

  • g_hwnd is a plain HWND written by the UI thread and read by the media thread (ApplyMedia / HandleNoMedia / WhTool_ModUninit). The consequences are benign (a PostMessage to a just-destroyed window fails harmlessly), but it's still a data race — std::atomic<HWND> costs nothing here.

  • GDI+ objects are rebuilt every frame. DrawTitle, DrawArtist, DrawAlbum, DrawClock, DrawDuration and UpdateTitleMetrics each construct a Gdiplus::Font, SolidBrush and StringFormat per frame, and UpdateTitleMetrics calls MeasureString on every frame even when the title hasn't changed. Font creation in particular is not cheap; cache them (keyed on the DPI scale) and only re-measure when g_cachedTitle changes.

  • The UI loop uses Sleep() instead of waiting on the message queue. With sleepMs up to 250 ms, a posted WM_APP + 20 can sit in the queue for a quarter second before it's dispatched. MsgWaitForMultipleObjectsEx(0, nullptr, sleepMs, QS_ALLINPUT, MWMO_INPUTAVAILABLE) gives you the same pacing but wakes immediately on input.

  • Unload can take several seconds. WhTool_ModUninit joins the media thread, which may be parked in RequestAsync (1.5 s) + TryGetMediaPropertiesAsync (1.5 s) + LoadCoverFromThumbnail (up to 3 s) + a queued command (0.8 s). It's bounded and the process exits right after, but disabling the mod feels hung. Re-checking g_running between the individual waits would cut it down.

  • Dead branch in IsDesktopActive. if (foreground == g_hwnd) return true; can't trigger — the window is WS_EX_NOACTIVATE and returns MA_NOACTIVATE, so it never becomes the foreground window.

  • Window class name hard-codes the version (L"MallssMusicOverlay151"). Use WH_MOD_ID so it doesn't need touching on every release.

  • Log strings. Wh_Log(L"Mallss Music Overlay 1.5.1 started as Windhawk tool") — Windhawk already prefixes the mod name in the log, and WH_MOD_VERSION is available if you want the version. Same for the [UI] prefix in Wh_Log(L"[UI] Overlay created hwnd=%p", ...).

  • Magic numbers in the hit test. The progress-bar hitbox uses y >= 60 && y <= 79 rather than deriving from PROGRESS_Y / PROGRESS_H, and the placeholder music-note glyph in DrawCover is drawn at absolute coordinates (S(49.0f), S(35.0f), …) instead of relative to COVER_X / COVER_Y. Both work today but will silently break if the layout constants move.

  • WM_SETTINGCHANGE returns 0 without calling DefWindowProcW — harmless for this window, but there's no reason not to let it fall through.

  • static std::wstring SafeString(hstring value) takes the hstring by value; a const& is enough.

  • Version 1.5.1 for an initial release is a little odd for a first catalog entry — purely cosmetic.

Functionality notes

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

  • The local playback clock never resyncs, so seeking inside the source app desyncs the progress bar permanently. ApplyMedia only re-anchors on a track change or a play/pause transition; the README frames this as intentional ("Normal media polling does not continuously overwrite the local playback clock"). The downside is that if the user scrubs in Spotify/the browser, the overlay keeps counting from the old anchor until the track changes. A threshold resync gets you both properties — no backwards jitter, no permanent drift:

    const double local = GetLocalPlaybackPositionLocked();
    if (std::abs(snapshot.position - local) > 1.5) {
        snapshot.playing ? StartLocalClockLocked(snapshot.position)
                         : FreezeLocalClockLocked(snapshot.position);
    }

    GetTimelineProperties().LastUpdatedTime() also lets you compensate for how stale the reported position is.

  • The marquee only animates while playing (visible && playing && titleOverflow), so a long title on a paused track is stuck showing its first ~178 px. Probably worth letting it scroll regardless of playback state.

  • Track-change detection is title/artist/album based, so putting the same track on repeat doesn't reset the position anchor or the marquee — the progress bar will read as if the track never restarted. GetTimelineProperties().Position() dropping sharply is a usable secondary signal.

  • Cover loading blocks the command queue. ApplyMediaUpdateCoverLoadCoverFromThumbnail runs on the media thread and can block ~3 s on the two WinRT waits, during which a queued play/pause/next sits unprocessed. Loading artwork on a separate worker (or shortening the timeouts) would keep the buttons responsive.

  • The "Desktop clock" feature only exists while media is available. When there's no session, g_visible goes false and the whole window is hidden — clock and date included. If the clock is meant to be a standalone feature (it's listed as one), consider keeping the window up with just the clock panel when nothing is playing.

  • UpdateOverlayZOrder(true) runs on every system-wide EVENT_SYSTEM_FOREGROUND, forcing a SetWindowPos even when the desktop-active state hasn't changed. Passing false there would let the existing state check skip the redundant calls.


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
@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

This pull request has already had 3 AI reviews in the last 24 hours, which is the limit, so no review was posted this time.

Comment /ai-review again after 2026-09-12 07:31 UTC (in 22 hours) to get another one.

@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
@ItsMeMal

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

This pull request has already had 3 AI reviews in the last 24 hours, which is the limit, so no review was posted this time.

Comment /ai-review again after 2026-09-12 07:31 UTC (in 22 hours) to get another one.

@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
@ItsMeMal

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

This pull request has already had 3 AI reviews in the last 24 hours, which is the limit, so no review was posted this time.

Comment /ai-review again after 2026-09-12 07:31 UTC (in 5 hours) to get another one.

@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
@ItsMeMal

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.


Good progress since the last round — the tool-mod structure is in place, the window class is registered against the mod's own module and unregistered on the way out, the thread and WinRT globals use the right [[clang::no_destroy]] shapes, the hit test now takes client coordinates, and the clock/date go through GetTimeFormatEx/GetDateFormatEx. What's left is concentrated in the window's show/z-order handling and the render loop.

1. Once the overlay hides, it never comes back

WM_MALSS_UPDATE hides the window but never shows it again:

case WM_MALSS_UPDATE: {
    if (g_visible.load()) {
        PositionOverlay();   // SetWindowPos without SWP_SHOWWINDOW
        RenderFrame();
        InvalidateRect(hwnd, nullptr, FALSE);
        UpdateWindow(hwnd);
    } else {
        ShowWindow(hwnd, SW_HIDE);
    }
    return 0;
}

ShowWindow(..., SW_SHOWNOACTIVATE) is called exactly once, right after CreateWindowExW in UIThreadProc, and PositionOverlay's SetWindowPos passes SWP_NOACTIVATE | SWP_NOOWNERZORDER — no SWP_SHOWWINDOW. So the sequence "play something → close the player (HandleNoMedia hides the window) → play something again" renders a fresh frame into a window that is still SW_HIDE, and the overlay stays gone until the mod is restarted.

Add the show back to the visible branch, or fold it into the positioning call:

SetWindowPos(g_hwnd, nullptr, x, y, width, height,
             SWP_NOACTIVATE | SWP_NOOWNERZORDER | SWP_NOZORDER |
                 SWP_SHOWWINDOW);

quick-launch-media-panel.wh.cpp#L927-L931 does exactly this (SWP_NOACTIVATE | SWP_SHOWWINDOW | SWP_NOOWNERZORDER).

2. Every media state change yanks the overlay in front of whatever you're using

PositionOverlay inserts at HWND_TOP and doesn't pass SWP_NOZORDER:

SetWindowPos(g_hwnd, HWND_TOP, x, y, width, height,
             SWP_NOACTIVATE | SWP_NOOWNERZORDER);

and it's called from WM_MALSS_UPDATE (i.e. on every track change and every play/pause), from WM_DPICHANGED, from WM_DISPLAYCHANGE/WM_SETTINGCHANGE — and WM_SETTINGCHANGE is broadcast for all sorts of unrelated system events — and from WhTool_ModSettingsChanged. The result is that the card pops over the user's foreground window every time a track changes. That directly contradicts what the README promises ("Normal application windows naturally cover it when they are opened").

Use SWP_NOZORDER for repositioning and only choose a z-order position when you actually (re)show the window. If you want it pinned just above the wallpaper rather than floating, the insertAfter walk in quick-launch-media-panel.wh.cpp#L903-L925 is a good reference.

3. The UI thread re-renders at 30–60 fps for as long as anything is playing, even when the overlay is completely covered

if (playing || minuteChanged || g_forceRedraw.exchange(false)) {
    RenderFrame();
    InvalidateRect(g_hwnd, nullptr, FALSE);
    UpdateWindow(g_hwnd);
}

int sleepMs = playing && titleOverflow ? 16 : playing ? 33 : 250;
Sleep(sleepMs);

playing alone drives the redraw — nothing checks whether the window is on screen. With a maximized browser in front, the mod still runs a full GDI+ repaint 30 times a second: rounded-rect GraphicsPaths, antialiased text, bicubic album-art scaling, and a fresh Font + SolidBrush + StringFormat constructed inside every one of DrawTitle / DrawArtist / DrawAlbum / DrawClock / DrawDuration / UpdateTitleMetrics. Only the BitBlt gets clipped away; the expensive part has already happened. That's a permanent CPU/battery cost for anyone who leaves music on, and it isn't needed — the elapsed-time text changes once a second and the progress bar moves about a pixel a second on a 286 px bar.

Suggested changes, in order of payoff:

  • Use the 16 ms cadence only while the marquee is actually in MarqueePhase::Scroll; 4–10 Hz covers the clock and progress bar otherwise.
  • Skip RenderFrame when the overlay isn't visible on screen. A SetWinEventHook on EVENT_SYSTEM_FOREGROUND (which you had in an earlier revision) or a cheap GetForegroundWindow check is enough to know when to go quiet, and you already have g_forceRedraw to force one redraw when it comes back.
  • Cache the fonts and brushes, keyed on g_scale, instead of rebuilding them per frame.

While you're in that loop: Sleep means the thread stops pumping messages for up to 250 ms, so a click on play/pause can sit in the queue that long before it's even dispatched, and a cross-process broadcast SendMessage to this window blocks its sender for the same time. MsgWaitForMultipleObjectsEx(0, nullptr, sleepMs, QS_ALLINPUT, MWMO_INPUTAVAILABLE) gives you the same pacing but wakes immediately on input or a posted WM_MALSS_UPDATE.

4. The README still has no screenshot or GIF

This is a purely visual mod — a corner card is exactly the kind of thing users judge from the catalog picture, and right now there's nothing to look at. Please add a screenshot (or a short GIF showing the marquee and the controls). Allowed image hosts are i.imgur.com and raw.githubusercontent.com.

5. Say explicitly how this differs from the existing desktop media overlays

The README's "Difference from other desktop media overlays" section doesn't name anything it's differing from:

The focus of this mod is a compact corner card combining album artwork, media controls, clock and date in one desktop-oriented widget.

The two closest neighbours are also windhawk.exe tool mods drawing a desktop surface from GSMTC with artwork, metadata, a timeline and transport controls:

The fixed corner card with an integrated clock/date is a defensible difference, but please name those two in the README and state what this does that they don't. The maintainer's consistent preference is to add an option to an existing mod over merging a fourth entry in the same cluster, so it's worth making the case up front.

Optional improvements

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

  • WhTool_ModSettingsChanged does UI and GDI work on the wrong thread. It runs on an arbitrary Windhawk thread, and from there it calls LoadSettings() (which reassigns g_settings.monitor, a std::wstring the UI thread reads in GetSelectedMonitor), DestroyBackBuffer() (which DeleteDCs the DC the UI thread may be drawing into at that moment) and RenderFrame() (a second GDI+ Graphics on the same g_backDC, plus unsynchronised writes to g_cachedTitle* via UpdateTitleMetrics). It's only reachable on a settings change, but the fix is cheap: post a private message to the overlay window and do all of it in the window proc, so everything stays on the UI thread.

  • PresentFrame is dead code. Declared at line 383, defined at line 3521, never called — the loop and WM_MALSS_UPDATE both go through InvalidateRect + UpdateWindow instead. Worth deleting.

  • [[clang::no_destroy]] on globals that don't need it. It's correct and necessary on g_uiThread / g_mediaThread and on the WinRT g_session / g_sessionManager. But it's also on g_commandMutex (std::mutex), g_commandCv (std::condition_variable), g_commandQueue (a deque of plain values), g_coverBitmap (a raw pointer) and g_gdiplusToken (ULONG_PTR) — none of those have a destructor that can misbehave at process shutdown, so the attribute is a no-op there and invites cargo-culting. Note that g_stateMutex doesn't have it while g_commandMutex does, which is a good illustration that it isn't doing anything. Background: Global objects and process shutdown.

  • ReadStringSetting reinvents WindhawkUtils::StringSetting. #include <windhawk_utils.h> and use the RAII wrapper instead of the manual Wh_GetStringSetting + Wh_FreeStringSetting pair. Also, Wh_GetStringSetting never returns NULL (it returns L"" when unset or on error), so the value ? value : L"" check can go.

  • QpcSecondsSince calls QueryPerformanceFrequency on every invocation (line 427). The frequency is fixed for the lifetime of the system — query it once into a global. It's called several times per rendered frame.

  • Log prefixes. Wh_Log(L"[UI] Overlay created hwnd=%p", ...), [Media] — Windhawk already prefixes log lines with the mod name, so the tags are noise. (Wh_Log itself is cheap and off by default; the call sites are fine.)

  • The window class name hard-codes the version (L"MallssMusicOverlay151"). WH_MOD_ID is available as a wide-string literal and doesn't need touching on every release.

  • -loleaut32 in @compilerOptions looks unused — nothing here uses BSTR/VARIANT. Worth checking whether the build still links without it.

  • Launcher boilerplate drift. It's very close to the wiki snippet now, but the GetModuleFileName check lost the truncation case. The snippet is:

    switch (GetModuleFileName(nullptr, currentProcessPath,
                              ARRAYSIZE(currentProcessPath))) {
        case 0:
        case ARRAYSIZE(currentProcessPath):
            Wh_Log(L"GetModuleFileName failed");
            return;
    }

    Pasting it verbatim (including the reformatting) keeps future maintenance easy — see the bottom of explorer-folder-hover-menu.wh.cpp.

  • DrawCover holds g_stateMutex across the bicubic DrawImage (lines 2579–2611), which blocks the media thread's ApplyMedia/HandleNoMedia for the duration of a full-quality scale. Taking a local copy of the pointer under the lock isn't safe here (the media thread can delete it), but an std::shared_ptr<Gdiplus::Bitmap> swapped under the lock would let you draw outside it.

  • static std::wstring SafeString(hstring value) takes the hstring by value; a const& is enough.

  • WM_SETTINGCHANGE returns 0 without calling DefWindowProcW — harmless for this window, but there's no reason to swallow it.

  • A failed RegisterClassExW/CreateWindowExW leaves the tool process running with no UI. UIThreadProc returns early, but WhTool_ModInit has already returned TRUE and the media thread keeps polling GSMTC forever with nothing to draw to. Signalling the failure back (so the process exits) would be cleaner.

  • Formatting. The repo has a .clang-format; running it would cut the file down a lot — one argument per line plus a blank line between every statement is what turns roughly 1500 lines of logic into 4852 — and make future diffs far easier to read.

Functionality notes

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

  • The seek hit area overlaps the album name and the time labels. HitTest treats y in [58, 82] as the progress bar, but the album text is drawn at y 49–63 and the elapsed/total labels at y 72–87. So clicking the album name or the duration text seeks the track. Tightening the band to roughly the bar plus the knob (y 62–76) would avoid the surprise.

  • The clock and date vanish when nothing is playing. HandleNoMedia hides the whole window, so the two features the README lists as standalone ("Clock and date") only exist while media is available. Keeping the window up with just the clock panel when there's no session would make that promise hold.

  • The marquee only animates while playing (playing && titleOverflow in the UI loop), so a long title on a paused track is frozen showing its first ~178 px. Probably worth letting it scroll regardless of playback state.

  • The local playback clock never resyncs, so scrubbing inside the source app desyncs the progress bar until the track changes. ApplyMedia only re-anchors on a track change or a play/pause transition. A threshold resync gets you both properties — no polling jitter, no permanent drift:

    const double local = GetLocalPlaybackPositionLocked();
    if (std::abs(snapshot.position - local) > 1.5) {
        snapshot.playing ? StartLocalClockLocked(snapshot.position)
                         : FreezeLocalClockLocked(snapshot.position);
    }
  • Cover loading blocks the command queue. ApplyMediaUpdateCoverLoadCoverFromThumbnail runs on the media thread and can block ~3 s across the two WinRT waits; a play/pause queued during that window sits unprocessed until it finishes. Loading artwork on a separate worker (or shortening the timeouts) would keep the buttons responsive.

  • Monitor selection by \\.\DISPLAY1DISPLAY4 is fragile. GDI device numbers aren't necessarily the numbers shown in Windows Settings, and after docking/undocking a machine can end up with e.g. \\.\DISPLAY5 on a two-monitor setup — in which case none of the four options match and GetSelectedMonitor silently falls back to the primary. Enumerating monitors and offering them by index (or by EnumDisplayDevices friendly name) would be more predictable.

  • Track-change detection is title/artist/album based, so putting the same track on repeat doesn't reset the position anchor or the marquee — the progress bar reads as if the track never restarted. GetTimelineProperties().Position() dropping sharply is a usable secondary signal.


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
@ItsMeMal

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 12, 2026
@m417z

m417z commented Sep 12, 2026

Copy link
Copy Markdown
Member

Please explain why the review items are left unaddressed. Start from these items:

Say explicitly how this differs from the existing desktop media overlays

The README still has no screenshot or GIF

The tool-mod launcher boilerplate deviates from the wiki snippet.

The launcher must be a verbatim copy of the snippet from Mods as tools: Running mods in a dedicated process so it stays maintainable. ... See mods/explorer-folder-hover-menu.wh.cpp for a verbatim copy of the current snippet.

@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 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