Redirect Settings to Control Panel enhancement update#4727
Conversation
|
After Explorer's restart, the Network redirect still stops working. |
Does the rest of the mod (redirects, sound context menu redirect) work? So that I can understand what is fixed and what isn't |
|
The other, you mean sound? Works for now. |
…r explorer restart
|
After Explorer's restart the network redirect still does not work. |
|
You do not need fallback, you need to make the existing method to not depend on whether the mod was activated when explorer was running or explorer was started later. In the latest version the redirect still does not work after explorer's restart. |
OK, thanks for the suggestion. I will take more time to study a more fitting solution along with fixing the validation issue. Since it works on my setup (Windows 11 24H2 with ExplorerPatcher) it thought that it was fixed. |
Let me know if it works now. I've tested on Windows 11 24H2 with ExplorerPatcher (as mentioned above) and it seems to work. |
|
Network redirect does not work after Explorer's restart. |
In this case since it works on my setup I need the logs (if present) to see what happens because perhaps something should be logged at least, if possible please send them so that I avoid random fix attempts |
|
There are no logs when clicking on the network icon after Explorer's restart. Zero. Your program does not know about the click. |
|
Here is the only log after restart: |
OK thank you. This confirms that it is partially installed and the problem is specific to the context menu option. I will try to fix this specific part then |
I've added a function that detects the system tray after explorer's restart which should make it more robust now. Please let me know if this fixes the issue or if it's the same |
|
@Anixx I have 2 questions:
|
|
Nothing changed. After Explorer's restart the network redirect does not work. You can include whatever parts you want, it is under free license. |
Thanks for the permission. However, since I've tried multiple approaches to make it more robust but without result on your setup (Windows 10 20H1 taskbar on Windows 11 23H2, according to WinClassic forums if I am not wrong), I've noted it as a limitation for now until I find a solution to the problem. In the meantime, if you have any other reports feel free to tell them |
…od and update README
Where did u get it? I am using the taskbar from 23H2. If you mod can do something when it gets enabled, it should be able to do the same after it injects Explorer. |
|
I don't think it is a good idea to just double functionality of the mods and include the functions of another unrelated mod. Otherwise you just can combine any mods you like to get a huge giant mod of your own. Why you don't want just install my mod? If you have issues, I can fix them. |
I'm sorry, but it seems like the sound redirect phrasing has been repeated multiple times. Could you clarify? While I acknowledge that you're using the latest version, I can't understand what is working and what isn't. |
|
sound redirect works, network redirect stops working. |
I understand, in this case I think that this is the best that could be done for now until I find a better solution for another future update. Thanks for testing and if you find any other issues let me know |
|
@m417z Hi, if possible could you let me know if the code requires other fixes? So that I can improve it if necessary |
Submission reviewNote: This review was done by Claude, and then refined manually. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding. Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them. 1. The two new features are gated behind settings that don't exist, so they never run. g_settings.comActivationRedirect = Wh_GetIntSetting(L"ComActivationRedirect") != 0; // line 229
g_settings.legacyNameMappingFix = Wh_GetIntSetting(L"LegacyNameMappingFix") != 0; // line 230but neither 2. The COM vtable patch is never undone → crash after the mod is disabled/updated. If you must keep it, prefer using Also 3. 4. Re-hooking the same modules repeatedly. Optional improvements
Minor polish — none of this affects users, so it's your call.
Functionality notes
Non-critical observations about the feature behavior itself.
|
Thanks for the review. I've tried to address the reported issues and I've updated the README of the mod. Let me know if other fixes are required. |
Submission reviewNote: This review was done by Claude, and then refined manually. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding. Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them. The main problem is that the central fixes in this update never actually take effect, because the hooks are registered from a background thread with no follow-up apply. 1. The new hooks are registered after In this PR, all three of the new/relocated hook installers are called only from
None of these are ever applied, so the "Fixed menu items appearing blank" fix, the COM-activation redirect, and the legacy-name-mapping fix (i.e. essentially the entire changelog) silently do nothing. Note that in the previous version Fix: call 2. Optional improvements
Minor polish — none of this affects users, so it's your call.
Functionality notes
Non-critical observations about the feature behavior itself (assuming issue #1 is fixed so the hooks actually run).
|
Thanks for the review, now all issues should be solved. Let me know if other fixes are required |
| } | ||
|
|
||
| if (hookCount > 0) { | ||
| if (WindhawkUtils::HookSymbols(hShell32, shell32_dll_hooks, hookCount)) { |
There was a problem hiding this comment.
The conditional hooking in this function will break symbol caching. Avoid this, and even better, avoid calling Wh_ApplyHookOperations. Just set all hook that might be needed, and if nothing should be done in the hook, just call the original function.
Submission reviewNote: This review was done by Claude, and then refined manually. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding. Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them. 1. The BOOL WINAPI TrackPopupMenuEx_Hook(...) {
return CommonTrackPopupMenuEx_Hook(..., g_origTrackPopupMenuEx, L"TRAY-HOOK"); // extra frame
}
2. Disabling
So a user who toggles either setting off (e.g. to troubleshoot) sees no change. Gate the bodies on the current setting and fall through to the original when disabled, e.g.: bool COpenControlPanel__MapLegacyName_hook(...) {
if (!g_settings.legacyNameMappingFix)
return COpenControlPanel__MapLegacyName_orig(pThis, pszLegacyName, pszNewName, uLen, nameChanged);
...
}3. if (pszNewName && uLen > 0) *pszNewName = L'\0';Optional improvements
Minor polish — none of this affects users in the common case, so it's your call.
Functionality notes
Non-critical observations about the feature behavior itself.
|
Thanks for the additional review. Is the code correct now? |
Submission reviewNote: This review was done by Claude, and then refined manually. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding. Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them. This is an update to your own mod, so there's no overlap/duplication concern. The main thing to fix is a deadlock that this PR newly introduces via the tray-subclass mutex, plus a follow-up to the maintainer's earlier hook comment. 1.
static void SetupTraySubclass() {
std::lock_guard<std::mutex> lk(g_traySubclassMutex); // lock held...
...
if (WindhawkUtils::SetWindowSubclassFromAnyThread(hToolbar, ...)) { // ...across a cross-thread SendMessageAt the same time, HWND WINAPI CreateWindowExW_Hook(...) {
HWND hwnd = CreateWindowExW_Original(...);
{
std::lock_guard<std::mutex> lk(g_traySubclassMutex); // victim
trayToolbarMissing = (g_hTrayToolbar == nullptr);
}Classic lock-and- Note the base version (10.0.20) did not hold a lock here at all — this is a regression introduced by the new watchdog/mutex code. Fix: copy the handle out under the lock, release it, then make the cross-thread call: static void SetupTraySubclass() {
HWND hToolbar;
{
std::lock_guard<std::mutex> lk(g_traySubclassMutex);
if (g_hTrayToolbar && IsWindow(g_hTrayToolbar)) return;
g_hTrayToolbar = nullptr;
hToolbar = FindTrayToolbar();
}
if (!hToolbar || !InitTrayDllInfo()) return;
BOOL ok = WindhawkUtils::SetWindowSubclassFromAnyThread(hToolbar, TrayToolbarSubclassProc, 0);
if (ok) {
std::lock_guard<std::mutex> lk(g_traySubclassMutex);
g_hTrayToolbar = hToolbar;
}
}
static void RemoveTraySubclass() {
HWND h;
{
std::lock_guard<std::mutex> lk(g_traySubclassMutex);
h = g_hTrayToolbar;
g_hTrayToolbar = nullptr;
}
if (h) WindhawkUtils::RemoveWindowSubclassFromAnyThread(h, TrayToolbarSubclassProc);
}2. This is the still-unaddressed half of the reviewer's earlier comment ("if nothing should be done in the hook, just call the original function"). The hook is installed unconditionally (good, for cache stability), but when static bool ICMH_CALL ICMH_CAODTM_hook(HMENU, HWND) {
if (!g_settings.enableRedirects || !g_settings.redirectSystemTray) return true; // not a true no-op
return false;
}So by default the mod still forces static bool ICMH_CALL ICMH_hook_pnidui(HMENU m, HWND w) {
if (!g_settings.enableRedirects || !g_settings.redirectSystemTray)
return g_icmhOrig_pnidui(m, w);
return false;
}
// ...same for SndVolSSO and Shell32Devices, each calling its own orig.This also makes the currently-unused Optional improvements
Minor polish — none of this affects users, so it's your call.
Functionality notes
Non-critical observations about the feature behavior itself.
|
Thanks for reporting the issues. If possible, let me know if this new version is more stable and without regression |
Submission reviewNote: This review was done by Claude, and then refined manually. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding. Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them. 1. The
using ICMH_HCM_t = bool(ICMH_CALL*)(HMENU, UINT);
static bool ICMH_CALL ICMH_hook_Shell32Devices(HMENU m, UINT u) {
if (!g_settings.enableRedirects || !g_settings.redirectSystemTray)
return g_icmhOrig_Shell32Devices ? g_icmhOrig_Shell32Devices(m, u) : true;
return false;
}So the prototype is wrong:
Note this fires with the tray feature off (the default You already handled this correctly for using ICMH_HCM_t = bool(ICMH_CALL*)(void* /*pThis*/, HMENU, UINT);
static bool ICMH_CALL ICMH_hook_Shell32Devices(void* pThis, HMENU m, UINT u) {
if (!g_settings.enableRedirects || !g_settings.redirectSystemTray)
return g_icmhOrig_Shell32Devices ? g_icmhOrig_Shell32Devices(pThis, m, u) : true;
return false;
}The symbol string stays as-is ( Human note: I wanted to verify this manually, but the symbol is missing on my Win11 shell32.dll. Which Windows version is it for? 2. The 32-bit builds have several calling-convention problems. The mod has no
The simplest fix, given the mod is only tested on 64-bit, is to add Human note: I haven't verified it. Have you tested it on 32-bit? |
I'm sorry, but I currently do not have a 32 bit system to test on so I can't test it directly. If I can, I'll try on 32 bit systems too but I'm not sure that I can due to my PC not being completely working. If required, I will drop 32 bit support because I do not have enough storage to test anyways thanks for the review. Please tell me what's the best option this case so that the mod can work in a more reliable way. |
|
If it's untested and is likely broken, it's better to just limit it to 64-bit. 32-bit OSes aren't common nowadays, and Windhawk will be dropping support for them soon anyway. Many mods are already limited to 64-bit. |
Thanks for telling me, in this case I will just make it compatible with 64 bit architecture and I'll try to fix the reported issues above. Thanks |
… they are rarely used
I've removed the support for 32 bit based operating systems and I've noted it as a limitation. Is the mod correct now? |
Submission reviewThe mod is large and the findings below were missed in a previous runs, but they seem like real issues worth addressing.
The setting defaults to
On Windows 11 the hook ( There's also a second inconsistency:
|
I've put ComActivationRedirect false by default and I've also removed the unnecessary parts of the code and fixed the typos. Let me know if it is correct now. |
|
@Anixx would you like to test it before we merge the update? |
|
All the same for me. |
3 participants
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.