Skip to content

Add taskbar clock to left - #5189

Open
pleromyst wants to merge 11 commits into
ramensoftware:mainfrom
pleromyst:add-taskbar-clock-to-left
Open

Add taskbar clock to left#5189
pleromyst wants to merge 11 commits into
ramensoftware:mainfrom
pleromyst:add-taskbar-clock-to-left

Conversation

@pleromyst

@pleromyst pleromyst commented Aug 23, 2026

Copy link
Copy Markdown

Adds a new Windows 11 mod that moves the native clock and notification-center button to the left side of the taskbar.

The mod is designed to:

  • Keep the centered Start button and application icons in their original position.
  • Avoid leaving an empty clock area on the right side.
  • Remain compatible with clock customization mods that increase the clock width.
  • Safely restore the original taskbar layout when the mod is disabled or unloaded.
  • Protect deferred operations and event handlers during unloading.

Credits:

Taskbar-related implementation techniques were informed by existing Windhawk mods by Michael Maltsev (@m417z), particularly Taskbar Clock Customization and Multirow Taskbar. Development and code review were assisted by OpenAI Codex.

Changelog

Not applicable. This pull request introduces a new mod.

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): OpenAI Codex
    • 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.

Add Taskbar Left Performance Clock based on m417z's Taskbar Clock Customization.
Moves the native Windows 11 clock to the left without shifting centered taskbar apps.
@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 Aug 23, 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.

@pleromyst

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 Aug 23, 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.


The overall approach is sound — hooking TaskbarFrame::SystemTrayExtent to keep the layout baseline instead of inserting spacer columns is a nice idea, and the module resolution follows the established Taskbar.View.dll/SystemTray.dll pattern. The main problems are in the teardown path, which is built around cross-thread polling instead of a synchronous UI-thread round trip, and in where the left host is placed in the visual tree.

1. Global containers hold strong XAML/WinRT references and are destroyed at process shutdown

g_movedClocks (line 92) holds MovedClockData::leftHost, a strong Controls::Grid (line 62), and g_deferredClocks (line 91) holds DeferredClockData::moveOperation, a strong IAsyncOperation<bool> created by the taskbar's CoreDispatcher (line 80). Wh_ModUninit is not called when Explorer terminates (restart, sign-out, reboot), but the CRT destructors of these globals are — on the shutdown thread, after every other thread has been killed and the XAML core is gone. Releasing UI-thread XAML objects there can crash the host.

Both need the [[clang::no_destroy]] std::optional<...> treatment, with the explicit release kept in Wh_ModUninit on the taskbar UI thread:

[[clang::no_destroy]] std::optional<std::vector<DeferredClockData>>
    g_deferredClocks{std::in_place};
[[clang::no_destroy]] std::optional<std::vector<MovedClockData>>
    g_movedClocks{std::in_place};

Use reset() (which runs the element destructors), not clear(). See https://github.com/ramensoftware/windhawk/wiki/Global-objects-and-process-shutdown — sections #4-xaml-and-other-ui-thread-objects and #5-containers-of-resource-owning-or-thread-affine-elements match this case exactly. tray-utility-customizer is a good in-repo reference.

2. g_movedClocks is unsynchronized, and teardown mutates the XAML tree from an arbitrary thread

g_movedClocks has no lock at all (only g_deferredClocks is guarded by g_deferredMutex), yet it is written from two different threads:

  • The taskbar UI thread, via MoveClock/RestoreClock — reached from DateTimeIconContent_OnApplyTemplate_Hook (line 774) and BadgeIconContent_get_ViewModel_Hook (line 793). Neither hook takes a CallbackScope or checks g_callbacksEnabled, and hooks stay installed for the whole of Wh_ModBeforeUninit, so UpdateClockPlacementRestoreClock can be erasing from the vector on the UI thread while Wh_ModBeforeUninit iterates it.
  • The Windhawk unload/settings thread, via RestoreAllMovedClocks (line 727), which reads g_movedClocks.front()/empty() directly.

Concurrent std::vector::erase and iteration is UB — this can crash Explorer on unload.

Worse, RestoreAllMovedClocks calls CleanupMovedClockData() at line 731 directly on the calling thread and outside the try block. That path runs RemoveLeftHostroot.Children().IndexOf(...) / RemoveAt(...), i.e. a XAML tree mutation from a non-UI thread. That throws RPC_E_WRONG_THREAD, and the exception escapes Wh_ModUninit into the Windhawk engine. It also destroys the strong Grid reference on the wrong thread.

