Skip to content

Add: Gallery -> Photos App Launcher mod - #5047

Open
jakubix30 wants to merge 19 commits into
ramensoftware:mainfrom
jakubix30:main
Open

Add: Gallery -> Photos App Launcher mod#5047
jakubix30 wants to merge 19 commits into
ramensoftware:mainfrom
jakubix30:main

Conversation

@jakubix30

Copy link
Copy Markdown

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

@jakubix30

Copy link
Copy Markdown
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 8, 2026
@jakubix30

Copy link
Copy Markdown
Author

/ai-review

@windhawk-reviewer

Copy link
Copy Markdown

@jakubix30 /ai-review can't be applied here: an AI review was already requested, please wait for it to be posted.

@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 idea (intercept at the BrowseObject chokepoint) is sound, and the @include scope is correctly narrowed to explorer.exe. The problems are all in how the hook gets installed — the polling worker thread creates several ways to crash or hang Explorer, and all of them disappear if the hook is installed with HookSymbols in Wh_ModInit instead.

1. The worker thread can outlive the mod, and can install a hook after Windhawk has already removed the mod's hooks.

This is the most serious issue, and it has three separate failure modes:

  • Wh_ModUninit waits with a timeout:

    WaitForSingleObject(g_workerThread, 5000);
    CloseHandle(g_workerThread);

    If the wait times out, Wh_ModUninit returns while WorkerProc is still running. Windhawk then FreeLibrarys the mod image, and the worker's instruction pointer / return address point into unmapped memory → Explorer crashes. The wait must be INFINITE — the mod must be fully unloadable the moment Wh_ModUninit returns.

  • The worker can block indefinitely in SendMessageW(hwndExplorer, WM_USER + 7, 0, 0) if the target Explorer UI thread is busy or hung, which is exactly what makes the timeout above fire. Worse, Wh_ModUninit runs on an arbitrary thread — if it happens to be that same UI thread, you get a hard deadlock. Use SendMessageTimeoutW(..., SMTO_ABORTIFHUNG, 1000, ...) like remove-context-menu-items.wh.cpp#L369 does.

  • The worker calls Wh_SetFunctionHook / Wh_ApplyHookOperations from a background thread at an arbitrary time. windhawk_api.h documents that both "can't be called after Wh_ModBeforeUninit returns". The mod has no Wh_ModBeforeUninit, so Windhawk removes the mod's hooks before Wh_ModUninit runs — and the worker is still alive during Wh_ModUninit. A hook installed in that window survives the removal pass and leaves a detour pointing into the unmapped mod image → crash on the next navigation. If you keep the worker, it must be stopped and joined in Wh_ModBeforeUninit, not Wh_ModUninit.

2. Install the hook with HookSymbols in Wh_ModInit instead of polling for a window.

This removes the worker thread, the WM_USER + 7 round-trip, the hardcoded vtable index, and the Wh_ApplyHookOperations call — i.e. it makes item 1 moot — and it also fixes real functional gaps: right now, if the mod is enabled at Explorer startup before any window exists, the worker spins on a 2-second poll indefinitely (forever, if the user never opens an Explorer window), and any navigation that happens before the first successful poll is missed.

CShellBrowser::BrowseObject in explorerframe.dll is the same function you're patching via vtable[11]. file-explorer-content-animation.wh.cpp#L1856-L1870 already does exactly this:

HMODULE hEF = GetModuleHandleW(L"explorerframe.dll");
if (hEF) {
    WindhawkUtils::SYMBOL_HOOK explorerframe_dll_hooks[] = {
        {
            {
                LR"(public: virtual long __cdecl CShellBrowser::BrowseObject(struct _ITEMIDLIST_RELATIVE const __unaligned *,unsigned int))",
                LR"(public: virtual long __cdecl CShellBrowser::BrowseObject(struct _ITEMIDLIST_RELATIVE const * __ptr64,unsigned int))",
                LR"(public: virtual long __cdecl CShellBrowser::BrowseObject(struct _ITEMIDLIST const *,unsigned int))",
            },
            (void**)&g_BrowseObject_Original,
            (void*)BrowseObject_Hook,
        },
    };
    WindhawkUtils::HookSymbols(hEF, explorerframe_dll_hooks,
                               ARRAYSIZE(explorerframe_dll_hooks));
}

Note explorerframe.dll may not be loaded yet when the mod initializes at process start. Handle that by hooking LoadLibraryExW and applying the hook when it loads — and hook the kernelbase.dll copy, not the kernel32 import the mod links against, since internal callers go straight to kernelbase:

HMODULE kernelBase = GetModuleHandleW(L"kernelbase.dll");
auto pLoadLibraryExW = (decltype(&LoadLibraryExW))GetProcAddress(kernelBase, "LoadLibraryExW");
WindhawkUtils::SetFunctionHook(pLoadLibraryExW, LoadLibraryExW_hook, &LoadLibraryExW_orig);

3. The PIDL is treated as absolute regardless of wFlags.

SHGetNameFromIDList(reinterpret_cast<PCIDLIST_ABSOLUTE>(pidl), SIGDN_DESKTOPABSOLUTEPARSING, &pszName);

BrowseObject's pidl is only absolute when SBSP_ABSOLUTE (which is 0) is in effect. With SBSP_RELATIVE it is relative to the currently browsed folder — that's the flag used when you double-click a folder in the view. Casting that to PCIDLIST_ABSOLUTE makes the shell parse it as if it were rooted at the desktop, so the resulting name is meaningless. Guard on the flags before doing the lookup:

if (wFlags & (SBSP_RELATIVE | SBSP_PARENT | SBSP_NAVIGATEBACK | SBSP_NAVIGATEFORWARD)) {
    return g_BrowseObject_Original(pThis, pidl, wFlags);  // not an absolute pidl
}

4. The mod blocks navigation even when the launch fails, and it launches synchronously on the Explorer UI thread.

ShellExecuteW(NULL, L"open", g_settings.targetCommand.c_str(), NULL, NULL, SW_SHOWNORMAL);
return S_OK; // Zwracamy sukces, ale NIE nawigujemy

If the Photos app isn't installed (or the user typo'd targetCommand), ms-photos: is an unregistered protocol: the user gets a shell error dialog and Gallery becomes permanently unreachable, with no way to tell what went wrong. Check the result and fall through to the original on failure:

if ((INT_PTR)ShellExecuteW(...) <= 32) {
    return g_BrowseObject_Original(pThis, pidl, wFlags);
}

Separately, ShellExecuteW on a ms-photos: URI goes through package activation and can block for a noticeable time. BrowseObject runs on the Explorer UI thread, so the whole window freezes for the duration. Do the launch off that thread — e.g. a single worker thread created in Wh_ModInit, signalled with an event from the hook, and stop-signalled + WaitForSingleObject(..., INFINITE)-joined in Wh_ModUninit.

5. User-facing strings must default to English.

The settings block and several log strings are Polish only:

  $name: Ustawienia
  $description: CLSID elementu do przechwycenia i komenda do uruchomienia.
Wh_Log(L"[GalleryLauncher] === PRZECHWYCONO GALERIE! Uruchamianie: %s ===", ...);
Wh_Log(L"[GalleryLauncher] Inicjalizacja moda v2.0");

Use English as the default and add Polish via the localization suffix ($name:pl-PL, $description:pl-PL) — the mod name/description in the metadata block already do the right thing by being English.

6. Drop the debugLog setting and the [GalleryLauncher] log prefixes.

Windhawk already provides a per-mod logging toggle in the mod's settings UI, and Wh_Log already prefixes the mod name — a mod-defined log-level setting and a manual [ModName] prefix are redundant. Just call Wh_Log unconditionally and delete the setting. (Also, the README's "Detailed debug logging via OutputDebugString (viewable in DbgView)" is inaccurate — the mod uses Wh_Log, which is viewed in the Windhawk log; that bullet goes away with this change.)

Optional improvements

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

  • Settings race. Wh_ModSettingsChanged reassigns g_settings.targetClsid / targetCommand (both std::wstring) on an arbitrary thread while the hook may be reading them on an Explorer UI thread. Concurrent read/write of a std::wstring is a data race that can crash. It only happens on a settings change, so it's low priority, but it's easy to avoid — guard with a mutex, or use std::shared_ptr<const Settings> swapped atomically.

  • volatile bool isn't a synchronization primitive in C++. Use std::atomic<bool> for g_stopWorker and g_hooked (if you keep the worker at all).

  • WindhawkUtils::SetFunctionHook / StringSetting. Prefer the type-safe WindhawkUtils::SetFunctionHook() over raw Wh_SetFunctionHook with void* casts, and WindhawkUtils::StringSetting (RAII) over Wh_GetStringSetting + manual Wh_FreeStringSetting. Also, Wh_GetStringSetting never returns NULL (it returns L"" on error/unset), so the s && *s check can just be *s.

  • Unused dependencies. -lcomctl32, -lshlwapi and -luuid aren't used by anything in the file (no comctl32/shlwapi call, no IID_* referenced) — -lole32 alone covers CoTaskMemFree, and shell32 is linked by default (see start-menu-open-real-file-location.wh.cpp which uses SHGetNameFromIDList with just -lole32). #include <shlwapi.h> is unused too. Conversely, towlower comes from <cwctype> — include it rather than relying on a transitive include.

  • Per-setting names. targetClsid, targetCommand and debugLog have no $name/$description, so users see the raw keys in the settings UI. Also, the single settings: group wrapper doesn't buy anything — three top-level settings with proper names would read better.

  • Generic implementation vs. specific name. The mod is named/described as "Gallery → Photos" but is implemented as a generic "redirect any shell folder CLSID to any command". Either lean into that (rename and describe it as a generic redirector, which is arguably more useful) or hardcode the Gallery CLSID and drop the settings — asking users to paste raw CLSIDs is rough UX for a mod whose stated purpose is one specific folder.

  • Validate the WM_USER + 7 result before dereferencing *(void***)pShellBrowser and patching vtable[11]. If any window in the chain ever returns a non-IShellBrowser value for that message, the mod patches an arbitrary code address. A QueryInterface(IID_IShellBrowser, ...) check (plus the matching Release) would make it safe. Moot if you switch to HookSymbols per item 2.

  • README visual. The mod has a visible effect (Photos opening instead of the Gallery view) — a short GIF would help users understand what they're getting. Only i.imgur.com and raw.githubusercontent.com are allowed image hosts.

  • Code comments are mostly Polish. Not a rule, but English comments make future maintenance by others easier.

Functionality notes

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

  • CLSID matching is a substring search on the parsing name. contains_ci(name, clean) matches Gallery and everything below it, plus any path that merely contains that GUID as text. It also costs a SHGetNameFromIDList shell round-trip on the UI thread for every navigation. Resolving the target once at init and comparing PIDLs directly is both cheaper and exact:

    // once, at init:
    SHParseDisplayName(L"shell:::{e88865ea-0e1c-4e20-9aa6-edcd0212c87c}", nullptr, &g_targetPidl, 0, nullptr);
    // in the hook:
    if (ILIsEqual(reinterpret_cast<PCIDLIST_ABSOLUTE>(pidl), g_targetPidl)) { ... }

    (Free it with CoTaskMemFree on unload.) If you want subfolders of Gallery to redirect as well, ILIsParent covers that explicitly.

  • Back/forward navigation to Gallery isn't intercepted. With SBSP_NAVIGATEBACK / SBSP_NAVIGATEFORWARD the pidl is NULL, so IsTargetPidl returns false and Explorer navigates to Gallery from history as normal. Whether that matters depends on how you want the mod to feel — worth a conscious decision either way.

  • Returning S_OK without navigating. The caller believes the navigation succeeded, so the nav pane may end up with Gallery selected while the view still shows the previous folder. Worth testing what that actually looks like; you may prefer returning a failure HRESULT, or re-selecting the previous item.


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 8, 2026
@jakubix30

Copy link
Copy Markdown
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 8, 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 idea is sound and the hook point is well chosen — CShellBrowser::BrowseObject is the same chokepoint file-explorer-content-animation uses, and the symbol strings match it verbatim. There's no meaningful overlap with existing mods either (hide-home-gallery-explorer removes the Gallery entry rather than redirecting it). The items below are about the plumbing around that hook.

1. The late-load path never actually installs the hook — Wh_ApplyHookOperations() is missing.

Windhawk applies registered hook operations automatically only when Wh_ModInit returns. HookExplorerFrame() is also called from LoadLibraryExW_Hook (line 202), i.e. long after init, where nothing applies them — so if explorerframe.dll ever loads after init, HookSymbols registers the hook and it is silently never installed. From windhawk_api.h:

Applies hook operations registered by Wh_SetFunctionHook and Wh_RemoveFunctionHook. Called automatically by Windhawk after Wh_ModInit.

Have HookExplorerFrame apply the operations when it isn't running inside Wh_ModInit, e.g. an applyNow parameter:

static void HookExplorerFrame(HMODULE hEF, bool applyNow) {
    ...
    if (WindhawkUtils::HookSymbols(hEF, explorerframe_dll_hooks,
                                   ARRAYSIZE(explorerframe_dll_hooks))) {
        if (applyNow && !Wh_ApplyHookOperations()) {
            Wh_Log(L"Wh_ApplyHookOperations failed");
        }
    }
}

See explorer-folder-hover-menu.wh.cpp#L2353-L2365 for the same init-vs-runtime split.

Related: the re-entry guard is g_BrowseObject_Original != nullptr, which is only ever set if symbol resolution succeeded. If it fails once, every subsequent LoadLibraryExW(L"explorerframe.dll") in the process re-runs HookSymbols on the same module. HookSymbols must not be called more than once per module — each extra call invalidates its symbol cache and forces a slow re-resolution (and can re-show the symbol-download UI). Guard on "already attempted for this module" instead.

2. The worker thread never initializes COM before ShellExecuteW.

LaunchWorkerProc (line 127) is a fresh thread with no apartment. ms-photos: is an AppX protocol activation, which ShellExecuteW delegates to COM-based shell handlers; MSDN says COM should be initialized (STA) before calling it. Initialize once at thread entry and uninitialize before returning:

static DWORD WINAPI LaunchWorkerProc(LPVOID) {
    HRESULT hrCo = CoInitializeEx(nullptr, COINIT_APARTMENTTHREADED |
                                               COINIT_DISABLE_OLE1DDE);
    ...
    if (SUCCEEDED(hrCo)) {
        CoUninitialize();
    }
    return 0;
}

Same pattern in taskbar-empty-space-clicks.wh.cpp#L655.

3. Match the PIDL, not a substring of its parsing name.

IsTargetPidl (line 93) calls SHGetNameFromIDList(..., SIGDN_DESKTOPABSOLUTEPARSING, ...) on every navigation, on the Explorer UI thread, and then does a case-insensitive substring search for the GUID text. Two problems:

  • It's a shell display-name round trip in the navigation hot path (for non-filesystem/network PIDLs this can bind to the folder and block).
  • Substring matching is a false-positive source — any location whose parsing name merely contains the GUID text matches (GUID-named folders are common in package/installer caches, and the CLSID is user-configurable).

Resolve the target once (in LoadSettings) and compare PIDLs instead:

// L"::" + clsid, e.g. L"::{e88865ea-0e1c-4e20-9aa6-edcd0212c87c}"
SHParseDisplayName(target.c_str(), nullptr, &g_targetPidl, 0, nullptr);
...
if (g_targetPidl && (ILIsEqual(pidl, g_targetPidl) ||
                     ILIsParent(g_targetPidl, pidl, FALSE))) { ... }

That's exact, locale-independent, and costs nothing per navigation. Free the PIDL with CoTaskMemFree/ILFree in Wh_ModUninit and when settings change.

4. Drop the ExplorerPatcher framing from the mod name and description.

The name is Gallery -> Photos App Launcher (ExplorerPatcher Win10 Fix), but the README itself states the mod "works with ExplorerPatcher (Win10 overlay) and without it" — the BrowseObject hook is UI-independent. Naming it after a third-party tool makes it hard to find for the users it actually serves, and the maintainer has repeatedly asked for working-title/qualifier suffixes to be dropped from mod names. Something like Open Photos App instead of Gallery describes what it does; the ExplorerPatcher compatibility note belongs in the README body (where it already is).

Optional improvements

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

  • Make the worker loop fully event-driven. WaitForSingleObject(g_launchEvent, 500) (line 129) wakes the thread twice a second forever inside explorer.exe just to re-check a flag, and delays unload by up to 500 ms. Add a second manual-reset stop event and WaitForMultipleObjects on both, so the loop blocks indefinitely and exits immediately on shutdown. While there, volatile bool g_stopWorker isn't a synchronization primitive — use std::atomic<bool> (or just rely on the stop event).

  • LoadLibraryW(L"explorerframe.dll") (line 231) loads by bare name. ExplorerFrame.dll is not a KnownDLL, so the default search order includes the executable's own directory. The blast radius here is small since the mod only injects into %SystemRoot%\explorer.exe, but it's free to harden: LoadLibraryExW(L"explorerframe.dll", nullptr, LOAD_LIBRARY_SEARCH_SYSTEM32).

  • g_settings is read on the Explorer UI thread and rewritten from Wh_ModSettingsChanged. std::wstring assignment while IsTargetPidl reads g_settings.targetClsid is a data race that can dereference a freed buffer. Only reachable on a settings change, so low priority — but if you move to the PIDL comparison in item 3 above, guarding a single PIDLIST_ABSOLUTE with a lock or std::atomic swap is trivial.

  • g_cmdToLaunch is a WCHAR[MAX_PATH] written by the UI thread and read by the worker without synchronization, and wcsncpy_s(..., _TRUNCATE) silently truncates a longer targetCommand. Since the command comes from settings, just snapshot it into a std::wstring under the same lock as above.

  • Compare the file name, not the whole path, in LoadLibraryExW_Hook. contains_ci(lpLibFileName, L"explorerframe.dll") (line 200) allocates and lowercases two strings on every DLL load in the process, and would also match e.g. notexplorerframe.dll. Use the last path component with _wcsicmp.

  • Unused link libraries and includes. Nothing in the mod uses comctl32 or shlwapi APIs, and no IID_/CLSID_ constants are referenced, so -lcomctl32 -lshlwapi -luuid can go from @compilerOptions; likewise #include <shlwapi.h>. (-lole32 is still needed for CoTaskMemFree.) Also towlower is used without including <cwctype>.

  • The two settings have no $name/$description. targetClsid and targetCommand will render as raw keys in the settings UI. Add per-setting names/descriptions (with the :pl-PL variants you already use for the group).

  • Consider a short GIF in the README showing the Gallery click opening Photos — mods with a visible effect are much easier to evaluate from the catalog page with one.

Functionality notes

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

  • Returning S_OK without navigating may leave the UI inconsistent. The navigation pane has usually already moved its selection to Gallery by the time BrowseObject is called, so the tree can end up highlighting Gallery while the content pane and address bar still show the previous folder. Worth checking on your setup; if it happens, either restore the previous selection or experiment with a failure HRESULT (test that one carefully — Explorer may surface an error UI).

  • Back/Forward and parent navigations are exempt. Filtering out SBSP_NAVIGATEBACK/SBSP_NAVIGATEFORWARD/SBSP_PARENT/SBSP_RELATIVE (line 148) means Gallery can still be reached through history. That looks deliberate (and it's the right call for relative PIDLs, which can't be passed to SHGetNameFromIDList as absolute), but users may see it as inconsistent — a sentence in the README would help.

  • Sub-items of Gallery are also intercepted. Because the match is a substring of the desktop-absolute parsing name, anything under the Gallery node matches too. ILIsEqual vs. ILIsParent in item 3 lets you make that choice explicit.

  • Launch requests coalesce. g_launchEvent is auto-reset and only one command slot exists, so two rapid triggers produce one launch. That's almost certainly what you want, just noting it's implicit.

  • Possible generalization: since the CLSID and the command are already settings, supporting a small list of CLSID → command pairs would turn this into a general "redirect a shell folder to an app" mod at almost no extra cost.


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 8, 2026
@jakubix30

Copy link
Copy Markdown
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 8, 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 overall approach is sound — hooking CShellBrowser::BrowseObject is the right interception point, the kernelbase.dll LoadLibraryExW hook for late-loading explorerframe.dll is the correct pattern, and the worker thread is properly joined in Wh_ModUninit. The issues below are mostly around the worker thread and the module-detection logic.

1. The worker thread wakes up twice a second, forever, in every Explorer process

LaunchWorkerProc waits with a 500 ms timeout purely so it can re-check g_stopWorker:

