Autohide Taskbar on Desktop Only - #5004
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 |
|
/ready-for-reviewer |
|
@qwertyuiop00-art |
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. Main issues: the WinEvent hook won't fire in the common case, the mod permanently overwrites the user's taskbar auto-hide setting, and the functionality overlaps an existing mod. 1. The WinEvent hook is installed on a thread with no message loop, so it usually never fires.
This is exactly the "works after a reboot but not when enabled mid-session" class of bug the maintainer won't merge. Every mod in the repo that uses out-of-context WinEvent hooks creates its own message-loop thread — see keep-rainmeter-always-bottom.wh.cpp#L71 and taskbar-auto-hide-when-maximized.wh.cpp#L1076: HANDLE g_thread;
DWORD g_threadId;
DWORD WINAPI WinEventHookThread(LPVOID) {
HWINEVENTHOOK hook =
SetWinEventHook(EVENT_SYSTEM_FOREGROUND, EVENT_SYSTEM_FOREGROUND,
nullptr, WinEventProc, 0, 0, WINEVENT_OUTOFCONTEXT);
if (!hook) {
Wh_Log(L"SetWinEventHook failed: %u", GetLastError());
return 1;
}
BOOL bRet;
MSG msg;
while ((bRet = GetMessage(&msg, nullptr, 0, 0)) != 0) {
if (bRet == -1) {
break;
}
TranslateMessage(&msg);
DispatchMessage(&msg);
}
UnhookWinEvent(hook); // same thread that installed it
return 0;
}
BOOL Wh_ModInit() {
g_thread = CreateThread(nullptr, 0, WinEventHookThread, nullptr, 0, &g_threadId);
return g_thread != nullptr;
}
void Wh_ModUninit() {
if (g_thread) {
PostThreadMessage(g_threadId, WM_QUIT, 0, 0);
WaitForSingleObject(g_thread, INFINITE);
CloseHandle(g_thread);
g_thread = nullptr;
}
// ...restore the taskbar state here (see below)
}2. The mod destroys the user's taskbar auto-hide setting and doesn't restore it.
Read the state with UINT g_originalState;
void SetTaskbarAutoHide(bool enable) {
APPBARDATA abd = {sizeof(APPBARDATA)};
UINT state = (UINT)SHAppBarMessage(ABM_GETSTATE, &abd);
UINT newState = enable ? (state | ABS_AUTOHIDE) : (state & ~ABS_AUTOHIDE);
if (newState != state) {
abd.lParam = newState;
SHAppBarMessage(ABM_SETSTATE, &abd);
}
}...capture Worth deciding explicitly what should happen when the user already has auto-hide enabled system-wide — with the current design the mod turns their setting off whenever an app is focused, which is probably not what they want. 3. Overlap with taskbar-auto-hide-when-maximized already covers "taskbar hidden when something is in the way, visible on the desktop", with 4. This doesn't need to be injected into The mod has no function hooks at all; it only calls 5. The initial state is never applied. Nothing runs at load time, so the taskbar keeps whatever state it had until the user next switches windows. If the desktop is already focused when the mod is enabled, the taskbar stays visible. Call the same update path once from init (evaluate 6. No screenshot/GIF in the README. This mod has a clearly visible effect. A short GIF showing the taskbar hiding on the desktop and reappearing on app focus would help a lot — 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. |
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 idea is fine and the tool-mod choice is the right one for it, but as submitted the mod never executes any of its own code, and the mechanism it uses rewrites a persisted system setting. Details below. 1. The tool-mod boilerplate is missing — the mod currently does nothing. The file defines Paste the snippet verbatim from Mods as tools: Running mods in a dedicated process at the end of the file. Note that the snippet calls void WhTool_ModSettingsChanged() {}Examples of the boilerplate in place: theme-toggler-tray.wh.cpp#L150 and explorer-folder-hover-menu.wh.cpp#L3837. 2.
There is also a smaller variant of the same problem: The clean approach is the one taskbar-auto-hide-when-maximized.wh.cpp uses: enable auto-hide once, suppress attempts to disable it, and then drive the actual show/hide through explorer's own tray code ( 3. Overlap with taskbar-auto-hide-when-maximized already does dynamic auto-hide driven by the foreground window (it has a 4. Unload can hang forever. PostThreadMessage(g_threadId, WM_QUIT, 0, 0);
WaitForSingleObject(g_thread, INFINITE);A thread has no message queue until it calls a user32 function that creates one. Between Have the worker create its queue and signal readiness before MSG msg;
PeekMessage(&msg, nullptr, WM_USER, WM_USER, PM_NOREMOVE); // force queue creation
SetEvent(g_threadReadyEvent);Related: 5.
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 idea is nice and the code is small and readable, but the mechanism it uses — flipping the persisted, system-wide taskbar auto-hide setting on every focus change — has several consequences that need fixing before this can be merged. 1. The mod makes a persistent system change that can survive its own removal
For reference, 2. Two separate problems:
Fix: capture the original state once, lazily (from the worker thread, after the taskbar window exists), and persist it with 3. abd.lParam = enable ? ABS_AUTOHIDE : ABS_ALWAYSONTOP;Disabling auto-hide also force-enables always-on-top, even for a user who had it off. Read the current state and flip only the one bit, as in abd.lParam = enable ? (state | ABS_AUTOHIDE) : (state & ~ABS_AUTOHIDE);4.
static int g_lastApplied = -1; // or std::optional<bool>
if (g_lastApplied != (int)isDesktop) {
g_lastApplied = isDesktop;
SetTaskbarAutoHide(isDesktop);
}5. This should be a tool mod, not an The mod installs no function hooks at all — it only uses Please convert it to a tool mod: (Note this doesn't fix item 1 by itself — the tool process is also terminated at sign-out/shutdown without 6. Unload can unload the DLL while the worker thread is still running PostThreadMessage(g_threadId, WM_QUIT, 0, 0);
WaitForSingleObject(g_thread, 2000);If the wait times out, Check the 7. Relationship to existing mods
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. |
Mod authorship
If this pull request introduces a new mod, please complete the section below.
This mod was created by: