Skip to content

Update Win32 UI modernizer#4850

Merged
m417z merged 14 commits into
ramensoftware:mainfrom
crazyboyybs:Win32-UI-Modernizer
Jul 21, 2026
Merged

Update Win32 UI modernizer#4850
m417z merged 14 commits into
ramensoftware:mainfrom
crazyboyybs:Win32-UI-Modernizer

Conversation

@crazyboyybs

Copy link
Copy Markdown
Contributor

Changelog

  • Fixed the mod getting stuck on unloading
  • Fixed the glyphs blocking the File Explorer thread while animating
  • Made the glyphs use direct composition when inside File Explorer
  • Fixed black background behind glyphs
  • Styled the Registry Editor separator
  • Styled the old address bar's overflow chevron
  • Enhanced text rendering
  • Added a new Fade animation option for the navigation panel pill
  • The buttons and combo boxes styles are now closer to the winui ones
  • Changed the focus border option into a drop down and fixed the style not being applied
  • Added styles for missing controls
  • Corrected non dpi aware styles
  • Fixed the "Optimize Drives" window not being painted dark
  • Fixed leaks
  • Removed dead code
  • Hardened glyphs detection

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.

* Fixed the mod getting stuck on unloading
* Fixed the glyphs blocking the File Explorer thread while animating.
* Made the glyphs use direct composition when inside File Explorer
* Fixed black background behind glyphs
* Styled the Registry Editor separator
* Styled the old address bar's overflow chevron
* Enhanced text rendering
* Added a new Fade animation option for the navigation panel pill
* The buttons and combo boxes styles are now closer to the winui ones
* Changed the focus border option into a drop down and fixed the style not being applied
* Added styles for missing controls
* Corrected non dpi aware styles
* Fixed the "Optimize Drives" window not being painted dark
* Fixed leaks
* Removed dead code
* Hardened glyphs detection
@m417z

m417z commented Jul 21, 2026

Copy link
Copy Markdown
Member

Submission review

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

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

Not sure about 2. Most other items seem like actual issues.


1. Deadlock on unload — CheckAnims_Cleanup holds g_checkAnimsMutex across a cross-thread subclass removal. (CheckAnims_Cleanup, lines 3646–3660)

std::lock_guard<std::recursive_mutex> lk(g_checkAnimsMutex);   // 3648
for (auto& [key, anim] : g_checkAnims) {
    ...
    KillTimer(hw, kCheckTimerId);
    WindhawkUtils::RemoveWindowSubclassFromAnyThread(hw, CheckBoxSubclassProc); // 3654 — lock still held
    InvalidateRect(hw, nullptr, TRUE);
}

RemoveWindowSubclassFromAnyThread is implemented as a synchronous SendMessage to the checkbox's owning UI thread. CheckBoxSubclassProc takes g_checkAnimsMutex at the top of every message (line 3547). So when the checkbox lives on a different thread than the one running Wh_ModUninit (routine in multi-UI-thread Explorer): the teardown thread holds the mutex and blocks in SendMessage; the checkbox's thread enters CheckBoxSubclassProc, blocks on the mutex, and never pumps the message — classic lock-and-SendMessage deadlock, and the recursive mutex doesn't help since it's a different thread. This is the one cleanup that departs from the pattern you already use everywhere else (ButtonPopCleanup line 5223, BreadcrumbChevronCleanup 5372, PlacesBarCleanup 26665, TabPillRemoveAll 27130, AuxiliarySubclassCleanup 27493). Fix it the same way — copy the HWNDs into a local vector under the lock, g_checkAnims.clear(), release the lock, then loop doing KillTimer / RemoveWindowSubclassFromAnyThread / InvalidateRect on the locals.

2. Teardown hang — the unbounded g_pillDCMutex acquire defeats the bounded pill-thread join. (Wh_ModUninit, join at 37221–37240, release at 37388–37391)

The pill thread is joined with a deliberately bounded 500 ms wait (37240), and the comment at 37231–37237 spells out the reasoning: if the thread is stuck in a GPU/DXGI call (driver TDR, RDP/virtualized GPU), time out and leak rather than hang. But 150 lines later:

std::lock_guard<std::mutex> lk(g_pillDCMutex);   // 37389 — unbounded
PillDCompRelease_Locked();

The pill worker holds g_pillDCMutex for the whole duration of its GPU draw/commit (it try_locks at 7341 and calls PillDCompDrawFrame_Locked under the lock at 7368). In exactly the stuck-driver scenario the bounded join was written for, the thread is wedged while holding g_pillDCMutex → the 500 ms join times out (pillThreadJoined == false), and then line 37389 blocks forever on that same mutex. The careful bounded join buys nothing. Fix: gate the DComp release on pillThreadJoined (skip + leak the device, consistent with your own 37231–37237 philosophy), or acquire with a bounded try_lock and skip on failure.