while (!g_stopWorker) {
    DWORD waitRes = WaitForSingleObject(g_launchEvent, 500);

That is a polling loop: it keeps a thread in every explorer.exe waking twice per second for the lifetime of the process, even though nothing ever happens between navigations. Use a second (manual-reset) stop event and wait on both handles with INFINITE — that removes the wakeups and makes unload immediate instead of taking up to 500 ms.

While you're there: the thread initializes an STA (COINIT_APARTMENTTHREADED) but never pumps messages. ShellExecuteEx/ShellExecute can delegate to Shell extensions that require a pumping STA, and a non-pumping STA is a classic source of cross-apartment COM hangs — which matters here because Wh_ModUninit blocks on this thread (see item 2). MsgWaitForMultipleObjectsEx fixes both at once:

static HANDLE g_stopEvent = NULL;  // CreateEventW(NULL, TRUE, FALSE, NULL) — manual reset

static DWORD WINAPI LaunchWorkerProc(LPVOID) {
    HRESULT hrCo = CoInitializeEx(nullptr, COINIT_APARTMENTTHREADED | COINIT_DISABLE_OLE1DDE);

    HANDLE handles[] = {g_stopEvent, g_launchEvent};
    for (;;) {
        DWORD r = MsgWaitForMultipleObjectsEx(ARRAYSIZE(handles), handles, INFINITE,
                                              QS_ALLINPUT, MWMO_INPUTAVAILABLE);
        if (r == WAIT_OBJECT_0) {
            break;  // stop requested
        }
        if (r == WAIT_OBJECT_0 + 1) {
            // ... launch ...
        } else if (r == WAIT_OBJECT_0 + ARRAYSIZE(handles)) {
            MSG msg;
            while (PeekMessageW(&msg, nullptr, 0, 0, PM_REMOVE)) {
                TranslateMessage(&msg);
                DispatchMessageW(&msg);
            }
        } else {
            break;  // WAIT_FAILED
        }
    }

    if (SUCCEEDED(hrCo)) {
        CoUninitialize();
    }
    return 0;
}

and in Wh_ModUninit, SetEvent(g_stopEvent); before the WaitForSingleObject on the thread handle (keep the join — a thread must not outlive Wh_ModUninit, or the host crashes when the mod image is unmapped).

Related edge case in the current code: if CreateEventW fails, g_launchEvent is NULL, WaitForSingleObject(NULL, 500) returns WAIT_FAILED immediately, and the loop spins at 100% CPU. The else break; above covers that; alternatively bail out of Wh_ModInit if the events can't be created.

2. Wh_ModUninit can block (or deadlock) Explorer while a launch is in flight

WaitForSingleObject(g_workerThread, INFINITE) waits for a thread that may be inside ShellExecuteW, which for a ms-photos: URI goes through out-of-process shell/AppX activation and can take seconds and send messages. Best case, disabling/updating the mod stalls for that long; worst case, if Wh_ModUninit happens to run on a thread the activation needs to reach, it deadlocks and hangs Explorer. Wh_ModUninit runs on an arbitrary thread, so you can't assume it isn't one of them.

The join itself has to stay, so the fix is to shorten the blocking window: pump messages on the worker (item 1, so it can service incoming cross-apartment calls while blocked), and consider ShellExecuteExW with SEE_MASK_ASYNCOK | SEE_MASK_FLAG_NO_UI so the shell is allowed to return before the activation completes rather than blocking the worker for the whole launch.

3. Module detection: substring match on an arbitrary path, and a non-atomic guard flag

if (contains_ci(lpLibFileName, L"explorerframe.dll")) {
    HookExplorerFrame(hMod, true);
}

Two problems:

  • lpLibFileName is whatever the caller passed — any path merely containing that text matches (C:\explorerframe.dll.bak\something.dll, a folder named explorerframe.dll_old, …). Because HookExplorerFrame sets g_hookAttempted = true unconditionally on entry, one such false positive permanently prevents the real explorerframe.dll from ever being hooked, and the mod silently does nothing.
  • g_hookAttempted is a plain bool written from the LoadLibraryExW hook, which runs on arbitrary threads. Two concurrent loads of explorerframe.dll can both observe false and both call HookSymbols for the same module. Windhawk caches resolved symbols per module, and a second HookSymbols call for the same module invalidates that cache and forces a slow re-resolution — it must be called at most once per module.

Compare module handles instead of strings and use an atomic exchange, which is what explorer-frame-classic does:

static std::atomic<bool> g_explorerFrameHooked;

static void HookExplorerFrame(HMODULE hEF, bool applyNow) {
    if (!hEF || g_explorerFrameHooked.exchange(true)) {
        return;
    }
    // ...
}

static HMODULE WINAPI LoadLibraryExW_Hook(LPCWSTR lpLibFileName, HANDLE hFile, DWORD dwFlags) {
    HMODULE hMod = g_LoadLibraryExW_Original(lpLibFileName, hFile, dwFlags);
    if (hMod && hMod == GetModuleHandleW(L"explorerframe.dll")) {
        HookExplorerFrame(hMod, true);
    }
    return hMod;
}

That also makes contains_ci (and #include <shlwapi.h>) dead code — delete them.

Optional improvements

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

  • Settings-change race on g_targetPidl and g_settings.targetCommand. Wh_ModSettingsChangedLoadSettings runs on an arbitrary thread while the hooks are active, and it CoTaskMemFrees g_targetPidl and reassigns g_settings.targetCommand (reallocating the std::wstring buffer). BrowseObject_Hook reads both on Explorer's UI thread with no synchronization, so a navigation that races a settings change can dereference freed memory and crash Explorer. It only happens on a settings change, hence the optional bucket, but the fix is cheap — take a std::mutex around both the update in LoadSettings and the read in the hook (copy the command to a local, release the lock, then SetEvent).

  • SHParseDisplayName's result is ignored. If parsing fails — unsupported build, Gallery disabled by policy, or just a typo in the user-configurable targetClsidg_targetPidl stays nullptr, the mod silently does nothing, and there's no log line to explain why. Check the HRESULT and Wh_Log it.

  • SYMBOL_HOOK entries are cast to void. (void**)&g_BrowseObject_Original / (void*)BrowseObject_Hook defeat the strong-typed SYMBOL_HOOK constructor, which exists precisely to catch a hook whose signature doesn't match the original. Drop the casts and pass &g_BrowseObject_Original, BrowseObject_Hook directly — that's how explorer-frame-classic and taskbar-clock-customization write it.

  • Small gap between the GetModuleHandleW check and hook activation. Hooks registered in Wh_ModInit only become active when it returns, so an explorerframe.dll load on another thread in that window is missed by both paths. explorer-frame-classic closes it with a re-check in Wh_ModAfterInit — the atomic guard from item 3 makes that safe to add.

  • Unused link libraries and includes. -luuid, -lcomctl32 and -lshlwapi don't appear to be used by anything in the mod (SHParseDisplayName, ShellExecuteW, ILIsEqual, ILIsParent are all shell32, which mingw links by default; -lole32 is genuinely needed for CoTaskMemFree/CoInitializeEx). #include <shlwapi.h> is unused too, and towlower should come with an explicit #include <cwctype> rather than a transitive include.

  • volatile bool g_stopWorkerstd::atomic<bool>. volatile isn't a threading primitive in C++; std::atomic<bool> is what you want. (With the stop-event rewrite in item 1 the flag can go away entirely.)

  • Redundant null checks and manual string-setting management. Wh_GetStringSetting never returns NULL — it returns L"" when unset or on error — so s && *s can be *s. Also, WindhawkUtils::StringSetting is the RAII form and removes the manual Wh_FreeStringSetting calls.

  • Duplicated defaults. The defaults live both in the settings block and as literals in LoadSettings. Since Windhawk already returns the declared default, the fallbacks only trigger when a user clears the field, which silently resurrects the default instead of doing nothing — probably fine, just noting it's a second copy to keep in sync.

  • The settings: group wrapper doesn't earn its keep. It adds a settings. prefix to every key and a generic $name: Settings / $description: Target item CLSID to intercept and command to execute for the user. Two top-level settings with real names and descriptions read better. More broadly — is exposing a raw CLSID as a user setting intentional, or an AI-generated artifact? For a mod named "Open Photos App instead of Gallery", a configurable targetCommand makes sense (some users prefer a different viewer), but targetClsid is hard to explain to a user and easy to break.

  • g_cmdToLaunch[MAX_PATH] silently truncates a command longer than 259 characters, and it's an extra global that only exists to carry a value the worker could read from g_settings directly (under the mutex from the first bullet).

  • Wh_Log(L"Initializing mod v2.0") hardcodes the version, which will drift from @version. WH_MOD_VERSION is available as a wide-string literal.

  • No screenshot/GIF in the README. The effect is visible (Photos opens instead of the Gallery view), so a short GIF would help users understand what they're getting. Only i.imgur.com and raw.githubusercontent.com are allowed image hosts.

Functionality notes

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

  • Returning S_OK claims the navigation succeeded. The hook blocks the navigation but reports success, so the caller (nav pane CNscTree, breadcrumb bar, address bar) may update its own state — selection highlight, travel log — as if Explorer had moved to Gallery, leaving the UI out of sync with the content pane. Worth testing what each entry point does, and whether S_FALSE or HRESULT_FROM_WIN32(ERROR_CANCELLED) behaves better.

  • Explorer opened directly at Gallery. If a window is launched straight into Gallery (e.g. explorer.exe shell:::{e88865ea-0e1c-4e20-9aa6-edcd0212c87c}, a pinned shortcut, or a jump-list entry), the hook cancels the only navigation that window ever gets, so Photos opens and a blank Explorer window is left behind. Consider redirecting to a fallback location (Home / This PC) instead of just returning when there's nothing to fall back to.

  • ILIsParent(g_targetPidl, pidl, FALSE) also intercepts descendants of Gallery, not just Gallery itself. Intentional? Worth a line in the README either way.

  • Symbol resolution cost. Hooking CShellBrowser::BrowseObject requires the explorerframe.dll PDB, so the first run after each Windows update triggers a symbol download for the user. That's the normal trade-off for symbol hooks and there's no clean alternative — just noting it, and that it's worth stating in the README which Windows builds you've tested on (the README mentions ExplorerPatcher compatibility but not the tested builds).


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 8, 2026
@jakubix30

Copy link
Copy Markdown
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 8, 2026
@jakubix30

Copy link
Copy Markdown
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 11, 2026
@windhawk-reviewer

Copy link
Copy Markdown

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 again after 2026-08-11 19:04 UTC (in 21 minutes) to get another one.

@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 11, 2026
@jakubix30

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


No blocking issues found — the core of this mod is in good shape. A few things it gets right that are worth calling out, since they're the usual failure points:

  • explorerframe.dll late-loading is handled correctly, hooking LoadLibraryExW resolved from kernelbase.dll rather than the kernel32 import — that's the pattern that actually catches internal loads, same as mods/explorer-frame-classic.wh.cpp#L893. Both the already-loaded and the late-load paths are covered.
  • HookSymbols is called at most once for the module (the g_explorerFrameHooked CAS latch), so the symbol cache isn't invalidated by repeated calls.
  • The worker thread is signalled and joined inside Wh_ModUninit before the handles are closed, so the mod is unloadable when Wh_ModUninit returns. All globals are trivially destructible, so there's nothing that misbehaves at process shutdown.
  • Settings are read fresh at each launch, so no Wh_ModSettingsChanged / reload is needed, and both declared settings are actually used.
Optional improvements

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

Drop the extra reference on explorerframe.dll. GetModuleHandleExW(0, L"explorerframe.dll", &g_explorerFrameRef) bumps the module's refcount for the whole lifetime of the mod, and Wh_ModUninit then drops it. No other explorerframe-hooking mod does this (explorer-frame-classic, explorer-command-bar) — Windhawk's hook engine already deals with a hooked module being unloaded. Side effects of keeping it: the DLL stays loaded in explorer.exe even when Explorer would have released it, and the FreeLibrary at teardown can end up running explorerframe.dll's DllMain(DLL_PROCESS_DETACH) on the Windhawk engine thread.

Fall back to normal navigation when the launch can't be triggered. In BrowseObject_Hook the navigation is cancelled unconditionally:

EnsureWorkerThread();
if (g_launchEvent) {
    SetEvent(g_launchEvent);
}
return HRESULT_FROM_WIN32(ERROR_CANCELLED);

If CreateEventW/CreateThread failed inside EnsureWorkerThread (rare, but the code already logs for it), the Gallery item becomes completely inert — no navigation and no Photos. Letting the original run in that case is a one-line change:

EnsureWorkerThread();
if (!g_launchEvent || !SetEvent(g_launchEvent)) {
    return g_BrowseObject_Original(pThis, pidl, wFlags);  // couldn't launch, don't eat the navigation
}
return HRESULT_FROM_WIN32(ERROR_CANCELLED);

Check pidl before EnsureTargetPidl(). EnsureTargetPidl() is called first and pidl is only tested afterwards, so a BrowseObject(nullptr, SBSP_ABSOLUTE|...) call still triggers a shell namespace parse on the first pass. Swapping the order (if (!pidl) return g_BrowseObject_Original(...);) is free. Related nit: when SHParseDisplayName fails, nothing is cached, so it is retried on every subsequent navigation — a one-shot "tried and failed" flag would avoid that.

The SEE_MASK_ASYNCOK comment overstates the guarantee. The comment says the flag ensures "Wh_ModUninit can always complete a clean join", but the documented behavior is weaker: "in certain cases ShellExecuteEx ignores this flag and the execution is performed on the calling thread's context." The join itself is correct and must stay; it's just that it can block for however long the shell call takes. Worth softening the comment so a future reader doesn't rely on it.

Functionality notes

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

A misconfigured targetCommand fails completely silently. The navigation is cancelled before the launch outcome is known (it happens asynchronously on the worker), and SEE_MASK_FLAG_NO_UI suppresses the shell's own error UI. So if a user types a bad path or a protocol with no handler into the setting, clicking Gallery does nothing at all, with the only diagnostic in the Windhawk log — which most users never look at. Since the setting is free-form, consider either letting the shell show its error (drop SEE_MASK_FLAG_NO_UI, it's only reachable through an explicit user setting), or re-triggering the normal Gallery navigation from the worker when ShellExecuteExW fails.

Unload latency. Wh_ModUninit blocks until the worker returns, and the worker may be inside ShellExecuteExW. In practice that's a shell activation of a few hundred ms, but if the user points targetCommand at something that prompts (e.g. an executable requiring elevation), disabling the mod will appear to hang for as long as that prompt is up. Nothing obviously better to do here — the join is required for the mod to be unloadable — just something to be aware of.

targetCommand and paths with spaces. lpFile is passed straight through, so a user pointing this at C:\Program Files\Some Viewer\app.exe will hit ShellExecuteExW's ambiguous-path resolution, and wrapping it in quotes does not help (lpFile is not a command line). It may be worth saying in the $description that the value is a program path or protocol, not a full command line, and that arguments go in the separate setting.

Back/Forward and SBSP_RELATIVE. The README already documents that these bypass the redirect, which is the right call — for SBSP_NAVIGATEBACK/SBSP_NAVIGATEFORWARD the pidl is NULL, so the destination isn't knowable at this level. No change suggested; just confirming the limitation looks inherent to the hook point rather than an oversight.


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 12, 2026
@jakubix30

Copy link
Copy Markdown
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 12, 2026
@windhawk-reviewer

Copy link
Copy Markdown

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 again after 2026-08-12 17:31 UTC (in 1 hour) to get another one.

@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 12, 2026
@jakubix30

Copy link
Copy Markdown
Author

/ai-review

@windhawk-reviewer windhawk-reviewer Bot added the waiting-for-ai-review An AI review was requested and is being prepared. label Aug 12, 2026
@windhawk-reviewer

Copy link
Copy Markdown

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 again after 2026-08-12 17:31 UTC (in 2 minutes) to get another one.

@jakubix30

Copy link
Copy Markdown
Author

/ai-review

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


Overall this is in good shape — the late-load handling (kernelbase LoadLibraryExW + GetModuleHandleW re-check + Wh_ModAfterInit catch-up) matches the canonical pattern in taskbar-clock-customization.wh.cpp, HookSymbols is called at most once per module, the worker thread is a raw HANDLE joined in Wh_ModUninit, and the PIDL is freed on unload. One thing needs fixing:

The unload can hang indefinitely: ShellExecuteExW may put up modal UI on the thread that Wh_ModUninit joins with an INFINITE wait. SEE_MASK_FLAG_NO_UI is deliberately omitted so the shell shows its own error prompt, but that prompt is a modal, owner-less message box displayed synchronously on the worker thread. Meanwhile CleanupResources() does WaitForSingleObject(g_workerThread, INFINITE), and Wh_ModUninit runs on the Windhawk engine thread — so until the user finds and dismisses that box, the mod cannot be unloaded, updated or reloaded.

This is not an exotic path: the two most likely ways to hit it are the mod's own configurable setting (a typo in targetCommand → "Windows cannot find …") and a machine where the Photos app has been removed (a very common debloat) → "You'll need a new app to open this ms-photos link". In both cases the user's next move is to go to Windhawk and change the setting or disable the mod, which is exactly when the hang happens — and the dialog has no owner window, so it can easily be sitting behind Explorer where the user never sees it.

Fix: set SEE_MASK_FLAG_NO_UI and report failures through the log instead, so nothing on the joined thread can block on user input:

sei.fMask = SEE_MASK_ASYNCOK | SEE_MASK_FLAG_NO_UI;
...
if (!ShellExecuteExW(&sei)) {
    Wh_Log(L"ShellExecuteExW(%s) failed: %u", target, GetLastError());
}

This is what other mods that shell-execute do — e.g. settings-to-control-panel.wh.cpp, hotcorner-hotkeys.wh.cpp, explorer-command-bar.wh.cpp. Joining the worker in Wh_ModUninit is correct and must stay — the point is to make sure that join can't be held hostage by a dialog.

Optional improvements

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

  • BrowseObject_Hook doesn't check that the worker thread actually exists. If CreateThread fails (or g_stopEvent creation fails while g_launchEvent succeeds), EnsureWorkerThread() still latches g_workerInitOnce as done and never retries, SetEvent(g_launchEvent) succeeds, and the hook returns ERROR_CANCELLED — so Gallery navigation is blocked with nothing ever launching. Rare, but the fallback already exists, so it's a one-word fix:

    if (!g_workerThread || !g_launchEvent || !SetEvent(g_launchEvent)) {
        Wh_Log(L"Failed to signal launch event, falling back to original navigation.");
        return g_BrowseObject_Original(pThis, pidl, wFlags);
    }
  • The LoadLibraryExW hook is installed even when it can never do anything. In Wh_ModInit, if explorerframe.dll is already loaded but HookExplorerFrame fails (e.g. symbols unavailable), g_explorerFrameHooked is already latched true, so LoadLibraryExW_Hook's !g_explorerFrameHooked.load() check is permanently false — yet the hook stays installed on a very hot API for the life of the process. Gate it on the module simply not being present:

    HMODULE hEF = GetModuleHandleW(L"explorerframe.dll");
    if (hEF) {
        HookExplorerFrame(hEF, false);
    } else {
        // hook kernelbase LoadLibraryExW
    }
  • SEE_MASK_ASYNCOK doesn't buy anything here — the launch already runs on your own dedicated worker thread, so there's no UI thread to keep responsive. It's also the flag whose documented counterpart (SEE_MASK_NOASYNC) is recommended precisely when "the thread or process that called ShellExecuteEx may exit before the operation completes", which is what happens if the mod is unloaded right after a launch is triggered. Consider just dropping it.

  • The hardcoded L"ms-photos:" fallback in the worker duplicates the settings default. Harmless, but it means changing the default requires editing two places.

  • The Polish $description for targetArguments drops the "(only applies when the command is an executable, not a protocol)" caveat that the English one has — worth keeping them in sync, since that caveat is the non-obvious part.

Functionality notes

Non-critical observations about the feature behavior itself.

  • A failed launch leaves Gallery unreachable with no feedback. The hook cancels the navigation before it knows whether the launch will succeed, so if targetCommand is wrong, clicking Gallery does nothing at all — and once SEE_MASK_FLAG_NO_UI is set (above), the only trace is the Windhawk log. Worth calling out in the README or in the targetCommand $description, so users who point it at a custom viewer know where to look when nothing happens.

  • Rapid repeat triggers coalesce. g_launchEvent is auto-reset and carries no payload, so two Gallery clicks in quick succession (or from two Explorer windows) can collapse into a single launch. That's almost certainly the behavior you want for a launcher — just noting it's a consequence of the design rather than something to fix.

  • The Back/Forward bypass documented in the README comes from the SBSP_NAVIGATEBACK | SBSP_NAVIGATEFORWARD filter at the top of the hook. That's a reasonable trade-off (history navigations don't always carry a usable absolute PIDL), and you've documented it — no change needed.

  • No overlap with existing mods as far as I can tell: hide-home-gallery-explorer.wh.cpp hides the nav-pane entry rather than redirecting it, and add-virtual-folders-to-nav-top.wh.cpp only uses the same Gallery CLSID for an unrelated purpose. The symbol strings match the ones already proven in file-explorer-content-animation.wh.cpp, which is a good sign for resolution across builds.


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.

Updated descriptions for clarity and added notes regarding potential failures. Adjusted event signaling logic to ensure proper thread handling during app launch.
@jakubix30

Copy link
Copy Markdown
Author

/ai-review

@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 overall shape is good — narrow @include, a real symbol hook instead of UI poking, no persistent system changes, and the teardown is genuinely correct (worker thread signalled and joined, handles closed, PIDL freed, nothing left running past Wh_ModUninit). That last part is the most commonly botched thing in submissions, so it's worth calling out. A few things to fix:

1. Wh_ModInit ignores the hook result and always returns TRUE.

HookExplorerFrame sets the g_explorerFrameHooked latch before calling HookSymbols and never clears it on failure, so if symbol resolution fails (symbols unavailable, download declined, PDB not yet cached) nothing ever retries and the mod stays resident doing nothing — the only feedback is a log line the user has to go looking for. The convention is to return FALSE: Windhawk surfaces the failure and reloads the mod on the next settings change, giving it another chance.

HMODULE hEF = GetModuleHandleW(L"explorerframe.dll");
if (hEF) {
    if (!HookExplorerFrame(hEF, false)) {
        return FALSE;
    }
} else {
    // ... LoadLibraryExW hook
}

Compare mods/pinned-items-double-click.wh.cpp#L394-L398, which returns FALSE when HookTaskbarSymbols() fails.

