Add Dynamic Taskbar Theme Switcher mod - #5274
Conversation
|
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 See the pull request review process for the full details. |
|
/ai-review |
Submission reviewNote: 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 biggest question here is structural: this is a hard fork of an actively maintained mod, and that shapes most of what follows. 1. This is a fork of Windows 11 Taskbar Styler, not a new mod. Of the 21,243 lines, roughly 20,400 are a verbatim copy of windows-11-taskbar-styler.wh.cpp — the whole XAML diagnostics/styling engine plus all ~60 bundled community themes. Only about 1,050 lines are new (the per-monitor trigger thread, the
The genuinely new idea — "switch between two of the bundled themes per monitor depending on window state" — is a feature request for Taskbar Styler, not a reason to duplicate it. Please open an issue (or a PR against m417z/my-windhawk-mods) proposing a Everything below applies to the code as submitted, in case the fork route continues in some form. 2. Leftover upstream telemetry. 3. The mod repositions and re-tops every taskbar unconditionally, even when its own feature is off.
int distanceInPixels = 0;
bool raiseFromBottom = !context->forceBottom && ShouldRaiseTaskbarFromBottom(hWnd);
if (raiseFromBottom) { ... }
int targetTop = monitorInfo.rcMonitor.bottom - taskbarHeight - distanceInPixels;
if (std::abs(taskbarRect.top - targetTop) > 1) { setWindowPos(...); }So with
Both should be no-ops unless the mod actually raised that window: track the taskbars you moved (and their original z-order state) and only touch those, both while running and when restoring. 4. 250 ms polling doing very expensive work, plus the same work inside global
And This should be event-driven. 5. Data race on return !g_forceTaskbarBottom.load() && !IsMonitorUsingWindowTheme(monitor) &&
g_desktopThemeName == L"Minecraft_Hotbar" &&
g_settings.minecraft.raiseFromBottom > 0;It's called from Precompute the decision in std::atomic<bool> g_raiseMinecraftTaskbar; // desktopTheme == Minecraft && raiseFromBottom > 0
std::atomic<int> g_raiseFromBottomDip;and read only those from the hook paths. 6. The window theme's ProcessThemeStyles(g_desktopThemeName.c_str(), RuleThemeScope::desktop, false, true);
ProcessThemeStyles(g_windowThemeName.c_str(), RuleThemeScope::window, true, false);
7. } else if (themeScope == RuleThemeScope::window && wcscmp(themeName, L"Native") == 0) {
AddTaskIconSizeRules(g_settings.nativeIconSize, themeScope);
AddIndicatorCustomizationRules(g_settings.nativeIndicator, themeScope);
}
8. The restart-Explorer dialog still identifies as the upstream mod. 9. No screenshot in the README. This is about as visual as a mod gets — ~60 themes, a custom Minecraft hotbar layout, a raised floating taskbar. A GIF showing the switch happening as a window is maximized would communicate the mod far better than the prose does. Images must be hosted on Optional improvements
Minor polish — none of this affects users, so it's your call.
Functionality notes
Non-critical observations and ideas about the feature behavior itself.
Next steps:
See the review process for details. |
|
/ai-review |
Submission reviewNote: 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 dynamic per-monitor switching idea is genuinely useful and the new code is carefully written and well commented. The blocker is structural: the submission is a full copy of another mod's engine, and a few of the new mechanisms fight Explorer harder than they need to. 1. The mod is a fork of Windows 11 Taskbar Styler, not a new mod Of the 21,254 lines, ~20,400 are a verbatim copy of windows-11-taskbar-styler — the XAML diagnostics tap, the styling/matching engine, the image loader, the restart-Explorer prompt, and the entire bundled theme catalog. The genuinely new part is roughly 850 lines. This is the pattern the maintainer most consistently pushes back on: a duplicated catalog entry means every future Taskbar Styler fix, every new Windows build compatibility change, and every theme update has to be re-merged into this copy by hand, and it will diverge within weeks. It also puts users in a bad spot — your own README has to open with "disable Windows 11 Taskbar Styler first", which is the symptom, not a workaround. The functionality itself doesn't exist yet, so it's worth having. The right shape is one of:
Please don't spend effort on the items below until this is settled — most of them apply either way, but the packaging decision changes what code even stays. 2. Leftover download-stats timer pointing at the upstream project
static constexpr WCHAR kStatsBaseUrl[] =
L"https://github.com/ramensoftware/"
L"windows-11-taskbar-styling-guide/"
L"releases/download/stats-v6/";It's called from 3. Data race on the theme-name globals and
g_desktopThemeName = *desktopTheme ? desktopTheme : L"Minecraft_Hotbar";and read concurrently from Explorer threads while that write is in flight:
The cheap fix is to not read strings on the hot path at all: derive a 4. Taskbar z-order and position are enforced unconditionally, not only when the feature is on
if (!ShouldRaiseTaskbarFromBottom(hWnd)) {
if (!isTopmost) {
setWindowPos(hWnd, HWND_TOPMOST, 0, 0, 0, 0, flags);
}
return;
}So whenever the taskbar is not being raised — which includes every monitor showing the window theme, and every user who picked a non-Minecraft desktop theme or set
Both should be no-ops unless the raise-from-bottom feature is actually active — and on teardown, restore only what the mod actually changed (track which windows you moved / un-topmosted, and touch only those). 5. 250 ms polling loop with an expensive per-window probe
The events you need are all available: taskbar-background-helper solves the exact same "is there a maximized/fullscreen window on this monitor" question event-driven, with 6. Full desktop window walk inside the
If you keep the "taskbar below app windows" behaviour, cache the result: the same per-monitor window set the WinEvent hook maintains (item 5) already tells you the bottom-most relevant window, so the hook can read a precomputed 7. The restart-Explorer prompt still identifies itself as the other mod constexpr WCHAR kRestartExplorerPromptTitle[] =
L"Windows 11 Taskbar Styler - Windhawk";Users of this mod get a modal dialog titled with a different mod's name, offering to kill 8. README: add a visual, and drop the "self-contained" claim This is a purely visual mod and the README has no screenshot or GIF. A short GIF of the taskbar switching between the two themes as a window is maximized would communicate the whole mod in two seconds; a pair of before/after screenshots also works. Only Separately, "The mod is self-contained" isn't accurate — many of the bundled themes (Windows 7, Windows XP, Sun Valley, One UI 8.5, Plasma, …) reference PNG assets by Optional improvements
Minor polish — none of this affects users, so it's your call.
Functionality notes
Non-critical observations and ideas about the feature behavior itself.
Next steps:
See the review process for details. |
|
/ai-review |
Submission reviewNote: 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 switching idea is a reasonable one and the new code is thoughtfully commented, but the submission has one structural problem that dominates everything else, plus a set of correctness and cost issues in the new logic. 1. This is a hard fork of Windows 11 Taskbar Styler rather than a new mod. Diffing the submission against mods/windows-11-taskbar-styler.wh.cpp gives ~1,780 added and ~790 removed lines out of 21,403 — roughly 95% of the file is a verbatim copy of the styling engine and all 56 bundled themes. The README even instructs users to disable Taskbar Styler first, so it is a replacement, not a complement. That matters practically, not just on principle: the taskbar's XAML tree changes with almost every Windows feature update, and the styler's themes and selectors are actively maintained against those changes. A forked copy stops receiving them, so it will silently drift and break, and users who report a broken theme won't know which of the two mods to file against. The project's consistent preference is to extend an existing mod rather than merge a near-duplicate. Concretely: the dynamic per-monitor switching would be much better as an option inside Windows 11 Taskbar Styler itself (development happens at https://github.com/m417z/my-windhawk-mods), where it would work with every theme, stay in sync with the engine, and not duplicate 20k lines. Note also that the detection half already exists in mods/taskbar-background-helper.wh.cpp (per-monitor maximized/fullscreen tracking driving a per-monitor taskbar change), so what's genuinely new here is a fairly small amount of glue. 2. The window theme silently loses its theme resource variables. ProcessThemeStyles(g_desktopThemeName.c_str(), RuleThemeScope::desktop, false, true);
ProcessThemeStyles(g_windowThemeName.c_str(), RuleThemeScope::window, true, false);37 of the 56 bundled themes define There's no clean fix while keeping the current design — resource variables are merged into 3. Every accepted WinEvent triggers a full system-wide window rescan.
The hook set itself is fine — 4. The scan thread and its system-wide hooks start in every if (initialize) {
InitializeSettingsAndTap();
}
StartDynamicThemeThread();
}With "Launch folder windows in a separate process" enabled, or transiently during an Explorer restart, several 5. Test the cheap atomics first in all of these, and preferably don't install the hooks at all when the raise feature is off (the settings are known in 6. The taskbar z-order/position takeover isn't reliably reversible. While raised, the mod strips
7. "Native Windows taskbar" isn't native. Selecting 8. No screenshot or GIF in the README. This is an entirely visual mod whose whole point is what the taskbar looks like in each state, and the README currently has no images at all (the upstream styler README shows every theme). Please add at least a screenshot of the two states, ideally a short GIF of the switch. Only Optional improvements
Minor polish — none of this affects users, so it's your call.
Functionality notes
Non-critical observations and ideas about the feature behavior itself.
Next steps:
See the review process for details. |
|
/ai-review |
|
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 |
Summary
Adds Dynamic Taskbar Theme Switcher, a self-contained Windows 11 taskbar mod
that automatically switches between two bundled Taskbar Styler themes per
monitor when a normal window is open or a window is maximized/fullscreen.
The mod is derived primarily from Windows 11 Taskbar Styler and also adapts
concepts from Dynamic Taskbar Transparency and Taskbar Background Helper. The
source metadata and README identify the upstream projects, authors, theme
contributors, Minecraft Hotbar author, license, usage, differences, conflicts,
and limitations.
Local validation completed successfully with the repository's PR validator.
The mod also compiles successfully for x86-64 and ARM64 with Windhawk 1.7.3.
Changelog
If this pull request updates an existing mod, describe the changes below:
controls.
taskbar position and z-order only when the mod changed them, protects shared
settings used by hooks, and fixes Native customization behavior and labels.
and a 60-second safety scan, while caching the per-monitor dynamic-window
snapshot.
the Explorer process that actually owns a taskbar.
SetWindowPoscalls inhook paths.
offline limitations of image-based themes.
Mod authorship
If this pull request introduces a new mod, please complete the section below.
This mod was created by:
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.