Optimize taskbar thread detection - #5216
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 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.
Also, the negative result is cached permanently: The window-class check you removed has none of these problems — it works on every Windows version and keys off 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.
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 3. The PR description is still the unfilled template. This is a mod update, so the Changelog section applies — please replace the 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. |
Changelog
If this pull request updates an existing mod, describe the changes below:
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.