2. The navigation is cancelled before the launch is known to work, and the SEE_MASK_FLAG_NO_UI comment overstates what the flag does.

The comment on lines 147-148 says SEE_MASK_FLAG_NO_UI "ensures modal dialogs do not spawn and block the thread, guaranteeing Wh_ModUninit can cleanly complete the join." That's not what it does — per MSDN it only suppresses the error message box. It doesn't prevent the shell's "How do you want to open this?" / Store prompt when the configured target has no registered handler, and it has no bearing on how long the call takes. With SEE_MASK_NOASYNC set (correctly, since the worker can exit right after), ShellExecuteExW blocks until the whole operation finishes, and Wh_ModUninitCleanupResourcesWaitForSingleObject(g_workerThread, INFINITE) inherits that with no bound. So disabling or updating the mod while that prompt is up hangs.

This isn't hypothetical for the default value either: ms-photos: has no handler on systems where the Photos app has been removed, which is a fairly common state for the kind of user who installs this mod.

The same root cause produces the behaviour you documented in the setting description ("If the target is invalid, clicking Gallery will silently fail"): the hook has already returned HRESULT_FROM_WIN32(ERROR_CANCELLED) by the time the worker discovers the target doesn't resolve, so the user gets nothing at all — no Gallery, no Photos, no error.

Both go away if you resolve the target before cancelling. Check it once when it's first needed (AssocQueryStringW with ASSOCF_IS_PROTOCOL for a scheme, PathFileExistsW for a path), cache the result, and fall through to g_BrowseObject_Original(pThis, pidl, wFlags) when it doesn't resolve — then a misconfigured command degrades to normal Gallery navigation instead of a dead click. At minimum, correct the comment so the next reader doesn't rely on a guarantee the flag doesn't provide.

3. Please confirm the mod works after an explorer.exe restart, not just when enabled mid-session.

Windhawk injects before the process starts executing, so on a fresh Explorer start explorerframe.dll is normally not loaded yet — both Wh_ModInit and Wh_ModAfterInit take the "not loaded" branch and everything then depends on the LoadLibraryExW hook firing. That only happens if the module is brought in via LoadLibraryExW; anything the loader pulls in as a static import goes straight to LdrLoadDll and never reaches the hook. Other merged mods rely on the same path so it very likely works, but it's the one scenario worth testing explicitly (restart Explorer with the mod enabled, open a new window, click Gallery) — the maintainer asks about this routinely.

Optional improvements

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

  • LoadLibraryExW_Hook reacts to GetModuleHandleW(L"explorerframe.dll") rather than to what was actually loaded. Your comment explains the intent (catch indirect loads too), and that's a reasonable trade-off, but note the downside: the loader inserts a module into the module list before running its DllMain, so if some DLL calls LoadLibraryExW from its own DllMain while ExplorerFrame is still mid-load on that thread, you'll run HookSymbols + Wh_ApplyHookOperations — file I/O and thread suspension — with the loader lock held. Matching on the loaded module's name avoids that; see mods/add-virtual-folders-to-nav-top.wh.cpp#L2437-L2469.

  • targetArguments's description says it "only applies when the command is an executable, not a protocol", but the code passes it as lpParameters unconditionally. Either skip it when the target looks like a scheme, or drop the caveat.

  • HookExplorerFrame returns true when the latch was already set, regardless of whether the earlier attempt actually succeeded — so the return value means "someone tried" rather than "it's hooked". That's fine for the current call sites (the Wh_ModInit call is always the first one, so its return value is accurate), but worth a comment if you act on it per item 1 above.

Functionality notes

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

  • ILIsEqual is a binary comparison of the two ID lists. It should be fine for a CLSID folder, but if you ever run into a Gallery entry that isn't intercepted (reached through a different parent, or a differently-encoded PIDL), IShellFolder::CompareIDs on the desktop folder with SHCIDS_CANONICALONLY is the semantic equivalent.

  • Worth checking what the nav pane does after the hook returns ERROR_CANCELLED — whether the tree selection snaps back to the current folder or stays highlighted on Gallery while the content pane still shows the old location.

  • The back/forward limitation you documented looks inherent rather than a design choice: those calls frequently pass a NULL pidl, so there's nothing to compare against. Fine as documented.


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.

Updated descriptions for target command and arguments in comments. Improved handling of asynchronous shell execution and navigation interception.
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