Add: Gallery -> Photos App Launcher mod - #5047
Conversation
|
Thanks for the pull request! This repository uses a two-stage review: an AI review that you run yourself, followed by a human review. To get started, comment See the pull request review process for the full details. |
|
/ai-review |
|
/ai-review |
|
@jakubix30 |
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 idea (intercept at the 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:
2. Install the hook with This removes the worker thread, the
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 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 SHGetNameFromIDList(reinterpret_cast<PCIDLIST_ABSOLUTE>(pidl), SIGDN_DESKTOPABSOLUTEPARSING, &pszName);
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 nawigujemyIf the Photos app isn't installed (or the user typo'd if ((INT_PTR)ShellExecuteW(...) <= 32) {
return g_BrowseObject_Original(pThis, pidl, wFlags);
}Separately, 5. User-facing strings must default to English. The settings block and several log strings are Polish only: 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 ( 6. Drop the Windhawk already provides a per-mod logging toggle in the mod's settings UI, and 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. |
|
/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 idea is sound and the hook point is well chosen — 1. The late-load path never actually installs the hook — Windhawk applies registered hook operations automatically only when
Have 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 2. The worker thread never initializes COM before
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.
Resolve the target once (in // 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 4. Drop the ExplorerPatcher framing from the mod name and description. The name is 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. |
|
/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 overall approach is sound — hooking 1. The worker thread wakes up twice a second, forever, in every Explorer process
while (!g_stopWorker) {
DWORD waitRes = WaitForSingleObject(g_launchEvent, 500);That is a polling loop: it keeps a thread in every While you're there: the thread initializes an STA ( 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 Related edge case in the current code: if 2.
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 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:
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 Optional improvements
Minor polish — none of this affects users in normal operation, 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. |
|
/ai-review |
|
/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. 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:
Optional improvements
Minor polish — none of this affects users, so it's your call. Drop the extra reference on Fall back to normal navigation when the launch can't be triggered. In EnsureWorkerThread();
if (g_launchEvent) {
SetEvent(g_launchEvent);
}
return HRESULT_FROM_WIN32(ERROR_CANCELLED);If 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 The Functionality notes
Non-critical observations and ideas about the feature behavior itself. A misconfigured Unload latency.
Back/Forward and Next steps:
See the review process for details. |
|
/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 |
|
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. Overall this is in good shape — the late-load handling (kernelbase The unload can hang indefinitely: This is not an exotic path: the two most likely ways to hit it are the mod's own configurable setting (a typo in Fix: set 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 Optional improvements
Minor polish — none of this affects users in normal operation, so it's your call.
Functionality notes
Non-critical observations about the feature behavior itself.
Next steps:
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.
|
/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 overall shape is good — narrow 1.
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 2. The navigation is cancelled before the launch is known to work, and the The comment on lines 147-148 says This isn't hypothetical for the default value either: 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 Both go away if you resolve the target before cancelling. Check it once when it's first needed ( 3. Please confirm the mod works after an Windhawk injects before the process starts executing, so on a fresh Explorer start 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. |
Updated descriptions for target command and arguments in comments. Improved handling of asynchronous shell execution and navigation interception.
Fix missing newline at end of file.
Changelog
If this pull request updates an existing mod, describe the changes below:
Mod authorship
If this pull request introduces a new mod, please complete the section below.
This mod was created by:
Please select the options that best apply. Your selection does not affect the acceptance criteria, but it helps reviewers understand the context of the code and provide relevant feedback.