Never Auto-Expand Explorer Tree Items v1.1.1 - #5149
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 |
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. Two of the three changes here are fine (the 1.
The actual root cause is that the Fix: only touch else if (uMsg == WM_NOTIFY)
{
auto* pNotifyHeader = reinterpret_cast<LPNMHDR>(lParam);
if (pNotifyHeader != nullptr &&
pNotifyHeader->code == TVN_ITEMEXPANDINGW)
{
HWND hTreeView = pNotifyHeader->hwndFrom;
// Confirm the sender really is the nav pane tree view before
// treating lParam as an NMTREEVIEWW - other controls are free to
// use the same numeric notification code with a different struct.
WCHAR szClassName[16];
if (GetClassNameW(hTreeView, szClassName, ARRAYSIZE(szClassName)) &&
_wcsicmp(szClassName, L"SysTreeView32") == 0 &&
IsExplorerNavigationPane(hTreeView))
{
auto* pTreeViewNotify = reinterpret_cast<LPNMTREEVIEWW>(lParam);
if (pTreeViewNotify->action == TVE_EXPAND &&
!IsUserInteractingWithTreeView(hTreeView) &&
!IsSingleRootItem(hTreeView, pTreeViewNotify->itemNew.hItem) &&
!(settings.allowTopLevelItemAutoExpansion &&
IsTopLevelItem(hTreeView, pTreeViewNotify->itemNew.hItem)))
{
return TRUE;
}
}
}
}This removes both If you still see the crash after this, please check which read actually faulted — if it's 2. The new keyboard gate silently blocks explicit user expansions
A deterministic alternative that avoids polling async key state entirely: subclass the tree view, set a flag around the 3. Consider replacing the global
The nav pane tree is easy to target directly: hook Benefits: no per-message cost outside the nav pane, the I realize this is a larger rework than a patch release; if you'd rather ship the 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 |
|
Something went wrong while preparing the AI review, so there is nothing to post this time. This is a problem on the reviewer's side, not with this pull request. 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. The 1. The real problem is that the hook reinterprets else if (uMsg == WM_NOTIFY)
{
// The tree view sends WM_NOTIFY to its parent, which for the navigation
// pane is always a NamespaceTreeControl. Checking that first means we
// never dereference lParam for some other window's notification.
WCHAR szClassName[32];
if (GetClassNameW(hWnd, szClassName, ARRAYSIZE(szClassName)) &&
_wcsicmp(szClassName, L"NamespaceTreeControl") == 0)
{
auto* pNotifyHeader = reinterpret_cast<LPNMHDR>(lParam);
if (pNotifyHeader->code == TVN_ITEMEXPANDINGW)
{
auto* pTreeViewNotify = reinterpret_cast<LPNMTREEVIEWW>(lParam);
if (pTreeViewNotify->action == TVE_EXPAND)
{
...
}
}
}
}This fixes the OneDrive crash at the source instead of papering over it, and it also makes the hook much cheaper for every unrelated 2. Consider replacing the process-wide The established pattern for this is to hook
A subclass also gives you a much better signal than If you'd rather keep this PR minimal, item 1 is the must-have and this can be a follow-up — but the two are related, and doing item 2 makes item 1 unnecessary. 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. Two things stood out: the 1. The The mod is The crash fix itself is right, but the ordering isn't needed for it. The OneDrive crash was almost certainly the else if (uMsg == WM_NOTIFY)
{
auto* pTreeViewNotify = reinterpret_cast<LPNMTREEVIEWW>(lParam);
WCHAR szClassName[32];
// Only the NMHDR part is guaranteed valid for an arbitrary WM_NOTIFY, so
// test it first, and confirm the window is the navigation pane before
// touching any NMTREEVIEW-specific field.
if (pTreeViewNotify &&
pTreeViewNotify->hdr.code == TVN_ITEMEXPANDINGW &&
GetClassNameW(hWnd, szClassName, ARRAYSIZE(szClassName)) &&
_wcsicmp(szClassName, L"NamespaceTreeControl") == 0 &&
pTreeViewNotify->action == TVE_EXPAND)
{
...
}
}Worth considering as the more thorough fix: both messages the mod cares about are delivered to windows it can identify — 2. Requiring a live key/button state likely blocks expansions the user did ask for
If you move to the subclassing approach above, a much more precise signal becomes available: set a flag in the subclass proc while dispatching 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. |
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.