Skip to content

Optimize taskbar thread detection - #5216

Open
Anixx wants to merge 1 commit into
ramensoftware:mainfrom
Anixx:patch-466188
Open

Optimize taskbar thread detection#5216
Anixx wants to merge 1 commit into
ramensoftware:mainfrom
Anixx:patch-466188

Conversation

@Anixx

@Anixx Anixx commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Changelog

If this pull request updates an existing mod, describe the changes below:

  • Changelog item 1...
  • Changelog item 2...

Mod authorship

If this pull request introduces a new mod, please complete the section below.

This mod was created by:

    • The submitter, without AI assistance
    • The submitter, with AI assistance
    • Claude
    • ChatGPT
    • Gemini
    • Another AI (please specify):
    • Other (please specify):

Please select the options that best apply. Your selection does not affect the acceptance criteria, but it helps reviewers understand the context of the code and provide relevant feedback.

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

@Anixx

Anixx commented Aug 26, 2026

Copy link
Copy Markdown
Contributor 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.


The change swaps a self-contained window-class check for an OS-version-dependent, undocumented thread name. That trade needs a fallback, and the "optimize" framing doesn't hold up.

1. The taskbar exclusion now fails silently in two ways, both of which bring back the bug fixed in 1.3.

  • GetProcAddress(..., "GetThreadDescription") returns NULL on anything older than Windows 10 1607, so pGetThreadDescription is null, found stays false, and the taskbar gets un-greyed again. This matters more than the README note suggests: Classic theme is native on Windows 7, which is arguably the mod's most natural audience, and the mod still declares @include explorer.exe with no OS gate — it just quietly loses a fix there.
  • L"Taskbar" is an internal, undocumented thread name. Nothing guarantees Explorer sets it, on which builds, or that it keeps that value. No mod in the repo relies on it — the existing thread-description users target Win11 XAML shell threads (taskbar-thumbnails.wh.cpp#L147, shell-flyout-positions.wh.cpp#L829: MultitaskingView, JumpViewUI, ActionCenter, QuickActions). Which Windows builds did you verify Taskbar on?

Also, the negative result is cached permanently: checked = true is set before the call, so if the description hasn't been assigned yet the first time the hook runs on that thread, the thread is written off as "not the taskbar" forever. GetThreadDescription returns S_OK with an empty string when no description was set, which lets you tell "not named yet" apart from "named, but not the taskbar".

The window-class check you removed has none of these problems — it works on every Windows version and keys off Shell_TrayWnd/Shell_SecondaryTrayWnd, which have been stable for decades. Keep it as the fallback rather than replacing it:

bool IsCurrentThreadTaskbar()
{
    if (checked)
        return found;

    if (pGetThreadDescription)
    {
        PWSTR desc = nullptr;
        if (SUCCEEDED(pGetThreadDescription(GetCurrentThread(), &desc)) && desc)
        {
            bool named = *desc != L'\0';
            if (named)
            {
                found = wcscmp(desc, L"Taskbar") == 0;
                checked = true;   // thread is named -> result is final
            }
            LocalFree(desc);
            if (found)
                return true;
            if (named)
                return false;
        }
    }

    // No thread description (pre-1607, or not named): fall back to checking
    // whether this thread owns a taskbar window.
    EnumThreadWindows(GetCurrentThreadId(), EnumThreadWndProc_CheckTaskbar,
                      (LPARAM)&found);
    checked = true;
    return found;
}

(Note that the primary and secondary taskbars share one Explorer thread, so a single check covers both — keep both class names in the fallback anyway, since which one exists depends on the monitor setup.)

2. The optimization doesn't optimize anything measurable.

EnumThreadWindows was already guarded by the checked thread-local, so it ran at most once per thread — exactly like GetThreadDescription does now. Both are one-time-per-thread costs on a path that's only reached after IsGreyColor and WindowFromDC have already passed. So the OS-version dependency and the undocumented-name dependency are being taken on for no runtime gain.

There is a real improvement hiding in here, and it's worth stating as the actual motivation instead: a thread's description is set at the top of the thread proc, whereas the taskbar window may not exist yet the first time the enumeration runs — so GetThreadDescription is less prone to caching a stale false. That argues for using it first, with the window check as the fallback (as above), not for dropping the window check. Please update the PR title/description to match what the change actually does.

3. The PR description is still the unfilled template.

This is a mod update, so the Changelog section applies — please replace the Changelog item 1... placeholders with a real summary of what changed in 1.4.

Optional improvements

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

  • Use the typed hook helper. Wh_SetFunctionHook with void* casts is the old form; WindhawkUtils::SetFunctionHook is type-safe. And since the mod already links -lgdi32, you can reference SetTextColor directly instead of resolving it through GetProcAddress — it's the same address, and gdi32 is a KnownDLL so GetModuleHandle can't fail here anyway:

    decltype(&SetTextColor) SetTextColor_Original;
    ...
    WindhawkUtils::SetFunctionHook(SetTextColor, SetTextColor_Hook,
                                   &SetTextColor_Original);

    That also removes the SetTextColor_t typedef. Related: the current code ignores Wh_SetFunctionHook's return value, so a failed resolve would leave the mod loaded and doing nothing.

  • decltype(&GetThreadDescription) instead of the hand-written typedef. The declaration is present in the toolchain headers (other mods call it directly), so using GetThreadDescription_t = decltype(&GetThreadDescription); avoids a signature that can drift. Keep resolving it at runtime via GetProcAddress though — a static import would make the mod fail to load entirely on pre-1607.

  • Comment language. The file mixes Russian and English comments (the new IsCurrentThreadTaskbar comment is Russian, the SetTextColor_Hook one is English). English throughout would make it easier for others to maintain.

  • Globals naming. checked / found are file-scope thread_locals with very generic names; the repo convention is a g_ prefix (g_taskbarChecked / g_isTaskbarThread), or make them function-local thread_local statics inside IsCurrentThreadTaskbar.

  • README grammar. "The current version of the mod intended for Windows 10 version 1607 or higher." is missing a verb — "This version of the mod requires Windows 10 version 1607 or newer." If you add the fallback from item 1, the sentence can be dropped entirely.

Functionality notes

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

  • The hook is much broader than "menubar". SetTextColor_Hook rewrites any colour where r == g == b (and != COLOR_MENUTEXT) set on a memory DC by any Explorer thread that doesn't own the foreground window. That catches genuinely disabled UI text in inactive Explorer windows too — disabled toolbar buttons, disabled menu items, greyed placeholder text — not just the menubar, and those will render as if enabled. Not introduced by this PR, just noting it.

  • Per-thread blacklisting is a symptom of the above. The taskbar exclusion added in 1.3 (and now this rework) is patching a specific false positive; the next one will need another special case. Anchoring on the actual draw instead — hooking the menu band's paint path in browseui.dll/shell32.dll, or only enabling the override for the duration of the menubar's paint — would let you drop the colour heuristic, the WindowFromDC check, the foreground check and the taskbar check altogether. There's no drop-in alternative, so this is an FYI rather than something to change now.


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

1 participant