The fix is to stop touching g_movedClocks and XAML from anywhere but the taskbar UI thread: do the whole restore synchronously on that thread. tray-utility-customizer and taskbar-folder-menus both use a RunFromWindowThread helper (a SendMessage-based synchronous round trip to the taskbar window's thread) for exactly this.

3. Teardown can hang, spin at 100% CPU, or silently leave a mod-owned delegate registered

The unload path is built entirely on TryRunAsync + polling, and every failure mode of that is unhandled:

  • WaitForOperations (line 147) polls with Sleep(10) and has no timeout — if the taskbar UI thread isn't pumping, Wh_ModUninit blocks forever and Explorer's mod unload hangs. Same for while (g_runningCallbacks.load() != 0) Sleep(10); (line 722).
  • RestoreAllMovedClocks's while (!g_movedClocks.empty()) loop only makes progress if the dispatched lambda actually runs. CoreDispatcher::TryRunAsync completes with false (without invoking the callback) when the dispatcher's thread is shutting down. In that case the entry is never erased, WaitForOperations returns immediately, and the loop spins forever with no sleep — a 100% CPU hang.
  • Same problem in the reverse direction in StopDeferredCallbacks (line 697): the Loaded handler revocation is dispatched with TryRunAsync and only best-effort. If it doesn't run, a RoutedEventHandler whose code lives in the mod image stays registered on a live FrameworkElement. Windhawk FreeLibrarys the mod right after Wh_ModUninit returns, so the next Loaded event jumps into unmapped memory.

Replacing the async dispatch + polling with a synchronous RunFromWindowThread call (see item 2) removes all three: the callback either runs to completion before you return or the window is already gone, and there's nothing left to poll for.

4. HookSymbols is called twice for the same module on most Windows 11 builds

GetTaskbarViewModule() (line 897) and GetSystemTrayModule() (line 881) return the same handle whenever SystemTray.dll doesn't exist and Taskbar.View.dll is older than 2604 — i.e. on Windows 11 21H2 through 24H2, the common case (and also in the ExplorerExtensions.dll fallback). Wh_ModInit then runs TryHookTaskbarView(module, ...) and TryHookSystemTray(module, ...) back to back (lines 1001-1013), which is two HookSymbols calls against one module.

HookSymbols caches the resolved symbols per module, and a second call for the same module invalidates that cache and forces a full re-resolution — the mod will be noticeably slow to load every time. Collect the hooks into a single array and issue one HookSymbols call per distinct module handle.

5. The left host overlays the leftmost taskbar elements and steals their clicks

data.leftHost is appended as the last child of the taskbar root Grid (line 462) with HorizontalAlignment::Left and ColumnSpan/RowSpan covering the whole grid. That root Grid is the container that holds Taskbar.TaskbarFrame (which spans the full taskbar width) and SystemTray.SystemTrayFrame, so a left-aligned, last-added child sits on top of whatever TaskbarFrame draws at x=0:

  • The Widgets/weather button (Taskbar.AugmentedEntryPointButton) is the leftmost taskbar item and is enabled by default — the clock will be drawn over it and will take its clicks.
  • With Taskbar alignment = Left (Settings → Personalization → Taskbar → Taskbar behaviors), the Start button and app icons start at x=0, so Start becomes unclickable.

The mod needs to either reserve real space at the left (e.g. shift TaskbarFrame's content by the host width, mirroring what you already do for SystemTrayExtent on the right) or at minimum offset the host past the widgets button and handle/document the left-alignment case.

6. Add a screenshot to the README

This is a purely visual mod and the README has no image. Please add a screenshot (or GIF) showing the clock on the left — only i.imgur.com and raw.githubusercontent.com are allowed image hosts.

Optional improvements

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

  • Wh_ModBeforeUninit (line 1045) and Wh_ModUninit (line 1051) are byte-for-byte identical. Once the restore is done in Wh_ModBeforeUninit the second call is a no-op; keep just one (Wh_ModUninit is the natural place, since Wh_ModBeforeUninit's only advantage — hooks still installed — isn't used here).
  • Wh_ModSettingsChanged(BOOL*) never touches *bReload. When the mod never asks for a reload, the simpler void Wh_ModSettingsChanged() overload is equivalent and clearer.
  • RefreshTaskbarClock() is called from TryHookSystemTray(module, /*applyImmediately=*/true) (line 966), which is reachable from LoadLibraryExW_Hook — i.e. it does EnumWindows plus three registry calls while holding the loader lock. It's also pointless at that moment: SystemTray.dll has only just loaded, so no clock element exists yet and the OnApplyTemplate hook will pick it up naturally. Consider limiting the refresh to Wh_ModAfterInit/Wh_ModSettingsChanged.
  • Related: Wh_ModAfterInit can call RefreshTaskbarClock() twice — once via TryHookSystemTray(module, true) (line 1036) and once at line 1041.
  • The MoveClockToLeft setting is the mod's only setting and its false state is equivalent to disabling the mod, so it mostly adds a second, redundant off switch (plus the wasEnabled/isEnabled bookkeeping and the whole restore-without-unload path). Unless you plan to add more options, dropping it would simplify a fair amount of code.
  • MovedClockData::originalIndex is captured at move time and reused on restore. If tray items were added or removed meanwhile, the clock comes back at a slightly different position. Anchoring on a sibling element (or just appending) would be more robust.
  • The PR description credits Taskbar Clock Customization / Multirow Taskbar and Codex, but the README doesn't. Windhawk's model is one @author per mod, so additional credits belong in the README text — worth moving them there so they survive the merge.

Functionality notes

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

  • MeasureNativeClockLine (line 237) reconstructs the native clock width from a synthetic TextBlock with a hardcoded Segoe UI Variable Text / 12px font, then clamps the result to [64, 180] (line 284). That's a lot of guessing: it ignores the user's text-scaling setting, the taskbar's actual clock font/FontSize (which can differ by build and theme), the notification badge, and configurations where the date line is hidden. Whenever the estimate is off, the centered app group shifts by exactly that error — the thing the mod exists to prevent. A more robust source for the baseline is the value the taskbar itself hands you: record the last SystemTrayExtent value seen for a given XamlRoot before the clock is moved, then reserve previousValue - currentValue afterwards. That's exact, needs no font assumptions, and is naturally per-monitor and per-locale.
  • Preserving the original extent is a deliberate choice (documented in the README), but it does leave a clock-sized empty gap between the app group and the remaining tray. Some users will prefer the tray to collapse and the apps to re-center; if you keep the MoveClockToLeft setting, this might be the more useful thing to expose as an option.
  • MoveClock bails out unless the clock's parent is exactly SystemTrayFrameGrid (line 390). That's a reasonable guard, but it means the mod silently does nothing if another tray mod (e.g. Taskbar Multi-Tray, Island Media Controls) has already reparented the button. The Wh_Log message covers debugging, but it may be worth a README note about mod ordering.
  • Only the primary/secondary taskbar XamlRoots that go through DateTimeIconContent::OnApplyTemplate get handled. If a secondary taskbar's clock is created before the mod is enabled and RefreshTaskbarClock() doesn't retrigger the template on it, that monitor keeps the clock on the right until the next tray refresh — worth testing on a multi-monitor setup with "Show taskbar on all displays" enabled.


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 Aug 23, 2026
@pleromyst

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 Aug 24, 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.


Overall the mod is in good shape: the scope is narrow (explorer.exe), the symbol-hook module comments match what the arrays are actually resolved against, LoadLibraryExW is correctly hooked in kernelbase.dll, and the [[clang::no_destroy]] std::optional<> treatment of g_movedClocks (which holds a strong Controls::Grid) is exactly right. The items below are mostly about the teardown path and about hook setup running repeatedly.

1. A failed teardown leaves live XAML delegates pointing into the unloaded mod image (crash).

RunFromWindowThread uses SendMessageTimeout(..., SMTO_ABORTIFHUNG | SMTO_BLOCK, 5000, ...), and RunOnTaskbarThread treats failure as "log and move on". So if the taskbar thread is busy/considered hung, or FindExplorerTaskbarWindow returns nothing, StopDeferredCallbacksOnTaskbarThread never runs — and the Loaded / LayoutUpdated handlers registered in RegisterLoadedHandler / RegisterLayoutUpdatedHandler are lambdas whose code lives in the mod image. Windhawk unloads the mod with a single FreeLibrary right after Wh_ModUninit returns, so the next time XAML raises those events it calls into unmapped memory. (g_deferredClocks.reset() / g_movedClocks.reset() are also skipped, leaking the containers permanently since they're no_destroy.)

There's a second, narrower problem with the timeout: RunParameters lives on RunFromWindowThread's stack. On timeout the function returns and that frame dies, but the WH_CALLWNDPROC hook may already have been entered on the target thread (UnhookWindowsHookEx doesn't wait for an in-flight callback), so it writes through a dangling pointer.

Please use the canonical implementation with a plain SendMessage and no SMTO_BLOCK — see mods/taskbar-icon-separators.wh.cpp#L3821-L3860 and the Development tips wiki page. Teardown of the event handlers has to be a hard requirement, not best-effort.

2. Failed symbol hooking is retried on every LoadLibraryExW call, thrashing the symbol cache.

g_taskbarViewHooked / g_systemTrayHooked are set only when HookSymbols succeeds. If it fails once (e.g. symbols temporarily unavailable), LoadLibraryExW_HookTryHookAvailableModules(true) calls HookSymbols against the same module again for every subsequent DLL load in Explorer. HookSymbols must not be called more than once per module — each extra call invalidates the resolved-symbol cache and forces a re-resolution, which is very slow, and here it happens under the loader lock.

Mark the module as attempted rather than as succeeded, the way taskbar-clock-customization.wh.cpp#L5319-L5330 does it:

if (!g_systemTrayHooked.exchange(true)) {
    if (HookSystemTray(systemTrayModule)) { ... }
}

While you're there, please add an early-out at the top of TryHookAvailableModules when both modules are already handled. Right now every single LoadLibraryExW in Explorer pays for GetSystemTrayModule(), which calls GetModuleVersionInfoFindResource + LoadResource + VerQueryValue on Taskbar.View.dll.

3. RefreshTaskbarClock() runs under the loader lock.

LoadLibraryExW_Hook calls TryHookAvailableModules(true), which calls RefreshTaskbarClock()EnumWindows plus RegOpenKeyEx/RegSetValueEx/RegDeleteValue — while the loader lock is still held by the LoadLibraryExW call we're inside. The comment above it shows you're already aware of the hazard for XAML; the same reasoning applies to window enumeration and registry writes. Other mods only call Wh_ApplyHookOperations() from this hook (see taskbar-clock-customization.wh.cpp#L5333-L5345). Please move the refresh out of the loader-lock path.

4. g_movedClocks is shared between two threads without synchronization — by the mod's own assumption.

UpdateClockPlacement has a comment saying "SystemTray_Main can be owned by a different thread than Shell_TrayWnd", and g_clockThreadId / FindExplorerTaskbarWindow's preferred-thread logic exist precisely to handle that. But if that scenario is real, then TaskbarFrame_SystemTrayExtent_HookGetLayoutReservedClockWidthForTaskbarFrame iterates MovedClocks() on the TaskbarFrame's thread while MoveClock / RestoreClock / CleanupMovedClockData push_back/erase on the clock thread. Iterating a std::vector across a reallocation is undefined behavior, not just a stale read.

(Note that in that scenario the feature wouldn't work anyway: host.XamlRoot() on a Grid created by another XAML thread throws RPC_E_WRONG_THREAD, which the catch (...) swallows, so the extent compensation silently returns 0 and the app icons shift.)

Either resolve it one way — the clock and the taskbar root Grid must be in the same visual tree for MoveClock to succeed at all, so they're arguably always the same thread, in which case g_clockThreadId and the preferred-thread lookup can go away — or add a mutex and copy the needed values out under the lock before touching any XAML.

5. Taskbar alignment changes at runtime aren't handled.

TaskbarUsesCenteredAlignment() is only consulted inside MoveClock. If the user switches Start/apps to left alignment while the mod is enabled and the clock is already relocated, nothing re-evaluates it, so the clock stays pinned at the left edge on top of the Start button and app icons. The reverse (left → centered) also does nothing until something else re-triggers MoveClock. Please verify the behavior, and if the clock isn't re-templated on an alignment change, watch TaskbarAl (e.g. RegNotifyChangeKeyValue, or hook the read the taskbar performs) and restore / re-move accordingly.

6. Add a screenshot to the README.

The mod's effect is purely visual, so a before/after screenshot (or GIF) would help a lot. 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.

  • Wh_ModSettingsChanged never sets *bReload, so the simpler void Wh_ModSettingsChanged() variant is equivalent and clearer. The BOOL(BOOL*) form only earns its place when the mod conditionally asks for a reload.
  • HookTaskbarViewAndSystemTray duplicates the symbol strings from HookTaskbarView and HookSystemTray verbatim. Consider building one std::vector<SYMBOL_HOOK> (or two static arrays concatenated into a local) so the mangled names exist in exactly one place — a stray character in one copy would silently disable a hook.
  • The MoveClockToLeft setting is functionally the same as enabling/disabling the mod: when it's off the mod still installs all its hooks and does nothing. Either drop it, or return FALSE from Wh_ModInit when it's off (Windhawk reloads the mod after each settings change, so that's a valid pattern).
  • [[clang::no_destroy]] on g_deferredClocks isn't needed. DeferredClockData holds only winrt::weak_ref (an in-process control-block decrement) and winrt::event_token (POD), so its destructor is safe at process shutdown; the suppression is noise and invites cargo-culting. The one on g_movedClocks is correct and should stay — it holds a strong Controls::Grid. See Global objects and process shutdown.
  • FindExplorerTaskbarWindow prefers the first top-level window enumerated on g_clockThreadId, whatever its class — that could be a transient/tool window that gets destroyed between the enumeration and the SendMessage. Preferring SystemTray_Main / Shell_TrayWnd when they're on that thread, and only then falling back, would be sturdier.
  • TryHookAvailableModules has two consecutive if (applyImmediately && (taskbarViewHookedNow || systemTrayHookedNow)) blocks with identical conditions; they can be merged.
  • The "Shared taskbar module has an inconsistent hook state" branch looks unreachable: the combined path always sets both flags together, and the split path only runs when the two modules differ. Worth removing if you agree — the AI assist may have added it speculatively.

Functionality notes

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

  • CalculateNativeClockLayoutWidth is a heuristic (hard-coded Segoe UI Variable Text / font size 12, a fallback chrome width of 24, clamped to 64–180) and it's computed exactly once, when the clock is moved. It goes stale if the user changes the locale, the short-date format, or the "show seconds" setting, and it's off from the start if a theme/styling mod changes the clock font — the error shows up directly as a horizontal shift of the centered app icons. Recomputing it on WM_SETTINGCHANGE (or when the clock's text changes) would make it more robust.
  • GetLeftHostOffset(root) is likewise sampled once. If the Widgets/weather button becomes visible, hidden, or resized afterwards, the clock either overlaps it or leaves a stray gap. It also uses a hard-coded + 4 spacing.
  • If ClockLayoutIsReady never becomes true — e.g. another mod has already reparented the clock, so originalParent.Name() != L"SystemTrayFrameGrid" — the LayoutUpdated handler is never removed and runs on every taskbar layout pass, each time walking ancestors with winrt::get_class_name (which allocates an HSTRING per call). Consider giving up after N attempts or a timeout.
  • MoveClock calls root.UpdateLayout() from inside a LayoutUpdated handler. That's re-entrant layout; XAML can throw (layout-cycle detection), and the catch (...) then calls RestoreClock and returns false, so the feature silently does nothing with only a log line. Consider deferring the forced re-measure to the dispatcher instead.
  • Compensating SystemTrayExtent back to the native clock width keeps the centered icons in place (goal 1 in the PR description), but by construction it also keeps that much space reserved on the right, so the area the clock vacated stays blank — which reads as contradicting "avoid leaving an empty clock area on the right side". If I'm misreading the intent, it'd be worth spelling out in the README what the right side is expected to look like.
  • The left host is appended as the last child of the taskbar root Grid with ColumnSpan across all columns, so it renders on top of everything at the left edge. With many app buttons (or on a narrow/secondary taskbar) the centered app group can extend far enough left to end up underneath the clock. Worth checking on 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 Aug 24, 2026
@pleromyst

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 Aug 24, 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 submission overall — the symbol strings, the module-version split at 2604, the RunFromWindowThread helper, the AdditionalClocks refresh trick and the [[clang::no_destroy]] std::optional<> treatment of g_movedClocks all match the established conventions, and there's no existing mod that relocates the Win11 clock, so it isn't a duplicate. A few things to fix:

1. g_movedClocks is read from a different UI thread than the one that mutates it

TaskbarFrame_SystemTrayExtent_HookGetLayoutReservedClockWidthForTaskbarFrame iterates MovedClocks() on the TaskbarFrame's UI thread, while MoveClock (push_back), RestoreClock (erase) and g_movedClocks.reset() all run on g_clockThreadId via RunFromWindowThread. Your own comment says those two can be different threads:

// On current Windows builds, SystemTray_Main can be owned by a different
// thread than Shell_TrayWnd. Remember the actual XAML owner thread from the
// clock callback and use it for teardown and settings changes.
g_clockThreadId = GetCurrentThreadId();

If that's true, mutating a std::vector on one thread while another iterates it is a data race — torn reads and use-after-free, i.e. an Explorer crash. g_callbacksEnabled narrows the teardown window but doesn't close it, and it does nothing at all for the normal MoveClock/RestoreClock path during startup or a settings change.

Either serialize the container access, or publish just the value the extent hook needs (e.g. an atomic double per XamlRoot) so the hook never walks the vector. If you add a mutex, take it inside the taskbar-thread callbacks only — never around the SendMessage in RunFromWindowThread, or you get the classic lock-and-SendMessage deadlock. If the two are in fact always the same thread on every supported build, then the g_clockThreadId machinery and the comment above should go instead — right now the code says both things at once.

2. The relocation runs inside a LayoutUpdated callback and re-enters layout via UpdateLayout()

The LayoutUpdated handler calls MoveClockSafelyMoveClock, which reparents elements and then calls:

originalParent.InvalidateMeasure();
systemTrayFrame.InvalidateMeasure();
root.InvalidateMeasure();
root.UpdateLayout();

LayoutUpdated is raised by the layout manager itself, so this re-enters the layout pass from a layout notification. XAML can fail that with "Layout cycle detected", and the catch (...) right below then calls RestoreClock(clock) — so the feature just silently doesn't apply, with a log line as the only trace.

Defer the move off the layout pass instead, e.g. content.Dispatcher().TryRunAsync(CoreDispatcherPriority::High, ...). taskbar-start-button-position does exactly this, for exactly this reason:

// Runs one of the margin updaters on the taskbar thread, deferred off the
// current layout pass (changing margins during arrange would re-enter layout).
void ScheduleOnTaskbarThread(FrameworkElement element,
                             void (*func)(FrameworkElement)) {
    element.Dispatcher().TryRunAsync(
        winrt::Windows::UI::Core::CoreDispatcherPriority::High,
        [element, func]() { func(element); });
}

3. The LayoutUpdated handler can stay registered forever

ClockLayoutIsReady returns false whenever the clock's parent isn't a Grid named SystemTrayFrameGrid, and the handler is only revoked on the ready path. On any build where that element name changes (or the tray tree is reshaped), the handler is never removed and keeps running FindAncestor + VisualTreeHelper walks on every layout pass of the taskbar tree for the rest of the session, for nothing. Bound it — after N attempts or a deadline, unregister and log that the tray layout wasn't recognized.

4. Give up less easily when delivering the teardown callback

RunOnTaskbarThread uses whatever FindExplorerTaskbarWindow returns, which prefers the first top-level window that happens to be on g_clockThreadId, regardless of class:

if (windows->preferredThreadId &&
    threadId == windows->preferredThreadId &&
    !windows->preferredThreadWindow) {
    windows->preferredThreadWindow = window;
}

If that window is destroyed between EnumWindows and SendMessage, or SetWindowsHookEx fails, RunFromWindowThread returns false and both Wh_ModBeforeUninit and Wh_ModUninit simply give up. The Loaded/LayoutUpdated delegates then stay registered on live XAML elements while Windhawk unloads the mod image, and the next layout pass calls into unmapped memory — an Explorer crash, and the kind of thing that's very hard to trace back to the mod. You already detect the failure (runParameters.executed); use it: fall back to the other candidates (SystemTray_Main, Shell_TrayWnd, or the next window on that thread) instead of bailing after one attempt. Preferring a known window class over "first window on the thread" would also make the normal path more predictable.

5. The single MoveClockToLeft setting duplicates the mod's own enable/disable

The mod has exactly one setting, and turning it off makes the mod do nothing — which is what disabling the mod in Windhawk already does. Dropping it removes g_moveClockToLeft, both transition branches in Wh_ModSettingsChanged, and roughly half of the guard conditions scattered through the callbacks, which is a meaningful chunk of the complexity here. If it's a placeholder for future options, it's better to add it when there's a second option to sit next to.

Optional improvements

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

  • Wh_ModSettingsChanged never sets *bReload, so the simpler void Wh_ModSettingsChanged() form is equivalent and clearer. The BOOL(BOOL*) variant only earns its place when the mod conditionally asks Windhawk to reload it.

  • [[clang::no_destroy]] on g_deferredClocks isn't needed. DeferredClockData holds only a winrt::weak_ref (releases a control-block count from any thread) and winrt::event_tokens (POD), so ~vector() there is a plain heap free — safe on the process-shutdown path. Unneeded suppression is noise and invites cargo-culting, and dropping it also removes all the if (!g_deferredClocks) / DeferredClocks() indirection. Keep it on g_movedClocks — that one holds a strong Controls::Grid and is correct as written, including the reset() on the UI thread. See https://github.com/ramensoftware/windhawk/wiki/Global-objects-and-process-shutdown (specifically #4 and #5).

  • #include <winrt/Windows.UI.Core.h> is unused (nothing references Windows::UI::Core). It would become used if you take the dispatcher suggestion above.

  • LoadLibraryExW_Hook calls TryHookAvailableModules(true) on every successful DLL load, and until both roles are attempted that's three GetModuleHandle calls plus version-resource parsing per load — inside the loader lock. On Windows 10 or with the old Win11 taskbar neither role is ever attempted, so this runs for the entire life of the process with no possible benefit. Filter on the file name first, like taskbar-clock-customization does with HandleLoadedModuleIfSystemTray.

  • Wh_ModAfterInit's UpdateKnownClocksSynchronously() is a no-op in practice — g_deferredClocks is still empty at that point, since nothing has gone through UpdateClockPlacement yet. RefreshTaskbarClock() is what actually picks up an already-running taskbar. It costs an EnumWindows + cross-thread SendMessage to do nothing.

  • HookTaskbarView, HookSystemTray and HookTaskbarViewAndSystemTray spell out the same three SYMBOL_HOOK entries three times. One shared table with the entries selected per role would be easier to keep in sync when a symbol changes.

Functionality notes

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

  • The left host is appended as the last child of the taskbar root Grid with ColumnSpan over all columns, i.e. it's an input-capable overlay on top of the app area rather than something that reserves space. Since SystemTrayExtent is deliberately kept at its native value, the centered app group can still grow all the way to the left edge on a crowded taskbar — at which point the leftmost app buttons end up underneath the clock button and their clicks get swallowed. That's an inherent trade-off of "don't shift the centered apps", but it's worth knowing about (and maybe worth a line in the README).

  • layoutReservedWidth is a synthetic estimate: hardcoded Segoe UI Variable Text at size 12, plus chrome recovered from the tree, clamped to 64–180. It won't track the accessibility text-scale factor, a non-default taskbar font, or the notification badge changing the button's chrome width, and any error shifts the centered app group by that amount. It's also computed once at move time, so later changes — the user toggling "Show seconds in system clock", a locale/date-format change — never update it, because MoveClock returns early via ClockIsAlreadyMoved. Recomputing it when the clock re-templates would keep it honest.

  • GetLeftHostOffset(root) is likewise a one-shot: the host's left margin is fixed at the Widgets button's right edge + 4 at the moment of the move. If the Widgets/weather button is later shown, hidden, or resized (it changes width with the weather text), the clock's offset is stale — it'll either sit on top of the widget or leave a gap.

  • TaskbarUsesCenteredAlignment() is only consulted inside MoveClock. If the user switches Start/taskbar alignment to left while the mod is running, nothing re-evaluates until the clock happens to be re-templated, so the relocated clock sits over the left-aligned Start/app buttons in the meantime. Watching TaskbarAl (or just re-running the placement on WM_SETTINGCHANGE) would close that.

  • The README is clear about what the mod does, but it doesn't mention the two cases where it intentionally does nothing — a left-aligned taskbar, and builds where the tray tree isn't recognized. Users will otherwise read the "no effect" 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 Aug 24, 2026
Restore the clock to its native right-side position when taskbar apps are left-aligned, and move it back to the left when centered alignment is restored. This prevents overlap without shifting application icons.
@pleromyst

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 Aug 24, 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.


Overall the mod is in good shape: the [[clang::no_destroy]] std::optional<...> treatment of the strong XAML refs in g_movedClocks plus the explicit UI-thread release in Wh_ModBeforeUninit is exactly right, the kernelbase LoadLibraryExW resolution is correct, and the chrome-width measurement trick (clock.ActualWidth() - stackPanel.ActualWidth()) is a neat way to stay immune to clock-customization mods. Two things need attention.

1. The deferred alignment-update machinery (WH_CALLWNDPROC hook + PostMessage) is unsound and should be replacedAlignmentUpdateMessage/CancelQueuedAlignmentUpdate*/AlignmentUpdateCallWndProc/ScheduleKnownClockPlacementUpdate, lines 199–324.

  • WH_CALLWNDPROC monitors messages sent to a window procedure (SendMessage); posted messages surface through WH_GETMESSAGE. The trigger here is delivered with PostMessage (line 318), so AlignmentUpdateCallWndProc most likely never sees it. Worth verifying with logging — this is why the canonical mods pair WH_CALLWNDPROC with SendMessage (RunFromWindowThread, line 192) and pair PostMessage with a window proc / subclass (taskbar-vertical.wh.cpp#L4547 posts, #L693 handles it in the hooked wnd proc).

  • If it never fires, nothing ever removes the hook: g_alignmentUpdateHook stays installed on the taskbar UI thread for the rest of the session (the 5 s re-schedule at line 279 only runs on another alignment change), so mod code executes on every message sent to that thread indefinitely.

  • Independently of the above: UnhookWindowsHookEx does not guarantee the hook procedure isn't executing on the target thread when it returns, and the hook is installed with hMod = nullptr (line 300), so it holds no reference on the mod image. That means Wh_ModBeforeUninitCancelQueuedAlignmentUpdateSynchronously() → Windhawk's FreeLibrary can unmap the image while the taskbar thread is inside AlignmentUpdateCallWndProc. This is exactly the hazard your own comment at lines 188–191 describes for RunFromWindowThread.

    Simplest fix — delete all ~120 lines and defer through the XAML dispatcher, the way taskbar-start-button-position.wh.cpp#L423 does. You already extract the FrameworkElement from pThis in GetLayoutReservedClockWidthForTaskbarFrame, so the hook has an element to dispatch from:

    taskbarFrame.Dispatcher().TryRunAsync(
        winrt::Windows::UI::Core::CoreDispatcherPriority::High,
        [generation] {
            if (CallbackAllowed(generation)) {
                QueueKnownClockPlacementUpdate();
            }
        });

    If you want a teardown that is synchronously cancellable, the alternative is WindhawkUtils::SetWindowSubclassFromAnyThread on a taskbar window + PostMessage of a registered message, with RemoveWindowSubclassFromAnyThread in Wh_ModBeforeUninit.

    Note that the alignment change already recovers without this machinery: BadgeIconContent_get_ViewModel_HookUpdateClockPlacement re-evaluates TaskbarUsesCenteredAlignment() and restores/re-moves the clock in both directions. So removing the scheduler shouldn't cost you the feature.

2. LoadLibraryExW_Hook should key off the module that just loaded, and shouldn't permanently latch a fallback guess — lines 1465–1568.

TryHookAvailableModules ignores fileName/module entirely and instead re-resolves GetSystemTrayModule() / GetTaskbarViewModule() from scratch on every successful DLL load, then latches g_systemTrayHookAttempted on the first non-null result — including the ExplorerExtensions.dll fallback at line 1392. Concretely: if ExplorerExtensions.dll is present in the process before SystemTray.dll loads, GetSystemTrayModule() returns it, HookSystemTray resolves the SystemTray::DateTimeIconContent symbols against the wrong module and fails, the role is marked attempted (line 1542), and the mod silently never moves the clock for the rest of the session — even after the real SystemTray.dll arrives.

The canonical pattern gates on identity with the newly loaded module — see taskbar-icon-size.wh.cpp#L2446:

if (!g_systemTrayModuleHooked && GetSystemTrayModuleHandle() == module &&
    module != GetTaskbarViewModuleHandle() &&
    !g_systemTrayModuleHooked.exchange(true)) { ... }

That way a role is only ever attempted against a module that actually just loaded, and a wrong fallback can't consume the one attempt.

Optional improvements

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

  • [[clang::no_destroy]] on g_deferredClocks (line 104) isn't needed. DeferredClockData holds only a winrt::weak_ref (releasing an in-process weak reference is thread-agnostic and safe) and POD event_tokens, so its destructor is a plain heap free — safe on the process-shutdown path. Only g_movedClocks needs the suppression, because of the strong Controls::Grid leftHost. Unneeded suppression is noise that invites cargo-culting; the std::optional wrapper can stay if you want it as a torn-down sentinel. See https://github.com/ramensoftware/windhawk/wiki/Global-objects-and-process-shutdown (the #4-xaml-and-other-ui-thread-objects case is what g_movedClocks correctly implements).
  • BOOL Wh_ModSettingsChanged(BOOL*) (line 1631) never sets the out-parameter — it never requests a reload, so the simpler void Wh_ModSettingsChanged() form is equivalent and clearer.
  • The single MoveClockToLeft setting duplicates enabling/disabling the mod. With it off, the mod still installs every hook and does nothing user-visible; a user who wants the clock back can just disable the mod. Consider dropping the settings block, or replacing it with something that actually varies behaviour (e.g. the left gap currently hard-coded as +4 at line 392, or whether to keep the right-side layout space reserved).
  • Cache the TaskbarAl registry read. TaskbarUsesCenteredAlignment() does a full RegGetValue (open + query + close) on every TaskbarFrame::SystemTrayExtent call (line 748) and on every UpdateClockPlacement (line 1024), i.e. on every BadgeIconContent::get_ViewModel. Caching it and refreshing on WM_SETTINGCHANGE — or hooking the alignment property, which the taskbar already exposes as winrt::impl::produce<TaskbarFrame, ITaskbarFrame>::get_Alignment(int*) (taskbar-start-button-position.wh.cpp#L983) — would make this event-driven instead of polled.
  • FindExplorerTaskbarWindows (line 1212) runs EnumChildWindows recursively for every top-level window in the process, including ones on unrelated threads that can never match preferredThreadId. Scoping the child enumeration to the Shell_TrayWnd / SystemTray_Main windows you already identified would do the same job for a fraction of the work. Also, when g_clockThreadId is still 0 the entire descendant walk is guaranteed to find nothing.
  • Three near-identical symbol arrays (HookTaskbarView, HookSystemTray, HookTaskbarViewAndSystemTray, lines 1403–1457) duplicate the same three symbol strings; a future symbol fix has to be applied in multiple places. taskbar-icon-size.wh.cpp solves this with a single HookTaskbarViewDllSymbols(module, bool includeSystemTraySymbols).
  • #include <winrt/Windows.UI.Core.h> (line 55) appears unused — no CoreDispatcher/CoreWindow in the file. (It would become used if you take the dispatcher suggestion above.)
  • Wh_ModAfterInitUpdateKnownClocksSynchronously() (line 1597) is a no-op cross-thread SendMessage at that point: DeferredClocks() is still empty and g_clockThreadId is 0. RefreshTaskbarClock() on the next line is what actually kicks things off.
  • The LoadLibraryExW hook is installed unconditionally (line 1585) even when both modules were already hooked in Wh_ModInit, so it stays on every DLL load in explorer.exe for the process lifetime. taskbar-icon-size only installs it when delayLoadingNeeded.

Functionality notes

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

  • The relocated clock is a pure overlay with no layout interaction, so it can be overlapped by the centered app cluster. leftHost is appended to the root Grid with HorizontalAlignment::Left and a fixed margin (lines 644–670); nothing stops the centered group from growing leftward past it on a narrow monitor or with many open apps. This is the same problem taskbar-start-button-position.wh.cpp#L327 (UpdatePinnedSystemButtonMargin) solves by measuring the centered group's leading edge and adjusting. Worth at least checking how it looks with ~20 apps pinned/open at 100% scaling.
  • ClockLayoutIsReady returns true when the NotificationCenterButton ancestor isn't found (lines 844–848). That makes the LayoutUpdated retry loop stop waiting and hand off to MoveClockSafely, which then logs "NotificationCenterButton not found" and gives up — with the handler already removed. Since the ancestor being absent is precisely the "tree isn't assembled yet" state the retry loop exists for, !clock should probably mean keep waiting rather than ready. Recovery does happen via the next get_ViewModel/OnApplyTemplate callback, so this is mostly a startup-latency issue.
  • layoutReservedWidth is computed once, at move time (line 624), from a TextBlock probe hard-coded to Segoe UI Variable Text / 12 and clamped to [64, 180]. It won't track later changes to the locale, the 12/24-hour or seconds setting, or a notification badge appearing on the button, and the clamp will over- or under-reserve for very short (14:30) or very long (e.g. Japanese long-date) native strings. Any error shows up directly as the centered icons sitting slightly off-centre — the exact thing the mod is trying to prevent. Re-measuring when the clock's own DesiredSize changes would keep it in sync.
  • An alternative worth evaluating that would remove the SystemTrayExtent hook and the width measurement entirely: leave the clock parented where it is and apply a RenderTransform (TranslateTransform) to move it visually to the left. Render transforms don't participate in layout, so the native tray extent stays untouched by construction and no compensation is needed. The offset would have to be recomputed on taskbar resize, and you'd need to confirm the SystemTrayFrame doesn't clip it. Probably worth a quick experiment given how much machinery the current approach requires.
  • GetLeftHostOffset is evaluated once at move time (line 647), so if the Widgets/weather button is toggled on or off later the clock either overlaps it or leaves a stale gap until the next relocation.
  • RestoreClock doesn't invalidate measure, unlike MoveClock (lines 677–679). Reparenting the clock back into SystemTrayFrameGrid should invalidate the tray's measure implicitly and cause a fresh SystemTrayExtent call, but it's worth confirming the centered icons snap back immediately on disable rather than after the next unrelated relayout.
  • Moving the button out of the tray container changes keyboard/tab order in the taskbar. Probably acceptable, but worth a quick check with Win+T / Tab navigation.


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 Aug 25, 2026
Revert the unreliable deferred-message hook changes and restore the previously tested implementation.
@pleromyst

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 Aug 25, 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-08-25 15:52 UTC (in 12 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 Aug 25, 2026
@pleromyst

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 Aug 26, 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.


Overall this is a careful implementation — the module resolution mirrors taskbar-clock-customization, the RunFromWindowThread helper is a faithful copy of the reference version, the [[clang::no_destroy]] treatment of g_movedClocks (strong Controls::Grid released on the UI thread in CleanupTaskbarStateCallback) is exactly right, and LoadLibraryExW is correctly hooked through kernelbase.dll. The findings below are about the few paths that escaped the guards used everywhere else.

1. QueueKnownClockPlacementUpdate() is called from the extent hook without the clock-thread guard.

GetLayoutReservedClockWidthForTaskbarFrame deliberately bails out when the current thread isn't g_clockThreadId (lines 563-570), and UpdateClockPlacement documents why (lines 861-863: "SystemTray_Main can be owned by a different thread than Shell_TrayWnd"). But the alignment-change branch right after it runs with no such guard:

void WINAPI TaskbarFrame_SystemTrayExtent_Hook(void* pThis, double value) {
    double layoutReservedWidth = GetLayoutReservedClockWidthForTaskbarFrame(pThis);
    TaskbarFrame_SystemTrayExtent_Original(pThis, value + layoutReservedWidth);

    int centeredAlignment = TaskbarUsesCenteredAlignment() ? 1 : 0;
    int previousAlignment = g_lastTaskbarCenteredAlignment.exchange(centeredAlignment);
    if (previousAlignment >= 0 && previousAlignment != centeredAlignment) {
        ...
        QueueKnownClockPlacementUpdate();   // <-- runs on whatever thread TaskbarFrame is on
    }

QueueKnownClockPlacementUpdate iterates DeferredClocks() (a plain std::vector otherwise only touched on the clock thread) and then calls RegisterLayoutUpdatedHandler, which mutates that same vector and calls content.LayoutUpdated(handler) / content.InvalidateMeasure() on a thread-affine XAML object. On any build where TaskbarFrame and the tray XAML are on different threads — precisely the case the rest of the mod guards against — this is a data race on the vector plus a cross-apartment call that throws RPC_E_WRONG_THREAD. Move the alignment sampling into the already-guarded region, or gate this branch on g_clockThreadId == GetCurrentThreadId() the same way.

2. Two entry points can let a C++ exception escape into system code.

Every other entry point wraps its work in try/catch (DateTimeIconContent_OnApplyTemplate_Hook, BadgeIconContent_get_ViewModel_Hook, MoveClockSafely, StopDeferredCallbacksOnTaskbarThread, RestoreAllMovedClocksOnTaskbarThread). These two don't:

  • TaskbarFrame_SystemTrayExtent_HookQueueKnownClockPlacementUpdateRegisterLayoutUpdatedHandlercontent.LayoutUpdated(...) / InvalidateMeasure() — a WinRT exception unwinds straight into Taskbar.View.dll.
  • UpdateKnownClocksCallbackUpdateKnownClocksOnTaskbarThreadUpdateClockPlacement (line 942-961) — this one runs inside the WH_CALLWNDPROC hook proc, so an exception from RestoreClock or RegisterLayoutUpdatedHandler unwinds through user32's SendMessage dispatch. It's reached from Wh_ModAfterInit and from Wh_ModSettingsChanged.

Wrap both in the same try { ... } catch (...) { Wh_Log(...); } pattern used elsewhere.

3. Teardown is best-effort and silently gives up, leaving mod-owned delegates registered after the DLL is unloaded.

RunOnTaskbarThread passes expectedThreadId = g_clockThreadId, and FindExplorerTaskbarWindows only populates its result from the preferred-thread lists when preferredThreadId != 0 — the SystemTray_Main/Shell_TrayWnd fallback at lines 1120-1123 is unreachable in that case. So if EnumWindows doesn't surface a top-level window for the clock's thread (or SetWindowsHookEx/SendMessage fails, or the window is destroyed between enumeration and the send), CleanupTaskbarStateSynchronously logs and returns false — in both Wh_ModBeforeUninit and the Wh_ModUninit retry. Wh_ModUninit then returns and Windhawk FreeLibrarys the mod while the Loaded / LayoutUpdated delegates — objects whose vtables live in the mod image — are still registered on live XAML elements. The next layout pass calls into unmapped memory and takes Explorer down.

Concretely: instead of filtering the EnumWindows result by thread, enumerate the thread directly — EnumThreadWindows(g_clockThreadId, ...) returns the thread's non-child windows regardless of whether they show up in a desktop-wide EnumWindows walk — and fall back to the generic tray windows if that still comes up empty, so cleanup is at least attempted. Failing to revoke should also be logged as an error rather than a routine skip.

Optional improvements

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

  • [[clang::no_destroy]] on g_deferredClocks isn't needed (line 98). DeferredClockData holds only winrt::weak_ref (an in-process control-block release, safe from any thread), std::optional<winrt::event_token> (POD) and integers, so its destructor is a plain heap free — safe at process shutdown. The attribute is correct and necessary on g_movedClocks because MovedClockData::leftHost is a strong Controls::Grid, but carrying it on a type that doesn't need it is noise that invites cargo-culting. See Global objects and process shutdown (the containers case matches g_movedClocks). Dropping it lets g_deferredClocks be a plain std::vector and removes the !g_deferredClocks checks.

  • Use the void Wh_ModSettingsChanged() form (line 1420). The BOOL(BOOL*) variant only earns its place when the mod conditionally asks for a reload; here the parameter is unnamed and never written, so the simpler signature is equivalent and clearer.

  • UpdateKnownClocksSynchronously() in Wh_ModAfterInit is effectively dead (line 1392). g_deferredClocks is only ever populated from UpdateClockPlacement, i.e. from the tray hooks, which realistically haven't fired yet at that point — so the call does an EnumWindows plus a cross-thread SendMessage to iterate an empty list. RefreshTaskbarClock() on the next line is what actually kicks off the initial placement.

  • TaskbarUsesCenteredAlignment() does a RegGetValue on every SystemTrayExtent call (line 613). This is a layout-path setter that fires whenever the tray width changes. Caching the value and refreshing it via RegNotifyChangeKeyValue (or just re-reading it in UpdateClockPlacement) avoids a registry round-trip in the layout path.

  • The MoveClockToLeft setting duplicates the mod's own enable toggle. It's the mod's only setting and it gates the mod's only behaviour, so "off" is indistinguishable from disabling the mod. Unless you plan to add more settings around it, consider dropping it (and the wasEnabled/isEnabled transition handling in Wh_ModSettingsChanged that exists solely to serve it).

  • HookSymbols can be called twice against the same module in the mixed-attempted branch (lines 1299-1323): when taskbarViewModule == systemTrayModule but only one role was previously attempted, HookTaskbarView and HookSystemTray both resolve against that same module. Each extra HookSymbols call for a module invalidates the symbol cache and forces a re-resolution. It's a rare path, but reusing HookTaskbarViewAndSystemTray (or tracking which symbols still need resolving and issuing one call) avoids it.

  • #include <winrt/Windows.UI.Core.h> (line 54) doesn't appear to be used — nothing from Windows::UI::Core is referenced.

Functionality notes

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

  • The left host is an overlay, so it can cover the leftmost app buttons. data.leftHost is appended last to the taskbar root Grid and spans every row/column, so it paints and hit-tests above everything under it. GetLeftHostOffset only accounts for the Widgets/weather button; with enough pinned/running apps the centered group grows leftwards and will end up underneath the relocated clock, where clicks land on the clock instead of the app button. Worth checking with a full taskbar.

  • GetLeftHostOffset is computed once, at move time. If the Widgets button is subsequently shown or hidden (or its width changes with the weather text), the margin isn't recomputed — the clock either sits over the widget or leaves a gap until the mod is reloaded.

  • layoutReservedWidth is a re-measured estimate with hardcoded assumptions. MeasureNativeClockLine pins Segoe UI Variable Text / FontSize 12, chromeWidth defaults to 24, and the result is clamped to [64, 180]. Locales with a long short-date plus a long time format can exceed 180 and get clamped, and changing the time format / seconds setting / text scaling after the move leaves the value stale, both of which shift the centered icons off their native position. Re-deriving it when the clock's own template re-applies would keep it in sync.

  • The extent-preservation feature silently no-ops when TaskbarFrame and the tray are on different threads. GetLayoutReservedClockWidthForTaskbarFrame returns 0 in that case with a single log line, so on such a build the mod still moves the clock but the centered icons shift. If that configuration is reachable in practice, it may be better to skip the relocation entirely than to relocate with the layout unadjusted.

  • Reparenting NotificationCenterButton changes its visual-tree path. Themes and rules for Windows 11 Taskbar Styler commonly target the clock through SystemTray.SystemTrayFrame > Grid#SystemTrayFrameGrid > SystemTray.OmniButton#NotificationCenterButton; once the button lives under the new host grid those selectors no longer match. The README's compatibility note covers clock text mods — mentioning the styler-path caveat too would set expectations.

  • Alignment changes are detected by sampling TaskbarAl inside the extent hook. That works because switching alignment re-lays-out the taskbar, but it means the mod only notices after the next extent update. A RegNotifyChangeKeyValue watch (or hooking the setting read) would be more direct, if you find the current approach misses cases.


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 Aug 26, 2026
@pleromyst

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 Aug 26, 2026
@pleromyst

Copy link
Copy Markdown
Author

I reviewed the remaining AI findings. I restored this revision because a later attempt to address them introduced a confirmed regression involving PostMessage and WH_CALLWNDPROC.

This restored revision passed compilation and runtime testing, including enable/disable, Explorer restart, taskbar alignment changes, and compatibility testing with clock customization mods.

The remaining threading, exception-boundary, and teardown concerns are acknowledged. I would prefer human reviewer guidance before making further speculative lifecycle changes that could introduce regressions.

@m417z

m417z commented Aug 29, 2026

Copy link
Copy Markdown
Member

From the code:

    // On current Windows builds, SystemTray_Main can be owned by a different
    // thread than Shell_TrayWnd.

I don't think it's ever correct. If it's a mistake, update the comment and adjust the code. It will likely also resolve the first review note.

Address this:

  • The MoveClockToLeft setting duplicates the mod's own enable toggle. It's the mod's only setting and it gates the mod's only behaviour, so "off" is indistinguishable from disabling the mod. Unless you plan to add more settings around it, consider dropping it (and the wasEnabled/isEnabled transition handling in Wh_ModSettingsChanged that exists solely to serve it).

Also, I tested the mod on the latest Windows 11 build (26200.9278) and it didn't work. Please make sure it works on that version, which is going to be released for everyone in 10 days.

@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 Aug 29, 2026
@pleromyst

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 Aug 30, 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.


Overall the structure is solid: the module discovery (SystemTray.dll → versioned Taskbar.View.dllExplorerExtensions.dll), the kernelbase.dll LoadLibraryExW hook for late loads, the ((IUnknown**)pThis)[1] projection trick, the RunFromWindowThread helper and the AdditionalClocks refresh trick all match the established patterns in the repo, and the [[clang::no_destroy]] std::optional<...> treatment of g_movedClocks (which holds strong XAML refs) with an explicit reset() on the UI thread is exactly right. The items below are about the pieces that are specific to this mod.

1. The left-host offset is computed once and never refreshed, so the clock drifts out of place

GetLeftHostOffset(root) is evaluated a single time, in MoveClock, and baked into a static margin:

data.leftHost.Margin(Thickness{GetLeftHostOffset(root), 0, 0, 0});

The Widgets/weather entry point is not a fixed-width button — its width changes as the weather text updates (temperature/condition strings differ in length), and it can be shown or hidden at runtime (Settings → Personalization → Taskbar → Widgets). Every one of those changes leaves the clock either overlapping the widgets button or floating with a growing gap after it, until the mod happens to relocate the clock again.

There is also a startup ordering case: ClockLayoutIsReady only requires the taskbar root to have a non-zero size, so the move can happen before the widgets button has been measured. GetLeftHostOffset then hits the widgets.ActualWidth() <= 0 branch, returns 0, and the clock is parked at x=0 directly on top of the widgets button — permanently, since nothing recomputes it.

Keep the position live instead of snapshotting it: hold on to the widgets FrameworkElement, subscribe to its SizeChanged (and to IsVisibleChanged / re-check on LayoutUpdated), and re-apply leftHost.Margin(...) whenever the computed offset changes. Remember to revoke that subscription in StopDeferredCallbacksOnTaskbarThread along with the other handlers.

2. The "taskbar alignment changed" trigger isn't guaranteed to fire

The only place the mod notices that the user switched between centered and left-aligned taskbar is inside the layout hook:

void WINAPI TaskbarFrame_SystemTrayExtent_Hook(void* pThis, double value) {
    ...
    int centeredAlignment = TaskbarUsesCenteredAlignment() ? 1 : 0;
    int previousAlignment = g_lastTaskbarCenteredAlignment.exchange(centeredAlignment);
    if (previousAlignment >= 0 && previousAlignment != centeredAlignment) {
        ...
        QueueKnownClockPlacementUpdate();
    }

TaskbarFrame::SystemTrayExtent(double) is a setter the taskbar calls when the system tray's extent changes. Toggling TaskbarAl doesn't change the tray's width, so there is no guarantee this setter runs after an alignment switch. If it doesn't, the clock stays in the left overlay sitting on top of the now left-aligned Start button and app icons — which is precisely the failure the "Fix left-aligned taskbar overlap" commit was meant to prevent. Please verify this specific flow (toggle alignment back and forth with the mod enabled and nothing else changing on the taskbar).

A deterministic trigger would be better than piggybacking on a layout setter. Two options:

  • Read the taskbar's own alignment instead of the registry — TaskbarFrame exposes it, and the symbol is already used elsewhere in the repo: taskbar-start-button-position.wh.cpp#L983 hooks produce<TaskbarFrame, ITaskbarFrame>::get_Alignment(int*). Hooking that getter gives you a callback that actually runs when the taskbar re-evaluates its alignment.
  • Or watch the key explicitly with RegNotifyChangeKeyValue and marshal the update to the clock thread.

Related: TaskbarUsesCenteredAlignment() does a fresh RegGetValue on every SystemTrayExtent call and on every UpdateClockPlacement (which is driven by the BadgeIconContent::get_ViewModel hook, a property getter). It's not catastrophic, but if you keep the polling shape, cache the value in an atomic and only re-read it when something tells you it may have changed.

3. If the taskbar-thread cleanup can't run, the mod unloads with live delegates in it

Both teardown paths funnel through RunOnTaskbarThread, which needs FindExplorerTaskbarWindows() to return a window. On failure it only logs:

auto taskbarWindows = FindExplorerTaskbarWindows();
if (taskbarWindows.empty()) {
    Wh_Log(L"%s skipped: taskbar window not found", operation);
    return false;
}

Wh_ModUninit retries with the same mechanism, so if the first attempt failed the second one fails too. In that case Wh_ModUninit returns with the Loaded / LayoutUpdated delegates — whose code lives in the mod image — still registered on live XAML elements, and Windhawk's FreeLibrary leaves Explorer to crash on the next layout pass. Note also that once g_clockThreadId is set, FindExplorerTaskbarWindows returns only windows on that thread, so the list can legitimately come back empty if the taskbar window is being recreated at that moment.

Add a fallback that doesn't depend on finding an HWND. The stored elements already give you the UI thread directly — content.Dispatcher() (this looks like why <winrt/Windows.UI.Core.h> is included but otherwise unused) can run the cleanup on the owning thread even when no taskbar window is found:

if (auto dispatcher = content.Dispatcher(); dispatcher && !dispatcher.HasThreadAccess()) {
    dispatcher.RunAsync(winrt::Windows::UI::Core::CoreDispatcherPriority::High,
                        [] { /* revoke handlers, restore clocks */ }).get();
}
Optional improvements

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

  • The g_callbackGeneration machinery is redundant. The counter is only bumped in Wh_ModInit / Wh_ModBeforeUninit / Wh_ModUninit, and every handler is created after Wh_ModInit — so within a single load the captured generation is always equal to g_callbackGeneration, and CallbackAllowed(generation) is exactly g_callbacksEnabled. The same goes for the generation != g_callbackGeneration re-checks in RegisterLoadedHandler / RegisterLayoutUpdatedHandler. Dropping the counter removes a fair amount of code with no behavior change.

  • [[clang::no_destroy]] on g_deferredClocks isn't needed. DeferredClockData only holds a winrt::weak_ref and winrt::event_tokens, both of which are safe to destroy at process shutdown (an in-process refcount decrement and a POD), so the automatic destructor doesn't need suppressing there. It is correct on g_movedClocks, which owns a strong Controls::Grid. See https://github.com/ramensoftware/windhawk/wiki/Global-objects-and-process-shutdown — an unneeded suppression is noise that invites cargo-culting. (Keep the explicit reset() in CleanupTaskbarStateCallback either way; if you drop the attribute you can just clear the vector.)

  • Three near-identical SYMBOL_HOOK arrays. HookTaskbarView, HookSystemTray and HookTaskbarViewAndSystemTray repeat the same three symbol strings, so a fix has to be made in two places. Building one array and choosing how many entries to pass (or a small bool parameter, like taskbar-icon-size.wh.cpp does with hookSystemTraySymbolsInline) would be easier to keep in sync.

  • TryHookAvailableModules returns true when it doesn't do anything. If g_hookSetupInProgress.test_and_set() finds another thread in the middle of setup, the function returns true — i.e. "everything hooked" — which is what Wh_ModInit uses to decide whether to return FALSE. It can't currently bite (Wh_ModInit runs before the LoadLibraryExW hook exists), but returning the current g_taskbarViewHooked && g_systemTrayHooked would be more honest.

  • g_extentThreadMismatchLogged isn't needed. Wh_Log compiles to a cheap if (g_logsOn) and is off by default, so there's no reason to hand-throttle a log line with an extra atomic.

  • FindExplorerTaskbarWindow() is a one-line wrapper used from a single place (RefreshTaskbarClock); inlining it would remove a forward declaration.

  • #include <winrt/Windows.UI.Core.h> is currently unused — either drop it, or use it for the dispatcher fallback in item 3 above.

Functionality notes

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

  • The reserved width is a synthesized estimate. MeasureNativeClockLine hardcodes Segoe UI Variable Text at FontSize(12) and CalculateNativeClockLayoutWidth clamps the result to [64, 180] with a chromeWidth = 24 fallback. The intent (don't let a clock-customization mod poison the layout baseline) is right, but the probe won't track the real clock if the taskbar style changes. Reading FontFamily() / FontSize() / FontWeight() / FontStretch() off the live TimeInnerTextBlock when it exists (falling back to the constants otherwise) would follow the actual style, at the cost of picking up a customization mod's font — worth weighing which failure mode you prefer.

  • layoutReservedWidth is captured once, at move time. If the native clock's width later changes — the user toggles "Show seconds in system clock", changes the time/date format or locale, or the short date simply grows a digit — the reservation goes stale and the centered icons shift by the difference until the clock is relocated again. Recomputing it on the same trigger you use for item 1 would keep it accurate.

  • Overlap with taskbar buttons when many apps are open. The left host is an overlay (root.Children().Append(...)) and deliberately doesn't reserve layout space, which is what keeps the centered icons in place. The trade-off is that with enough open windows the centered repeater grows towards the left edge and the leftmost buttons end up underneath the clock, where they can't be clicked. There's no way to have both, so this is an FYI rather than something to fix — but it's worth confirming how bad it gets with a full taskbar. If it's noticeable, inserting the host at index 0 instead of appending would at least let the app buttons render on top rather than being hidden.

  • No left padding when Widgets is hidden. GetLeftHostOffset returns 0 in that case, so the clock sits flush against the screen edge, unlike every other taskbar element. A small constant inset would probably look better.

  • On a left-aligned taskbar the mod deliberately does nothing. That's a sensible default, but an opt-in setting for users who want the clock on the left there too (accepting the overlap) might be worth considering.


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 Aug 30, 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