3. Three raw subclasses are never removed on unload — dangling WndProc into the freed mod image. (installs at 10677, 13955, 27552)

TreeCursorSubclassProc (kTreeCursorSubId, nav-pane TreeView cursor), SplitButtonChevronSubclassProc (kSplitButtonChevronSubId, split buttons), and ClassicDlgToolbarSubclassProc (kClassicDlgToolbarSubId, #32770 dialogs) are installed with raw SetWindowSubclass and removed only in their own WM_NCDESTROY. The teardown sweep RemoveSubclassesFromWindow (line 27985) removes TreeViewSubclass / dark-mode / modern-border, but not these three (I grepped all removal sites — each of these IDs/procs appears only at install + its own WM_NCDESTROY). So when the mod is disabled while such a window is alive — e.g. the Explorer nav pane is long-lived, so TreeCursorSubclassProc is essentially always still installed — the subclass proc stays in comctl32's chain after the DLL unloads, and the next message (a WM_SETCURSOR from moving the mouse over the nav pane) dispatches into unmapped memory → crash. This is pre-existing (unchanged by this PR), but it's a real unload crash. Track their HWNDs in owner sets like g_fontViewSubclassOwners and remove them via RemoveWindowSubclassFromAnyThread in Wh_ModUninit, or fold them into the RemoveSubclassesFromWindow per-window sweep (which already runs under EnumWindows/EnumChildWindows with a PID filter).

Related, same theme (lower confidence): NavDividerFadeTimerProc is the mod's only TIMERPROC-based timer, killed at teardown via EnumWindowsNavDividerClearWindowKillTimer (16575) on the arbitrary teardown thread. If a fade is armed at unload (mouse on/near the divider) and the divider window is on a different thread, a cross-thread KillTimer that fails to remove the timer would let a post-unload WM_TIMER reach the freed TIMERPROC → crash. Worth confirming the cross-thread KillTimer path; the safe move is to kill it from the divider's own thread (or ensure it's disarmed under the unloading flag before the sweep).

4. Custom D2D UI is scaled from a single primary-monitor DPI snapshot (g_Dpi), so it's mis-sized on mixed-DPI multi-monitor setups.

g_Dpi is assigned exactly once, in Wh_ModInit from the primary monitor (line 36782), and never updated afterward — the WM_DPICHANGED handler (21145) updates only per-window state. Yet g_Dpi is used as the scale factor in ~40 D2D paint paths for elements that can appear on any monitor: animated tree chevrons (PaintTreeViewGlyph 7855), travel-band glyphs (5439), tab pill (8245), nav-pane pill (2648/5681), breadcrumb chevron (6147), etc. On a laptop-100% + external-150% config (increasingly common), every one of these renders at the primary monitor's scale on the secondary monitor — under/oversized glyphs and mis-sized pills — and likewise after a runtime primary-DPI change. You already do the right thing in several places (hwnd ? GetDpiForWindow(hwnd) : g_Dpi at 12833/12979/14283, and the explicit per-window scale at 17513–17515), and you even document the limitation (comment at 16311–16313). This is really "finish the migration you started": in the paint paths where the target window/HDC is in hand, derive DPI per-paint via GetDpiForWindow(WindowFromDC(hdc)) / g_tlsPaintHwnd instead of the fixed g_Dpi.

Optional improvements

Minor polish — none of this affects users in normal single-DPI use, so it's your call.

  • DrawTextExW_hook mutates the caller's lprc. (lines 22629–22630) In the winver button-suppression branch it calls DrawTextExW_orig(..., lprc, format | DT_CALCRECT, lpdtp) — passing the caller's own RECT with DT_CALCRECT added, which makes DrawTextEx write the measured box back into *lprc. The caller issued a drawing call and doesn't expect its rect mutated. Impact is negligible (winver's About dialog, and you already draw the button yourself), but the fix is what the sibling hooks already do — measure into a local copy: RECT tmp = *lprc; DrawTextExW_orig(hdc, lpchText, cchText, &tmp, format | DT_CALCRECT, lpdtp);.
  • Unused link dependency. -lversion on the @compilerOptions line (line 15) links version.dll, but no GetFileVersionInfo*/VerQueryValue is used anywhere — drop it.
  • Redundant NULL checks on string settings (e.g. lines 28073, 28085, 28117, 28142, 28184). Wh_GetStringSetting / WindhawkUtils::StringSetting never return NULL — they return L"" when unset — so if (!s) branches are dead. Harmless, just noise.
  • Misleading $options label: MenuCornerStyle defaults to round (line 341), but the dropdown labels smallround as "Small round (default)" (line 350). Move the "(default)" annotation to the round entry, or drop it.

Functionality notes

Non-critical observations about the feature behavior itself.

  • The dui70 PaintBackground hook may silently not install on 32-bit. The symbol lookup string (line 37024) is built with DUI_STDCALL, which is __stdcall on x86 (line 28301). A normal C++ member function is __thiscall on x86, so if the 32-bit dui70 export is decorated __thiscall, HookSymbols won't resolve it and the DirectUI background feature quietly turns off on 32-bit processes (x64 is definitively correct — there DUI_STDCALL is __cdecl, modeling the thiscall register passing fine). Since the mod is @include *, worth a quick check against a 32-bit dui70 PDB; if it doesn't match, the mod otherwise degrades gracefully.
  • Glyph background threads use a best-effort 2 s bounded wait at unload (37285–37291, polling g_glyphBgThreadCount). Their final writes are serialized against the teardown clears via g_glyphMapMutex/g_glyphTreeMutex, so there's no data race; the only tail case is a map build exceeding 2 s, after which teardown proceeds and a still-running thread could touch freed code. Low risk given the bounded work — just noting it since it's the one non-joined worker.

@crazyboyybs

Copy link
Copy Markdown
Contributor Author

Submission review

@m417z Addressed everything, all 4 required items plus the optional polish:

  • Fixed the CheckAnims_Cleanup deadlock (releases the mutex before the cross-thread subclass removal now, matching the pattern the other cleanup functions already use)
  • Fixed the g_pillDCMutex teardown hang (the DComp release is now gated on pillThreadJoined, so it's skipped instead of blocking if the bounded join already gave up)
  • TreeCursorSubclassProc/SplitButtonChevronSubclassProc/ClassicDlgToolbarSubclassProc are now tracked and removed on unload; also closed the related NavDividerFadeTimerProc race you flagged
  • Migrated the D2D paint paths off the stale g_Dpi snapshot onto per-window DPI
    Plus the DrawTextExW rect mutation, the unused -lversion, the redundant NULL checks, and the MenuCornerStyle default label.

One open question on my end: the dui70 32-bit __thiscall concern, I don't have a way to check against a real 32-bit PDB from here, so I added __thiscall as a second symbol-name alternative alongside the existing __stdcall one rather than guessing and replacing it. If you've got a way to confirm which one the real export uses, let me know and I'll clean it up to just the correct one

@m417z

m417z commented Jul 21, 2026

Copy link
Copy Markdown
Member

I don't have a way to check against a real 32-bit PDB from here

Why not? Isn't using Windhawk Symbol Helper on C:\Windows\SysWOW64\dui70.dll enough for checking that?

@crazyboyybs

Copy link
Copy Markdown
Contributor Author

I don't have a way to check against a real 32-bit PDB from here

Why not? Isn't using Windhawk Symbol Helper on C:\Windows\SysWOW64\dui70.dll enough for checking that?

Didn't thought of that lol, one second

@crazyboyybs

Copy link
Copy Markdown
Contributor Author

I don't have a way to check against a real 32-bit PDB from here

Why not? Isn't using Windhawk Symbol Helper on C:\Windows\SysWOW64\dui70.dll enough for checking that?

@m417z Done and sorry for the multiple commits, I got a littlle confused about how symbol naming was supposed to work. I ran Windhawk Symbol Helper against C:\Windows\SysWOW64\dui70.dll and confirmed DirectUI::Element::PaintBackground really is __thiscall on x86, not __stdcall, fixed it directly instead of hedging with two symbol variants. Should be clean now

@m417z
m417z merged commit c313853 into ramensoftware:main Jul 21, 2026
4 checks passed
@m417z

m417z commented Jul 21, 2026

Copy link
Copy Markdown
Member

Thanks.

By the way:

// Wh_SetFunctionHook + Wh_ApplyHookOperations must never run from inside a
// hook callback (or Wh_ModInit) -- only from a deferred handler -- per the
// Windhawk modding skill. NavDividerTrackAndGetHwnd runs inside the
// DrawThemeBackground(Ex) hook callback, so it can't call them directly.
// Spawning a one-shot thread here defers the actual call to a separate
// call stack, satisfying that rule while still installing the SetCursor
// hook lazily -- only in processes that actually show a real nav-pane
// divider, instead of unconditionally in Wh_ModInit for every process
// under this mod's @include *.

That's not true, a thread is not needed in this case, but not incorrect either, just inefficient. FYI for future updates if you want to improve it.

But in this case, why not just hook it in Wh_ModInit, or RgQueueRequiredHooks if you prefer having it hooked only when the option is enabled?

Or maybe it's all AI-generated and doesn't really matter as long as it works :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants