Update instant-taskbar-thumbnail-previews.wh.cpp - #4981
Conversation
|
/ai-review |
|
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. |
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 is small and well shaped: the new hook is optional and fails closed, the The close-button tag can survive its transition and re-flavor an unrelated dismissal
That is not an exotic path — it's the complement of the case your other mod (
The effect is bounded (one close is slower than configured), but it means the The cheapest robust fix is to bound the tag in time instead of relying on the next transition to consume it, since the close-button dismissal follows the click within a frame or two: // The dismiss transition caused by thumbnail removal follows the click almost
// immediately; anything later is an unrelated pointer exit.
#define THUMBNAIL_CLOSE_BUTTON_TAG_LIFETIME_MS 1000
thread_local ULONGLONG g_thumbnailCloseButtonClickTick = 0;
void WINAPI TaskItemThumbnailViewOnCloseButtonClicked_Hook(
void* object,
const void* sender,
const void* eventArguments) {
g_thumbnailCloseButtonClickTick = GetTickCount64();
TaskItemThumbnailViewOnCloseButtonClicked_Original(
object, sender, eventArguments);
}
void WINAPI TransitionToFlyoutDismissPendingState_Hook(void* object) {
ULONGLONG clickTick = g_thumbnailCloseButtonClickTick;
g_thumbnailCloseButtonClickTick = 0;
DismissDelaySource source =
(clickTick && GetTickCount64() - clickTick <=
THUMBNAIL_CLOSE_BUTTON_TAG_LIFETIME_MS)
? DismissDelaySource::thumbnailCloseButton
: DismissDelaySource::pointerExit;
DismissTransitionScope dismissTransitionScope(source);
TransitionToFlyoutDismissPendingState_Original(object);
}Optionally also clear it ( 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. |
55b65b6 to
5b4a3b6
Compare
|
/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 addition is small and well contained: one extra optional symbol hook, no new state that needs tearing down, the new tag is 1. The 1-second tag lifetime doesn't match what the comment says it's there for // Removal-triggered dismissal follows the collection event immediately
// Anything later is an unrelated pointer exit
constexpr ULONGLONG THUMBNAIL_REMOVAL_TAG_LIFETIME_MS = 1000;If a removal-driven dismissal really does follow the collection change immediately (same input/layout frame), then 1000 ms is orders of magnitude more than the correlation needs, and the surplus is exactly long enough for the user to do something unrelated. The mis-attribution case is a removal that does not itself start a dismiss transition — a background window of the hovered group closing, or a thumbnail closed while the pointer happens to stay over a neighbouring one. The tag stays armed, and the next genuine pointer-exit close within one second is then charged the removal delay: 500 ms instead of the 200 ms Suggest tightening One discriminator that looks tempting but isn't viable: clearing the tag when the pointer leaves the flyout frame ( Related: the hook tags on any 2. Default changes: the PR description and the code disagree, and existing users are affected The PR description says "Tweaked the default close delay to 150ms", but the diff sets More importantly, 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. |
Keep remaining previews open for a configurable delay whenever a thumbnail is removed, including close-button, supported middle-click, keyboard, and external window closes. Correlate removal events with the active flyout within a 150 ms window without adding timers. Default the general thumbnail close delay to 200 ms and the post-removal delay to 500 ms.
5b4a3b6 to
3eab9a1
Compare
|
/ai-review |
|
This pull request has already had 2 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 new "delay after thumbnail removal" feature itself is a reasonable design — reusing the existing 1. The README image is broken and uses a host that isn't allowed. Two problems: this isn't valid image markdown ( For reference, taskbar-dock-animation-plus.wh.cpp uses the 2. v1.0 used the standard wiki
Recommend restoring the standard implementation — see taskbar-notification-icon-spacing.wh.cpp#L861-L911, which is the pattern used by most taskbar mods (and what this mod shipped in 1.0). That deletes 3. Hooks registered during
That cost is paid on every Explorer start / mod reload for no benefit. Hooks registered in 4. Failed symbol resolution is retried up to three times against the same module.
Related: when the target modules never load (e.g. the mod is enabled on a Windows 10 Explorer, where neither 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. |
3b9f10e to
9002a6b
Compare
|
/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. Two things to fix — a broken README image and a lock that can hang Explorer. 1. The README image is broken and uses a disallowed host (line 18) Two separate problems:
Re-upload to imgur, or commit the file to your own GitHub repo and link it via Also note that if the asset is an 2. The new hook-install state machine takes
That's the classic inversion: thread A holds The lock isn't buying anything a one-shot atomic can't. Existing mods resolve each module exactly once with a plain void HandleLoadedModuleIfSystemTray(HMODULE module, LPCWSTR lpLibFileName) {
if (g_winVersion >= WinVersion::Win11 && !g_systemTrayModuleHooked &&
GetSystemTrayModuleHandle() == module &&
!g_systemTrayModuleHooked.exchange(true)) {
if (HookSystemTraySymbols(module)) {
Wh_ApplyHookOperations();
}
}
}Version 1.0 of this mod used the same 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:
Added a new feature / value to keep remaining previews open for a configurable delay after closing a window from its thumbnail. This way thumbnail previews won't instantly vanish when the user closes a thumbnail on the edge of a set of them because their cursor isn't immediately over another thumbnail.
Tweaked the default close delay to 200ms. I find this provides optimal expected behavior.
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.