Skip to content

UPDATE: Added Control Panel links restoration and enhanced the interface#4823

Open
babamohammed2022 wants to merge 2 commits into
ramensoftware:mainfrom
babamohammed2022:patch-12
Open

UPDATE: Added Control Panel links restoration and enhanced the interface#4823
babamohammed2022 wants to merge 2 commits into
ramensoftware:mainfrom
babamohammed2022:patch-12

Conversation

@babamohammed2022

@babamohammed2022 babamohammed2022 commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Changelog

If this pull request updates an existing mod, describe the changes below:

  • Added Ethernet support
  • Restored "Connect to a network" and "HomeGroup/sharing" links in Network Center Control Page like in Windows 7
  • Enhanced the interface to make it more similar to the original Windows 7 one
  • Better connection timeouts and auth error handling
  • Enhanced the stability

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.

@m417z

m417z commented Jul 18, 2026

Copy link
Copy Markdown
Member

Submission review

Note: 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.

Personal note: If you find yourself having to embed binary files, it might be a sign that it's not a good fit for a mod, and should be a standalone tool instead. I didn't read the generated review below past item 1.


1. Embedding a compiled PE and writing it to %TEMP% is the central concern. The mod embeds a ~6 KB compiled DLL as a hex byte array (kIconsDll[]), writes it to %TEMP%\Win7NetworkCenterLinks\win7ncl_icons_<pid>.dll on init, and has DirectUI LoadLibrary it (via a process‑wide LoadLibraryExW redirect) just to supply two 24×24 icons. Several problems with this:

  • Not auditable. An opaque compiled binary blob can't be reviewed from the source. Windhawk mods are meant to be source‑auditable, and there's no way for a reviewer (or a user) to confirm what that PE actually contains or does when loaded.
  • Loading an executable from a user‑writable location. %TEMP% is user‑writable; writing a DLL there and loading it is a pattern worth avoiding on principle, even though the redirect to a fixed path + FileMatchesEmbedded verification and the per‑user nature of %TEMP% keep the practical hijack risk low here.
  • Stale files accumulate. CleanupIconsDll() only runs from Wh_ModUninit, which is not called when the host process terminates (Explorer restart, sign‑out, reboot, crash). Since the temp file is named per‑PID, every such exit leaves a win7ncl_icons_<oldpid>.dll behind, and they pile up in %TEMP% over time.
  • It's cosmetic. There's already a working stock‑icon fallback (icon(22,…) / icon(27,…) from netcenter.dll when !g_dllReady), so all of the above buys only icon fidelity.

