[reactos] 01/01: [SHELL32] CDefView: Rework context menu handling

[Mark Jansen]

https://git.reactos.org/?p=reactos.git;a=commitdiff;h=544b7344982bdd6a1e118…

commit 544b7344982bdd6a1e1180e1a9964744ff0653b0
Author:     Mark Jansen <mark.jansen(a)reactos.org>
AuthorDate: Thu Sep 22 23:03:21 2022 +0200
Commit:     Mark Jansen <mark.jansen(a)reactos.org>
CommitDate: Sun Oct 2 00:36:42 2022 +0200

    [SHELL32] CDefView: Rework context menu handling
    
    Previously, we would share one object between a multitude of options.
    Now, the only two options that need to store something for later use each have their
own space for it.
    The context menu always cleans up after itself, the File menu does not.
    CORE-18345
    CORE-18361
    CORE-18366
---
 dll/win32/shell32/CDefView.cpp | 164 ++++++++++++++++++++++-------------------
 1 file changed, 89 insertions(+), 75 deletions(-)

diff --git a/dll/win32/shell32/CDefView.cpp b/dll/win32/shell32/CDefView.cpp
index 98b597c59d4..0bff83e1c9f 100644
--- a/dll/win32/shell32/CDefView.cpp
+++ b/dll/win32/shell32/CDefView.cpp
@@ -58,7 +58,7 @@ typedef struct
 static void
 ClientToListView(HWND hwndLV, POINT *ppt)
 {
-    POINT Origin;
+    POINT Origin = {};
 
     /* FIXME: LVM_GETORIGIN is broken. See CORE-17266 */
     if (!ListView_GetOrigin(hwndLV, &Origin))
@@ -68,6 +68,33 @@ ClientToListView(HWND hwndLV, POINT *ppt)
     ppt->y += Origin.y;
 }
 
+// Helper struct to automatically cleanup the IContextMenu
+// We want to explicitly reset the Site, so there are no circular references
+struct MenuCleanup
+{
+    CComPtr<IContextMenu> &m_pCM;
+    HMENU &m_hMenu;
+
+    MenuCleanup(CComPtr<IContextMenu> &pCM, HMENU& menu)
+        : m_pCM(pCM), m_hMenu(menu)
+    {
+    }
+    ~MenuCleanup()
+    {
+        if (m_hMenu)
+        {
+            DestroyMenu(m_hMenu);
+            m_hMenu = NULL;
+        }
+        if (m_pCM)
+        {
+            IUnknown_SetSite(m_pCM, NULL);
+            m_pCM.Release();
+        }
+    }
+};
+
+
 class CDefView :
     public CWindowImpl<CDefView, CWindow, CControlWinTraits>,
     public CComObjectRootEx<CComMultiThreadModelNoCS>,
@@ -116,6 +143,7 @@ class CDefView :
         DWORD                     m_grfKeyState;
         //
         CComPtr<IContextMenu>     m_pCM;
+        CComPtr<IContextMenu>     m_pFileMenu;
 
         BOOL                      m_isEditing;
         BOOL                      m_isParentFolderSpecial;
@@ -169,7 +197,7 @@ class CDefView :
         void OnDeactivate();
         void DoActivate(UINT uState);
         HRESULT drag_notify_subitem(DWORD grfKeyState, POINTL pt, DWORD *pdwEffect);
-        HRESULT InvokeContextMenuCommand(UINT uCommand);
+        HRESULT InvokeContextMenuCommand(CComPtr<IContextMenu> &pCM, UINT
uCommand);
         LRESULT OnExplorerCommand(UINT uCommand, BOOL bUseSelection);
 
         // *** IOleWindow methods ***
@@ -1310,13 +1338,6 @@ HRESULT CDefView::FillFileMenu()
     if (!hFileMenu)
         return E_FAIL;
 
-    /* Release cached IContextMenu */
-    if (m_pCM)
-    {
-        IUnknown_SetSite(m_pCM, NULL);
-        m_pCM.Release();
-    }
-
     /* Cleanup the items added previously */
     for (int i = GetMenuItemCount(hFileMenu) - 1; i >= 0; i--)
     {
@@ -1327,15 +1348,20 @@ HRESULT CDefView::FillFileMenu()
 
     m_cidl = m_ListView.GetSelectedCount();
 
-    /* Store the context menu in m_pCM and keep it in order to invoke the selected
command later on */
-    HRESULT hr = GetItemObject((m_cidl ? SVGIO_SELECTION : SVGIO_BACKGROUND),
-                               IID_PPV_ARG(IContextMenu, &m_pCM));
+    /* In case we still have this left over, clean it up! */
+    if (m_pFileMenu)
+    {
+        IUnknown_SetSite(m_pFileMenu, NULL);
+        m_pFileMenu.Release();
+    }
+    /* Store the context menu in m_pFileMenu and keep it in order to invoke the selected
command later on */
+    HRESULT hr = GetItemObject((m_cidl ? SVGIO_SELECTION : SVGIO_BACKGROUND),
IID_PPV_ARG(IContextMenu, &m_pFileMenu));
     if (FAILED_UNEXPECTEDLY(hr))
         return hr;
 
     HMENU hmenu = CreatePopupMenu();
 
-    hr = m_pCM->QueryContextMenu(hmenu, 0, FCIDM_SHVIEWFIRST, FCIDM_SHVIEWLAST, 0);
+    hr = m_pFileMenu->QueryContextMenu(hmenu, 0, FCIDM_SHVIEWFIRST, FCIDM_SHVIEWLAST,
0);
     if (FAILED_UNEXPECTEDLY(hr))
         return hr;
 
@@ -1470,7 +1496,7 @@ UINT CDefView::GetSelections()
     return m_cidl;
 }
 
-HRESULT CDefView::InvokeContextMenuCommand(UINT uCommand)
+HRESULT CDefView::InvokeContextMenuCommand(CComPtr<IContextMenu> &pCM, UINT
uCommand)
 {
     CMINVOKECOMMANDINFO cmi;
 
@@ -1485,7 +1511,11 @@ HRESULT CDefView::InvokeContextMenuCommand(UINT uCommand)
     if (GetKeyState(VK_CONTROL) & 0x8000)
         cmi.fMask |= CMIC_MASK_CONTROL_DOWN;
 
-    HRESULT hr = m_pCM->InvokeCommand(&cmi);
+    HRESULT hr = pCM->InvokeCommand(&cmi);
+    // Most of our callers will do this, but in case they don't do that (File menu!)
+    IUnknown_SetSite(pCM, NULL);
+    pCM.Release();
+
     if (FAILED_UNEXPECTEDLY(hr))
         return hr;
 
@@ -1513,33 +1543,24 @@ HRESULT CDefView::OpenSelectedItems()
     if (!hMenu)
         return E_FAIL;
 
-    hResult = GetItemObject(SVGIO_SELECTION, IID_PPV_ARG(IContextMenu, &m_pCM));
+    CComPtr<IContextMenu> pCM;
+    hResult = GetItemObject(SVGIO_SELECTION, IID_PPV_ARG(IContextMenu, &pCM));
+    MenuCleanup _(pCM, hMenu);
     if (FAILED_UNEXPECTEDLY(hResult))
-        goto cleanup;
+        return hResult;
 
-    hResult = m_pCM->QueryContextMenu(hMenu, 0, FCIDM_SHVIEWFIRST, FCIDM_SHVIEWLAST,
CMF_DEFAULTONLY);
+    hResult = pCM->QueryContextMenu(hMenu, 0, FCIDM_SHVIEWFIRST, FCIDM_SHVIEWLAST,
CMF_DEFAULTONLY);
     if (FAILED_UNEXPECTEDLY(hResult))
-        goto cleanup;
+        return hResult;
 
     uCommand = GetMenuDefaultItem(hMenu, FALSE, 0);
     if (uCommand == (UINT)-1)
     {
-        hResult = E_FAIL;
-        goto cleanup;
+        ERR("GetMenuDefaultItem returned -1\n");
+        return E_FAIL;
     }
 
-    InvokeContextMenuCommand(uCommand);
-
-cleanup:
-
-    if (hMenu)
-        DestroyMenu(hMenu);
-
-    if (m_pCM)
-    {
-        IUnknown_SetSite(m_pCM, NULL);
-        m_pCM.Release();
-    }
+    InvokeContextMenuCommand(pCM, uCommand);
 
     return hResult;
 }
@@ -1555,6 +1576,12 @@ LRESULT CDefView::OnContextMenu(UINT uMsg, WPARAM wParam, LPARAM
lParam, BOOL &b
 
     TRACE("(%p)\n", this);
 
+    if (m_hContextMenu != NULL)
+    {
+        ERR("HACK: Aborting context menu in nested call!\n");
+        return 0;
+    }
+
     m_hContextMenu = CreatePopupMenu();
     if (!m_hContextMenu)
         return E_FAIL;
@@ -1578,15 +1605,16 @@ LRESULT CDefView::OnContextMenu(UINT uMsg, WPARAM wParam, LPARAM
lParam, BOOL &b
     }
 
     m_cidl = m_ListView.GetSelectedCount();
-
+    /* In case we still have this left over, clean it up! */
     hResult = GetItemObject( m_cidl ? SVGIO_SELECTION : SVGIO_BACKGROUND,
IID_PPV_ARG(IContextMenu, &m_pCM));
+    MenuCleanup _(m_pCM, m_hContextMenu);
     if (FAILED_UNEXPECTEDLY(hResult))
-        goto cleanup;
+        return 0;
 
     /* Use 1 as the first id as we want 0 the mean that the user canceled the menu */
     hResult = m_pCM->QueryContextMenu(m_hContextMenu, 0, CONTEXT_MENU_BASE_ID,
FCIDM_SHVIEWLAST, CMF_NORMAL);
     if (FAILED_UNEXPECTEDLY(hResult))
-        goto cleanup;
+        return 0;
 
     /* There is no position requested, so try to find one */
     if (lParam == ~0)
@@ -1624,29 +1652,17 @@ LRESULT CDefView::OnContextMenu(UINT uMsg, WPARAM wParam, LPARAM
lParam, BOOL &b
         y = pt.y;
     }
 
+    // This runs the message loop, calling back to us with f.e. WM_INITPOPUP (hence why
m_hContextMenu and m_pCM exist) 
     uCommand = TrackPopupMenu(m_hContextMenu,
                               TPM_LEFTALIGN | TPM_RETURNCMD | TPM_LEFTBUTTON |
TPM_RIGHTBUTTON,
                               x, y, 0, m_hWnd, NULL);
     if (uCommand == 0)
-        goto cleanup;
+        return 0;
 
     if (uCommand == FCIDM_SHVIEW_OPEN && OnDefaultCommand() == S_OK)
-        goto cleanup;
-
-    InvokeContextMenuCommand(uCommand - CONTEXT_MENU_BASE_ID);
+        return 0;
 
-cleanup:
-    if (m_pCM)
-    {
-        IUnknown_SetSite(m_pCM, NULL);
-        m_pCM.Release();
-    }
-
-    if (m_hContextMenu)
-    {
-        DestroyMenu(m_hContextMenu);
-        m_hContextMenu = NULL;
-    }
+    InvokeContextMenuCommand(m_pCM, uCommand - CONTEXT_MENU_BASE_ID);
 
     return 0;
 }
@@ -1656,18 +1672,22 @@ LRESULT CDefView::OnExplorerCommand(UINT uCommand, BOOL
bUseSelection)
     HRESULT hResult;
     HMENU hMenu = NULL;
 
-    hResult = GetItemObject( bUseSelection ? SVGIO_SELECTION : SVGIO_BACKGROUND,
IID_PPV_ARG(IContextMenu, &m_pCM));
-    if (FAILED_UNEXPECTEDLY( hResult))
-        goto cleanup;
+    CComPtr<IContextMenu> pCM;
+    hResult = GetItemObject( bUseSelection ? SVGIO_SELECTION : SVGIO_BACKGROUND,
IID_PPV_ARG(IContextMenu, &pCM));
+    if (FAILED_UNEXPECTEDLY(hResult))
+        return 0;
+
+    MenuCleanup _(pCM, hMenu);
+
     if ((uCommand != FCIDM_SHVIEW_DELETE) && (uCommand != FCIDM_SHVIEW_RENAME))
     {
         hMenu = CreatePopupMenu();
         if (!hMenu)
             return 0;
 
-        hResult = m_pCM->QueryContextMenu(hMenu, 0, FCIDM_SHVIEWFIRST,
FCIDM_SHVIEWLAST, CMF_NORMAL);
+        hResult = pCM->QueryContextMenu(hMenu, 0, FCIDM_SHVIEWFIRST, FCIDM_SHVIEWLAST,
CMF_NORMAL);
         if (FAILED_UNEXPECTEDLY(hResult))
-            goto cleanup;
+            return 0;
     }
 
     if (bUseSelection)
@@ -1690,18 +1710,7 @@ LRESULT CDefView::OnExplorerCommand(UINT uCommand, BOOL
bUseSelection)
             return 0;
     }
 
-    InvokeContextMenuCommand(uCommand);
-
-cleanup:
-    if (m_pCM)
-    {
-        IUnknown_SetSite(m_pCM, NULL);
-        m_pCM.Release();
-    }
-
-    if (hMenu)
-        DestroyMenu(hMenu);
-
+    InvokeContextMenuCommand(pCM, uCommand);
     return 0;
 }
 
@@ -1936,10 +1945,12 @@ LRESULT CDefView::OnCommand(UINT uMsg, WPARAM wParam, LPARAM
lParam, BOOL &bHand
         case FCIDM_SHVIEW_NEWFOLDER:
             return OnExplorerCommand(dwCmdID, FALSE);
         default:
-            /* WM_COMMAND messages from the file menu are routed to the CDefView so as to
let m_pCM handle the command */
-            if (m_pCM && dwCmd == 0)
+            /* WM_COMMAND messages from the file menu are routed to the CDefView so as to
let m_pFileMenu handle the command */
+            if (m_pFileMenu && dwCmd == 0)
             {
-                InvokeContextMenuCommand(dwCmdID);
+                HMENU Dummy = NULL;
+                MenuCleanup _(m_pFileMenu, Dummy);
+                InvokeContextMenuCommand(m_pFileMenu, dwCmdID);
             }
     }
 
@@ -2370,10 +2381,10 @@ HRESULT SHSetMenuIdInMenuMsg(UINT uMsg, LPARAM lParam, UINT
CmdId);
 */
 LRESULT CDefView::OnCustomItem(UINT uMsg, WPARAM wParam, LPARAM lParam, BOOL
&bHandled)
 {
-    if (!m_pCM.p)
+    if (!m_pCM)
     {
         /* no menu */
-        ERR("no menu!!!\n");
+        ERR("no context menu!!!\n");
         return FALSE;
     }
 
@@ -2409,7 +2420,10 @@ LRESULT CDefView::OnInitMenuPopup(UINT uMsg, WPARAM wParam, LPARAM
lParam, BOOL
     int nPos = LOWORD(lParam);
     UINT  menuItemId;
 
-    OnCustomItem(uMsg, wParam, lParam, bHandled);
+    if (m_pCM)
+    {
+        OnCustomItem(uMsg, wParam, lParam, bHandled);
+    }
 
     HMENU hViewMenu = GetSubmenuByID(m_hMenu, FCIDM_MENU_VIEW);