Add Taskbar Horizontal Scroll Media Control v1.0.0 - #5023
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. |
ea819d0 to
2b89c97
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 gesture handling is solid, but the ShellHost half — capturing Windows' live 1. Debug diagnostics are written to persistent mod storage. There are 25 2. // In the teardown that runs on the XAML UI thread, after DestroyWindow:
UnregisterClassW(kCapturedHostClass, instance); // mod's own module handleSee autoscroll-win32.wh.cpp#L1051 for the pattern. 3. Global WinRT/XAML objects: missing [[clang::no_destroy]] winrt::Windows::UI::Xaml::FrameworkElement g_capturedMedia{nullptr};
[[clang::no_destroy]] winrt::Windows::UI::Xaml::Controls::Panel g_originalMediaParent{nullptr};
[[clang::no_destroy]] winrt::Windows::UI::Core::CoreDispatcher g_capturedDispatcher{nullptr};
[[clang::no_destroy]] winrt::Windows::UI::Xaml::Hosting::DesktopWindowXamlSource g_capturedXamlSource{nullptr};
[[clang::no_destroy]] winrt::com_ptr<MediaVisualTreeWatcher> g_visualTreeWatcher;and the explicit release must stay, but move Related: 4. Unload waits are bounded, then the DLL is unloaded regardless.
Once 5. The mod initializes in every DWORD shellPid = 0;
GetWindowThreadProcessId(GetShellWindow(), &shellPid);
if (shellPid != GetCurrentProcessId()) return FALSE; // not the shell instanceWorth noting more broadly: the 6. Forcing Quick Settings open and then hiding it. When nothing has been captured yet, This gets worse on builds where Quick Settings is not in 7. The captured element can go stale or be double-captured.
Handle 8. The dismiss timer is never stopped. 9. The shared events are writable by every process in the session, including untrusted ones. 10. License compatibility. The XAML-diagnostics TAP scaffolding ( 11. Ctrl+middle-click ignores the 12. Overlap with an existing mod. The track-switching half is close to Taskbar Scroll Actions, which already implements "assign an action to scrolling over the taskbar" with configurable scroll areas, step and throttling. Adding a next/previous track action plus horizontal-wheel support there would cover the gesture for everyone using that mod, and would leave this mod free to be just the standalone-Quick-Settings-card feature (which is genuinely new). Please state in the README how this differs from that mod, and consider proposing the gesture upstream instead. 13. Add a screenshot or GIF to the README. The mod's headline feature is a visible flyout hosting Windows' own media card; a screenshot (or a short GIF of the tilt gesture) makes it much easier to judge from the catalog. Only 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. |
2b89c97 to
f3c6b04
Compare
|
Addressed the review against the updated v1.0.0 commit:
The first-use ms-controlcenter bootstrap is intentionally retained: Windows does not instantiate the genuine ControlCenter.MediaTransportControls until its Quick Settings visual tree exists. The mod documents this, performs it only while no capture exists, and suppresses standalone creation whenever Quick Settings is already open. A screenshot was not added because this source-only PR has no approved external image URL yet. Validation: zero-warning Windhawk compile, clean official pr_validation.py, one changed mod file, and v1.0.0 remains a single commit. /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 gesture side is nicely built (the hook callback stays cheap, UI Automation is done on a worker thread, and the wheel latching logic is solid), but there are a few blocking problems around process detection, teardown and the Quick Settings bootstrap. 1. The mod never initializes in Explorer after a reboot or an Explorer restart.
if (g_role == ProcessRole::Explorer && !IsShellExplorerProcess()) {
Wh_Log(L"Skipping non-shell Explorer process");
return FALSE;
}
The simplest fix is to drop the process-level check and filter at the point where it actually matters — only the shell process owns bool IsTaskbarClass(HWND window) {
DWORD processId = 0;
GetWindowThreadProcessId(window, &processId);
if (processId != GetCurrentProcessId()) return false;
...
}That also makes the mod inert in short-lived / separate-process Explorer instances without needing 2. The host window and its window class survive mod unload → next load's flyout is dead, and ShellHost can crash.
{
std::lock_guard lock(g_captureMutex);
if (g_capturedDispatcher) dispatcher = g_capturedDispatcher;
}
if (dispatcher) { /* ...DestroyWindow + UnregisterCapturedHostClass... */ }But
Fix: keep a dispatcher for the host window's own thread that is not cleared when the captured element goes away (or simply don't null 3. Unload can hang ShellHost indefinitely. dispatcher.RunAsync(CoreDispatcherPriority::Low, [completed] { ... });
WaitForSingleObject(completed, INFINITE);
4. BOOL CALLBACK FindControlCenterWindowProc(HWND window, LPARAM parameter) {
wchar_t className[128];
GetClassNameW(window, className, ARRAYSIZE(className));
if (wcscmp(className, L"ControlCenterWindow") == 0) { ... }…followed by Also, both callbacks 5. When the ShellHost half isn't available, every gesture pops open Quick Settings and leaves it open. The router falls back to Please (a) gate the bootstrap so it is attempted at most once per ShellHost lifetime and never when the ShellHost half hasn't announced itself (a separate manual-reset "alive" event signalled from Related: 6.
7. Don't call
8. 100 ms polling on a shell UI thread. SetTimer(g_capturedHostWindow, kDismissTimer, 100, nullptr);Every tick runs 9. Process model — please justify keeping the gesture inside explorer.exe. The Explorer role installs no function hooks at all; it only uses 10. Overlap with Taskbar Scroll Actions. The README pre-empts this, and the standalone media card is genuinely new — but "wheel over the taskbar performs an action" is already owned by Taskbar Scroll Actions, which has a general action/area/step/throttle model. The maintainer's consistent preference is to extend an existing mod rather than add a variation. Adding a "next/previous track" action plus horizontal-wheel support there would cover the gesture half with settings users already know. Worth considering whether this mod should be just the media-card feature. 11. Add a screenshot or GIF to the README. The mod's headline feature is a visible card next to the taskbar and there's no image of it. 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. |
f3c6b04 to
8f09c57
Compare
|
Addressed the review against the updated v1.0.0 commit:
Optional cleanup included removing the unnecessary mutex Validation: zero-warning Windhawk compile, clean official /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 mod is well documented and the TAP boilerplate matches the reference mods, but the teardown path breaks Windhawk's unload contract and the input layer is heavier than it needs to be. 1. HMODULE keepAlive = nullptr;
GetModuleHandleExW(GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS, ..., &keepAlive);
...
if (wait == WAIT_OBJECT_0) {
if (keepAlive) FreeLibrary(keepAlive);
} else {
// Keep the image mapped if the UI dispatcher runs this callback late.Windhawk expects exactly one reference on the mod module when
Additionally, even on the success path The fix is to make the teardown synchronous on the XAML UI thread instead of an async 2. The UI Automation object is released after HRESULT apartmentResult = RoInitialize(RO_INIT_MULTITHREADED);
bool uninitializeApartment = SUCCEEDED(apartmentResult);
winrt::com_ptr<IUIAutomation> automation; // function scope
...
if (uninitializeApartment) RoUninitialize();
return 0; // ~automation runs here
automation = nullptr;
if (uninitializeApartment) RoUninitialize();3. Unload can hang Explorer. if (DWORD threadId = g_hookThreadId.load())
PostThreadMessageW(threadId, WM_QUIT, 0, 0);
if (g_hookThread) WaitForSingleObject(g_hookThread, INFINITE);
Related: the router thread is also joined with 4. A system-wide The repo convention is to handle this in the taskbar itself:
Both use 5. XAML diagnostics TAP conflict in ShellHost. 6. Overlap with Taskbar Scroll Actions. The standalone Quick Settings card is genuinely new, and the README already explains the difference — thanks for that. But the gesture half (wheel over the taskbar → an action, with a modifier and a direction-reverse option) is exactly what 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. |
8f09c57 to
8c50bf9
Compare
|
Addressed the review against the updated v1.0.0 commit:
Optional cleanup also replaced opaque numeric-stage log wrappers with descriptive Validation: zero-warning Windhawk compile against the installed 1.7.3 engine import library, clean official /ai-review |
8c50bf9 to
a9085bc
Compare
|
One final teardown ordering refinement is included in The full response to the preceding review is in the immediately previous comment. Compile and catalog validation remain clean. /ai-review |
|
@shreyasjswork |
|
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 |
Adds Taskbar Horizontal Scroll Media Control v1.0.0.
The mod provides next/previous track control through horizontal mouse-wheel tilt over the taskbar, optional modifier handling, Ctrl + middle-click play/pause on empty taskbar space, continuous-input suppression, and Windows' genuine Quick Settings media card as a standalone flyout when the panel is closed.
Validation:
pr_validation.pycheck passed forshreyasjswork.mods/taskbar-horizontal-scroll-media.wh.cpp.Changelog
Not applicable — this pull request introduces a new mod.
Mod authorship
This mod was created by: