Bump to 1.7.0: cover common display languages in chevron detection - #5260
Bump to 1.7.0: cover common display languages in chevron detection #5260wygodad wants to merge 8 commits into
Conversation
…pdate keywords) Fixes ramensoftware#5257 Users on locales outside the default keyword list fell back to the positional guess, which can select the wrong tray button (reported for Dutch in ramensoftware#5257: the fallback alternated between the chevron and the Quick Settings button). - Expand the default chevron name keywords to cover the most common Windows display languages - Use CharLowerBuffW instead of towlower for case-insensitive matching, so non-ASCII scripts (Cyrillic, Greek) fold correctly - Update the readme guidance for locales still missing from the list
|
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 diagnosis in issue 5257 is right and the fix direction is reasonable, but the keyword list is applied over a much wider set of elements than just the tray, and widening it from 4 to 24 substrings makes that a real problem. The findings below are mostly about the detection logic the PR touches. 1. Name matching is applied to every button on the taskbar, not just tray buttons
The name loop (lines 320-327) accepts the first element in tree order whose name contains any keyword, with no AutomationId or ClassName check — and task list buttons come before the tray in that order. Taskbar buttons are named after the window/app they represent, so:
The consequence isn't just a missed chevron: the matched element becomes the hover target, so hovering that taskbar button makes the mod Restrict the candidate set before matching by name — at minimum require the same AutomationId the fallback already uses: BSTR aid = nullptr;
e->get_CurrentAutomationId(&aid);
bool isTrayIcon = (aid && s.trayIconAutomationId == aid);
if (aid) SysFreeString(aid);
if (!isTrayIcon) { e->Release(); continue; } // then name-match / leftmostBetter still, scope the search root itself: 2. Match the chevron by its language-independent class name instead of by localized text The readme says the chevron has no language-independent identifier, but it does have one that several merged mods already rely on: the XAML type name Worth verifying with Accessibility Insights on your build, but if it holds it makes the whole language problem go away: match on the class name first, fall back to the keyword list, and only then to position. That is a real fix for 5257 rather than a list that will always be missing someone's locale (and whose entries are build-specific — 3. The positional fallback — the part that actually misfired in 5257 — is unchanged Per the issue thread, the reported symptom was the fallback landing on Quick Settings. Any locale still missing from the list (and any future build where the wording changes) hits the same path, so it's worth hardening while you're here:
4.
DWORD taskbarPid = 0;
GetWindowThreadProcessId(FindWindowW(L"Shell_TrayWnd", nullptr), &taskbarPid);
...
DWORD pid = 0;
GetWindowThreadProcessId(h, &pid);
if (pid != taskbarPid) continue;(The setting is off by default, so this is not the common path, but hiding another app's popup is not recoverable from the user's side.) 5. Mixed-DPI: UIA rectangles and
SetThreadDpiAwarenessContext(DPI_AWARENESS_CONTEXT_PER_MONITOR_AWARE_V2);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. |
Detection is now restricted to tray elements and no longer guesses. - Restrict candidates to tray elements (class name prefixed SystemTray., or the tray AutomationId) before any name or position test, so a task list button named after its window title can no longer be matched and invoked - Identify the chevron by class name plus AutomationId, which is language-independent, and accept it only when unambiguous - Keep name matching as a fallback, drop the generic "overflow" and "rozwin" keywords - Turn the positional guess into an opt-in setting, off by default: when the chevron cannot be identified the mod now does nothing and logs every tray candidate instead of invoking an unidentified button - Reject offscreen elements and degenerate rectangles - Hide only tooltips owned by the taskbar process - Set per-monitor-v2 DPI awareness on the worker thread - Read the settings generation before the snapshot, pre-lowercase the keywords, drop the unused <algorithm> include, correct the unload comment, restore the tool-mod boilerplate to the wiki version
|
Thanks, this was a useful review. Everything is addressed, with one finding corrected by measurement. Finding 2 does not hold for UIA on this build. I enumerated the entire UIA tree under
So the goal of your finding is met, just through the pair rather than a single class: the chevron is the only tray element carrying
4, 5. Tooltip hiding is filtered by the taskbar's process id, and the worker thread sets per-monitor-v2 DPI awareness. All optional items done: unused include removed, keywords pre-lowercased at load, settings generation read before the snapshot, unload comment corrected, boilerplate restored to the wiki version. The readme now tells existing users to reset the keyword setting to pick up new defaults. Verified on 25H2 26200.9278 (Polish): the mod log shows only open/collapse lines, confirming identification goes through the class name and never reaches the name fallback. |
|
/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 previous round's findings are all addressed, and the measurement-backed pushback on the 1. The periodic rectangle refresh doesn't apply the guard that discovery does
if (SUCCEEDED(pBtn->get_CurrentBoundingRectangle(&r))) {
cachedRect = r;
haveRect = true;
}
Apply the same guard as in discovery, and treat a bad rect like a stale element: RECT r;
BOOL offscreen = FALSE;
if (SUCCEEDED(pBtn->get_CurrentBoundingRectangle(&r)) &&
r.right > r.left && r.bottom > r.top &&
SUCCEEDED(pBtn->get_CurrentIsOffscreen(&offscreen)) && !offscreen) {
cachedRect = r;
haveRect = true;
} else {
pBtn->Release(); pBtn = nullptr;
haveRect = false;
overBtnPrev = false; // also stale once the button is gone
nextRefind = 0;
WaitForSingleObject(g_stopEvent, s.pollInterval);
continue;
}2.
} else if (now - leftAt >= (ULONGLONG)s.grace) {
DoCollapse(pBtn);
leftAt = 0;
flyoutBelievedOpen = false;Since the chevron exposes no ExpandCollapse pattern, Treat a collapse as an action for cooldown purposes — rename DoCollapse(pBtn);
lastActionAt = now;
leftAt = 0;
flyoutBelievedOpen = false;This also gives the 3. The After this PR the value is used in three places, only one of which is the fallback: the tray-element filter ( 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. |
- Apply the discovery guard to the periodic rectangle refresh: an element that is alive but not rendered returns S_OK with an empty rectangle, which turned the top-left corner of the screen into a hover hotspot and made the flyout cycle open and closed under a stationary cursor - Arm the action cooldown on collapse as well, so a collapse cannot be followed by an immediate re-open at small collapse delays - Rename the chevron AutomationId setting and describe it as part of the primary identification rather than a fallback - Fetch candidate properties through a cache request instead of one cross-process call per property - Log the candidate table once per failure streak - Filter the flyout lookup by the taskbar process and resolve that process id once instead of per tick - Add an opt-in hover delay before the flyout opens - Scope the fullscreen check to the monitor the chevron is on - Accept ChevronButton as a second chevron AutomationId - Document that the mod is Windows 11 only and how to restore the keyword defaults
|
All three required findings are fixed, and most of the optional items as well.
Optional items done: cache request via On the tooltip process filter from the previous round: I verified the chevron tooltip is owned by explorer.exe, the same process as From the functionality notes: added an opt-in "Hover delay" setting, which also covers a standing request in #4524; scoped the fullscreen check to the chevron's monitor; accepted Deliberately skipped: RTL handling in the positional guess, which is opt-in and logged, and explicit Verified on 25H2 build 26200.9278 at 140% scaling. |
|
/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 previous round's three items are all correctly fixed — the rect refresh now applies the discovery guard and drops the element on an empty rect, 1. Clicking the chevron to close the flyout makes the mod re-open it immediately
On that last step The cleanest fix is to treat "the flyout was open during this stay" as having served the stay, so a re-open requires leaving and re-entering the hit area: if (overBtn && flyoutVisible) {
dwellFired = true; // this stay has already seen an open flyout
}
if (enterEdge && !flyoutVisible) {
dwellFired = true;
...
}(Placed after 2. The fixed 3 s re-find timer is both too slow and too eager
One change fixes both: drive re-finding from the cursor instead of from a timer. The cursor is already read every tick, so gate it on the cursor being over the taskbar (or, cheaper, on the last known chevron rect / the taskbar's monitor edge) — retry immediately while it is, and don't walk at all while it isn't: // Only worth re-finding when the cursor is somewhere the chevron could be.
POINT pt; GetCursorPos(&pt);
HWND hTaskbar = FindWindowW(L"Shell_TrayWnd", nullptr);
RECT tb;
bool nearTaskbar = hTaskbar && GetWindowRect(hTaskbar, &tb) &&
PtInRectPad(tb, pt, TASKBAR_REVEAL_PAD);
if (!pBtn && nearTaskbar && now >= nextRefind) { ... }With 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. |
- Treat a hover stay that has seen an open flyout as served, so clicking the chevron to dismiss the flyout no longer re-opens it on the next tick. Moving up into the icons and back down onto the chevron used to leave the enter edge armed, because the flag that spends it was only set in the branch that opens the flyout - Drive re-finding from the cursor instead of a fixed timer. The rectangle guard drops the element on every auto-hide retract, so a fixed interval left the mod idle for up to three seconds after each reveal; conversely, with no hidden icons the chevron never exists and the timer walked the taskbar subtree for the life of the session. Retry every 250 ms while the cursor is at the taskbar, back off while it is elsewhere - Clear the collapse timer when the element is dropped, so the flyout is not collapsed on the first tick after the chevron comes back - Reset the hover-dwell state when settings change, so a cursor resting on the chevron does not skip the configured hover delay - Log the ambiguous-signature line once per failure streak, and skip the candidate dump when nothing was rendered - Drop the dead UI Automation header guard, note why the alternate chevron AutomationId is deliberately not a setting
|
Both findings confirmed in the code and fixed.
All optional items done as well: the collapse timer is cleared when the element is dropped, the hover-dwell state is reset on a settings change, the ambiguity line is gated by the same streak flag as the candidate dump, the dump is skipped when nothing was rendered (a retracted taskbar produces no candidates and needs no diagnosis), the dead header guard is gone, and the alternate AutomationId now carries a comment explaining why it is fixed rather than configurable. On the functionality notes: The reporter in #4684 has since confirmed the root cause was a missing keyword for Czech and that the 1.7.0 approach addresses it, so the PR now closes that issue too. Verified on 25H2 build 26200.9278 at 140% scaling, including the click-to-dismiss gesture from finding 1. |
|
/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 |
|
/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 previous round's two items are correctly fixed — a stay that has seen an open flyout is now served, so the click-to-dismiss gesture no longer re-opens, and re-finding is cursor-driven. The tool-mod boilerplate still diffs byte-for-byte clean against the wiki snippet, and the settings block and the code are in sync in both directions. All three findings below are in the new code: the context-menu guard, the broadened tooltip suppression, and the cost of the new re-find cadence. 1. static bool IsPopupMenuOpen() {
HWND h = FindWindowW(L"#32768", nullptr);
return h && IsWindowVisible(h);
}
static bool IsPopupMenuOpen() {
HWND h = nullptr;
while ((h = FindWindowExW(nullptr, h, L"#32768", nullptr))) {
if (IsWindowVisible(h)) return true;
}
return false;
}Note also that this test is desktop-wide and unfiltered — a menu open in any unrelated application suspends auto-collapse for as long as it is up. That is the safe direction and probably fine, but it is worth knowing; the tray-icon menus you actually care about belong to the icon's owner process, so filtering by PID is not an option, but you could require the menu to intersect the flyout or the taskbar's monitor. 2. Tooltip suppression now runs on plain hover, and
Since the class alone cannot discriminate, add a property that can. A tooltip is a single short line; a menu is not, so a height cap is the cheapest filter: LONG band = chevron.bottom - chevron.top;
if (r.bottom - r.top > 2 * band) continue; // menus/flyouts, not a tooltipAn interaction-based test works too (a tooltip never becomes the foreground window and never takes capture, a menu does), or simply skip the whole pass while a mouse button is down. Whichever you pick, please verify what the Win11 taskbar context menu reports for its class on your build before deciding this cannot happen. 3. The cursor-driven re-find walks the taskbar's UIA subtree 4×/second, indefinitely, for every user who has no hidden icons if (nearTaskbar) {
pBtn = FindOverflowButton(pAuto, s, !loggedCandidates, &didLog);
nextRefind = now + REFIND_NEAR_MS; // 250 ms
The previous round's finding was that a fixed 3 s timer left auto-hide users idle after a reveal; the fix over-corrected into an unbounded fast retry. Bounding it keeps both properties: count consecutive failed lookups and back off (250 ms → 500 → 1000 → …, capped at a few seconds), resetting the counter when the chevron is found and on the transition of the cursor into the band, so an auto-hide reveal still re-acquires on the first or second try while an absent chevron costs almost nothing. Complementary and cheap: on the fast path, query with 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. |
- Enumerate every #32768 window instead of testing only the first one: menu windows are kept alive hidden after use, so the first in Z-order is often a leftover and the guard could report no menu while one was on screen, which is exactly the case it was added for - Back off after consecutive failed chevron lookups, resetting on success and when the cursor enters the taskbar band: the previous round's cursor-driven re-find retried every 250 ms indefinitely for anyone with no hidden icons, forcing explorer to build automation peers on its UI thread while the user works with the taskbar - Refresh the cached rectangle only while the cursor is at the taskbar, since elsewhere the result cannot change any decision - Sample the mouse buttons every tick rather than only when the chevron is available, so the "pressed since the previous call" bit cannot report an arbitrarily old click as fresh - Clamp the hit area padding, log the name fallback once per streak Not adopted: the suggested height cap on tooltip candidates, meant to protect the chevron's context menu. Verified on this build that the chevron has no context menu, and the cap made tooltip hiding miss.
|
/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. Round four's menu-detection and re-find-backoff items are correctly fixed — 1. if (overlapsX && (overlapsFlyout || besideChevron)) {
ShowWindow(h, SW_HIDE);
}
So while explorer's UI thread is busy — which is not rare, and is exactly when a stray tooltip is most likely to be sitting on screen — the worker stalls inside
One-word fix: ShowWindowAsync(h, SW_HIDE);(or 2. Sampling SHORT kl = GetAsyncKeyState(VK_LBUTTON);
SHORT kr = GetAsyncKeyState(VK_RBUTTON);
SHORT km = GetAsyncKeyState(VK_MBUTTON);Moving these out of the The bit has exactly one consumer here — // `flyoutBelievedOpen` still holds last tick's value here.
bool anyBtnDown = false, pressedSinceTick = false;
if (s.autoClose && flyoutBelievedOpen) {
SHORT kl = GetAsyncKeyState(VK_LBUTTON);
...
}Note this also stops the polling entirely for users who turn auto-collapse off, where the result is never used at all. If you want to be strict about the one sample that can still be stale — the first tick after the flyout becomes visible — discard 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. |
- Hide the tooltip with ShowWindowAsync: the window belongs to explorer, so the synchronous form marshals into its UI thread and blocks until that thread handles it, which stalls the poll loop behind a busy shell and leaves the unload wait without a bound - Sample the mouse buttons only while auto-collapse is watching an open flyout: the "pressed since the previous call" bit is desktop-wide state that the first caller consumes, so polling it for the whole session took it away from every other application, while it is only ever read to decide whether a click landed inside an open flyout - Give the "identified, but not by class name" lines their own flag, which survives a successful find, so a build that always matches by name logs that once rather than on every acquisition - Floor the lookup cadence so repeatedly crossing the taskbar band cannot earn one full walk per crossing and bypass the backoff - Clear the collapse timer on the settings-change path too, so saving settings while the flyout is open cannot collapse it on the next tick - Widen the taskbar band to at least the hit-area padding, so there is no position treated as on the chevron but not as at the taskbar - Remove a duplicated comment line Not adopted: skipping the tooltip sweep for a second after any click. It would require polling the mouse state continuously, which is exactly what the second finding asks to stop doing.
|
/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. Both round-five items are fixed as described — the tooltip is hidden with
The gate added this round reads a flag that is only ever recomputed inside the bool anyBtnDown = false;
bool pressedSinceTick = false;
if (s.autoClose && flyoutBelievedOpen) { // evaluated every tick, unconditionally
SHORT kl = GetAsyncKeyState(VK_LBUTTON);
...
}
...
if (haveRect) {
...
flyoutBelievedOpen = flyoutVisible || cooling; // the only assignmentBoth paths that drop the element reset a batch of state and leave } else {
pBtn->Release(); pBtn = nullptr;
haveRect = false;
overBtnPrev = false;
insideSince = 0;
dwellFired = false;
leftAt = 0;
nextRefind = 0;
WaitForSingleObject(g_stopEvent, s.pollInterval);
continue;
}and the settings-change block, which clears So if the element is dropped on a tick where the flyout was open, The window is narrow — flyout visibility is re-checked every tick while One line in each reset, next to the existing ones: haveRect = false;
overBtnPrev = false;
insideSince = 0;
dwellFired = false;
leftAt = 0;
flyoutBelievedOpen = false; // nothing recomputes this without haveRect
clickedInFlyout = false; // same block; harmless but also stale here
nextRefind = 0;
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. |
- Clear flyoutBelievedOpen and clickedInFlyout on both paths that drop the element. Only the haveRect block assigns them, while the mouse sampling gate added last round reads flyoutBelievedOpen every tick, so dropping the chevron while the flyout was open latched the flag with no way back and resumed polling the shared key state for the rest of the session - Put the ambiguous-signature line on the flag that survives a successful find, like the other two weak-match lines - Record the previous mouse state right after sampling instead of at the end of the loop, which a `continue` can skip, leaving a two-ticks-old value that could produce a press edge that never happened - Let the positional guess consider the alternate chevron AutomationId, so it matches the test that admitted those candidates in the first place
|
/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 |
This updates the already-published Tray hover expand mod to 1.7.0.
The main change is how the chevron is identified. Until now it was matched by its
localized name, with a positional guess as a fallback, so users whose display
language was not in the keyword list fell through to that guess, which can select
a different tray button and invoke it. It is now identified by its class name
together with its AutomationId, which is the same in every display language, and
when it cannot be identified the mod does nothing at all instead of guessing.
The rest of the changes come from user reports about auto-collapse, the chevron
tooltip and tray icon context menus.
Changelog
display language, and accept it only when exactly one element matches
keyword and invoked
identified; the guess is now an opt-in setting, off by default
brushes past the chevron
ticks, and never collapse while a context menu is open
"Show hidden icons" and "Hide", and with the taskbar on either edge
corner of the screen into a hotspot and make the flyout cycle
repeated failures, so an auto-hiding taskbar works immediately on reveal and a
taskbar with no hidden icons costs almost nothing
defaults
Fixes #5257
Fixes #4684
Fixes #4732
Fixes #4747
Fixes #4524
Note on #4524: two of its three points are fixed here, along with the context
menu problem raised in a follow-up comment. The remaining one, the flyout
occasionally taking a second to disappear, I was unable to reproduce; the
reporter has been asked to say so if it persists.
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.