My recommendation is to reconsider whether the custom icons justify shipping + writing + loading an opaque PE at all — the stock fallback already renders the links. If you do keep it, at minimum: sweep and delete any leftover win7ncl_icons_*.dll on init (since uninit won't run at process exit), and do none of this when the feature is disabled (next point).

2. The DLL is written and all hooks are installed even when restoreClassicNetworkCenterLinks is off. Init() calls EnsureIconsDll() and HookAll() unconditionally, and SettingsChanged() calls EnsureIconsDll() unconditionally — the setting only gates Patch(). So a user who disables the feature still gets the temp DLL written to disk on every process start (and on every settings change). At least gate the disk write on the setting:

static bool Init() {
    bool enabled = Wh_GetIntSetting(L"restoreClassicNetworkCenterLinks") != 0;
    g_addConnect = g_addHomegroup = enabled;
    if (enabled) EnsureIconsDll();   // don't touch the disk when disabled
    if (!HookAll()) { CleanupIconsDll(); return false; }
    return true;
}

(Keeping the DUI hook installed while disabled is defensible so the toggle works live, but the file write and the redundant dui70.dll force‑load shouldn't happen for a feature that's off.)

Optional improvements

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

  • SetXML_Hook is a no‑op installed only to capture the trampoline. It forwards SetXML_Orig(t, x, r, h) unchanged and exists solely so SetXMLFromResource_Hook can call the original. SetXML is a public member (?SetXML@…@@QEAA…), so you can just GetProcAddress it and cast to SetXML_t, and drop the hook — that avoids adding your trampoline to a hot DirectUI path (SetXML runs for lots of shell UI in explorer.exe) for no functional reason.

  • debugEthernet is dead scaffolding. The setting is commented out (it lives in a /* */ block after ==/WindhawkModSettings==), so Wh_GetStringSetting(L"debugEthernet") always returns L"" and g_Settings.debugEthernet is permanently 0. That makes the debug branches in UpdateEthernetStatus, the debugEthernet != 2 guards, and the oldDebug tracking in Wh_ModSettingsChanged all unreachable. Since debugEthernet != 2 is always true, showWifiList reduces to g_NetworkCount > 0. Consider removing the whole debugEthernet plumbing (it reads like a leftover AI/testing artifact).

  • Raw Wh_SetFunctionHook + void* casts in HookAll() — the modern idiom is WindhawkUtils::SetFunctionHook() for type safety (0 uses currently vs. 3 raw calls).

  • In control.exe the flyout‑only init runs needlessly. Wh_ModInit runs LoadSystemIcons(), InitGdiPlusRendering(), DarkContextMenu::Init(), CreateMutexW, InitializeCriticalSection before the IsExplorerProcess() early‑return, even though only the Network Center hooks are relevant in control.exe. Cheap to move those after the gate.

  • InitGdiPlusRendering() uses the bare‑name LoadLibraryW(L"gdiplus.dll") (line ~1279) while the rest of the file already uses the safe LoadLibraryExW(L"gdiplus.dll", NULL, LOAD_LIBRARY_SEARCH_SYSTEM32) form (line ~1106). gdiplus.dll is not a KnownDLL; the hijack surface is negligible since the mod only targets explorer.exe/control.exe (both run from System32), but it's a one‑line consistency fix.

  • -loleaut32 appears unused as an import — SysFreeString is resolved via GetProcAddress and there's no other oleaut32 symbol linked directly. Safe to drop from @compilerOptions.

Functionality notes

Non‑critical observations about the feature behavior itself.

  • The LoadLibraryExW hook fires on every DLL load in explorer.exe (explorer loads a lot of DLLs — shell extensions, thumbnail/property handlers, etc.), running IsOurDll() string compares each time. The cost per call is small and, given the library(win7ncl_icons.dll) redirect approach, there's no obviously cleaner interception point — just noting it as an inherent cost of this design (another reason to prefer the stock‑icon fallback).

  • Ethernet detection runs GetAdaptersAddresses + a full NLM COM network/connection enumeration on every refresh tick (default 3 s). That's fine functionally (it's on the flyout's COM‑initialized hotkey thread, and g_pNLM is created/released consistently there), just heavier than the pure Wi‑Fi path — acceptable while the flyout is open.

@babamohammed2022

Copy link
Copy Markdown
Contributor Author

Submission review

Note: 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.

Personal note: If you find yourself having to embed binary files, it might be a sign that it's not a good fit for a mod, and should be a standalone tool instead. I didn't read the generated review below past item 1.

1. Embedding a compiled PE and writing it to %TEMP% is the central concern. The mod embeds a ~6 KB compiled DLL as a hex byte array (kIconsDll[]), writes it to %TEMP%\Win7NetworkCenterLinks\win7ncl_icons_<pid>.dll on init, and has DirectUI LoadLibrary it (via a process‑wide LoadLibraryExW redirect) just to supply two 24×24 icons. Several problems with this:

  • Not auditable. An opaque compiled binary blob can't be reviewed from the source. Windhawk mods are meant to be source‑auditable, and there's no way for a reviewer (or a user) to confirm what that PE actually contains or does when loaded.
  • Loading an executable from a user‑writable location. %TEMP% is user‑writable; writing a DLL there and loading it is a pattern worth avoiding on principle, even though the redirect to a fixed path + FileMatchesEmbedded verification and the per‑user nature of %TEMP% keep the practical hijack risk low here.
  • Stale files accumulate. CleanupIconsDll() only runs from Wh_ModUninit, which is not called when the host process terminates (Explorer restart, sign‑out, reboot, crash). Since the temp file is named per‑PID, every such exit leaves a win7ncl_icons_<oldpid>.dll behind, and they pile up in %TEMP% over time.
  • It's cosmetic. There's already a working stock‑icon fallback (icon(22,…) / icon(27,…) from netcenter.dll when !g_dllReady), so all of the above buys only icon fidelity.

My recommendation is to reconsider whether the custom icons justify shipping + writing + loading an opaque PE at all — the stock fallback already renders the links. If you do keep it, at minimum: sweep and delete any leftover win7ncl_icons_*.dll on init (since uninit won't run at process exit), and do none of this when the feature is disabled (next point).

2. The DLL is written and all hooks are installed even when restoreClassicNetworkCenterLinks is off. Init() calls EnsureIconsDll() and HookAll() unconditionally, and SettingsChanged() calls EnsureIconsDll() unconditionally — the setting only gates Patch(). So a user who disables the feature still gets the temp DLL written to disk on every process start (and on every settings change). At least gate the disk write on the setting:

static bool Init() {
    bool enabled = Wh_GetIntSetting(L"restoreClassicNetworkCenterLinks") != 0;
    g_addConnect = g_addHomegroup = enabled;
    if (enabled) EnsureIconsDll();   // don't touch the disk when disabled
    if (!HookAll()) { CleanupIconsDll(); return false; }
    return true;
}

(Keeping the DUI hook installed while disabled is defensible so the toggle works live, but the file write and the redundant dui70.dll force‑load shouldn't happen for a feature that's off.)

Optional improvements

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

  • SetXML_Hook is a no‑op installed only to capture the trampoline. It forwards SetXML_Orig(t, x, r, h) unchanged and exists solely so SetXMLFromResource_Hook can call the original. SetXML is a public member (?SetXML@…@@QEAA…), so you can just GetProcAddress it and cast to SetXML_t, and drop the hook — that avoids adding your trampoline to a hot DirectUI path (SetXML runs for lots of shell UI in explorer.exe) for no functional reason.
  • debugEthernet is dead scaffolding. The setting is commented out (it lives in a /* */ block after ==/WindhawkModSettings==), so Wh_GetStringSetting(L"debugEthernet") always returns L"" and g_Settings.debugEthernet is permanently 0. That makes the debug branches in UpdateEthernetStatus, the debugEthernet != 2 guards, and the oldDebug tracking in Wh_ModSettingsChanged all unreachable. Since debugEthernet != 2 is always true, showWifiList reduces to g_NetworkCount > 0. Consider removing the whole debugEthernet plumbing (it reads like a leftover AI/testing artifact).
  • Raw Wh_SetFunctionHook + void* casts in HookAll() — the modern idiom is WindhawkUtils::SetFunctionHook() for type safety (0 uses currently vs. 3 raw calls).
  • In control.exe the flyout‑only init runs needlessly. Wh_ModInit runs LoadSystemIcons(), InitGdiPlusRendering(), DarkContextMenu::Init(), CreateMutexW, InitializeCriticalSection before the IsExplorerProcess() early‑return, even though only the Network Center hooks are relevant in control.exe. Cheap to move those after the gate.
  • InitGdiPlusRendering() uses the bare‑name LoadLibraryW(L"gdiplus.dll") (line ~1279) while the rest of the file already uses the safe LoadLibraryExW(L"gdiplus.dll", NULL, LOAD_LIBRARY_SEARCH_SYSTEM32) form (line ~1106). gdiplus.dll is not a KnownDLL; the hijack surface is negligible since the mod only targets explorer.exe/control.exe (both run from System32), but it's a one‑line consistency fix.
  • -loleaut32 appears unused as an import — SysFreeString is resolved via GetProcAddress and there's no other oleaut32 symbol linked directly. Safe to drop from @compilerOptions.

Functionality notes

You are right, I've removed the DLL and replaced it with base64, initially I used the DLL because it worked decently compared to the base64 but I've enhanced the base64 implementation and made it more robust and I've also tried to address the other reported issues

@babamohammed2022

Copy link
Copy Markdown
Contributor Author

Submission review

Note: 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.

Personal note: If you find yourself having to embed binary files, it might be a sign that it's not a good fit for a mod, and should be a standalone tool instead. I didn't read the generated review below past item 1.

1. Embedding a compiled PE and writing it to %TEMP% is the central concern. The mod embeds a ~6 KB compiled DLL as a hex byte array (kIconsDll[]), writes it to %TEMP%\Win7NetworkCenterLinks\win7ncl_icons_<pid>.dll on init, and has DirectUI LoadLibrary it (via a process‑wide LoadLibraryExW redirect) just to supply two 24×24 icons. Several problems with this:

  • Not auditable. An opaque compiled binary blob can't be reviewed from the source. Windhawk mods are meant to be source‑auditable, and there's no way for a reviewer (or a user) to confirm what that PE actually contains or does when loaded.
  • Loading an executable from a user‑writable location. %TEMP% is user‑writable; writing a DLL there and loading it is a pattern worth avoiding on principle, even though the redirect to a fixed path + FileMatchesEmbedded verification and the per‑user nature of %TEMP% keep the practical hijack risk low here.
  • Stale files accumulate. CleanupIconsDll() only runs from Wh_ModUninit, which is not called when the host process terminates (Explorer restart, sign‑out, reboot, crash). Since the temp file is named per‑PID, every such exit leaves a win7ncl_icons_<oldpid>.dll behind, and they pile up in %TEMP% over time.
  • It's cosmetic. There's already a working stock‑icon fallback (icon(22,…) / icon(27,…) from netcenter.dll when !g_dllReady), so all of the above buys only icon fidelity.

My recommendation is to reconsider whether the custom icons justify shipping + writing + loading an opaque PE at all — the stock fallback already renders the links. If you do keep it, at minimum: sweep and delete any leftover win7ncl_icons_*.dll on init (since uninit won't run at process exit), and do none of this when the feature is disabled (next point).

2. The DLL is written and all hooks are installed even when restoreClassicNetworkCenterLinks is off. Init() calls EnsureIconsDll() and HookAll() unconditionally, and SettingsChanged() calls EnsureIconsDll() unconditionally — the setting only gates Patch(). So a user who disables the feature still gets the temp DLL written to disk on every process start (and on every settings change). At least gate the disk write on the setting:

static bool Init() {
    bool enabled = Wh_GetIntSetting(L"restoreClassicNetworkCenterLinks") != 0;
    g_addConnect = g_addHomegroup = enabled;
    if (enabled) EnsureIconsDll();   // don't touch the disk when disabled
    if (!HookAll()) { CleanupIconsDll(); return false; }
    return true;
}

(Keeping the DUI hook installed while disabled is defensible so the toggle works live, but the file write and the redundant dui70.dll force‑load shouldn't happen for a feature that's off.)

Optional improvements

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

  • SetXML_Hook is a no‑op installed only to capture the trampoline. It forwards SetXML_Orig(t, x, r, h) unchanged and exists solely so SetXMLFromResource_Hook can call the original. SetXML is a public member (?SetXML@…@@QEAA…), so you can just GetProcAddress it and cast to SetXML_t, and drop the hook — that avoids adding your trampoline to a hot DirectUI path (SetXML runs for lots of shell UI in explorer.exe) for no functional reason.
  • debugEthernet is dead scaffolding. The setting is commented out (it lives in a /* */ block after ==/WindhawkModSettings==), so Wh_GetStringSetting(L"debugEthernet") always returns L"" and g_Settings.debugEthernet is permanently 0. That makes the debug branches in UpdateEthernetStatus, the debugEthernet != 2 guards, and the oldDebug tracking in Wh_ModSettingsChanged all unreachable. Since debugEthernet != 2 is always true, showWifiList reduces to g_NetworkCount > 0. Consider removing the whole debugEthernet plumbing (it reads like a leftover AI/testing artifact).
  • Raw Wh_SetFunctionHook + void* casts in HookAll() — the modern idiom is WindhawkUtils::SetFunctionHook() for type safety (0 uses currently vs. 3 raw calls).
  • In control.exe the flyout‑only init runs needlessly. Wh_ModInit runs LoadSystemIcons(), InitGdiPlusRendering(), DarkContextMenu::Init(), CreateMutexW, InitializeCriticalSection before the IsExplorerProcess() early‑return, even though only the Network Center hooks are relevant in control.exe. Cheap to move those after the gate.
  • InitGdiPlusRendering() uses the bare‑name LoadLibraryW(L"gdiplus.dll") (line ~1279) while the rest of the file already uses the safe LoadLibraryExW(L"gdiplus.dll", NULL, LOAD_LIBRARY_SEARCH_SYSTEM32) form (line ~1106). gdiplus.dll is not a KnownDLL; the hijack surface is negligible since the mod only targets explorer.exe/control.exe (both run from System32), but it's a one‑line consistency fix.
  • -loleaut32 appears unused as an import — SysFreeString is resolved via GetProcAddress and there's no other oleaut32 symbol linked directly. Safe to drop from @compilerOptions.

Functionality notes

If possible, let me know if the issues have been addressed properly now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants