https://git.reactos.org/?p=reactos.git;a=commitdiff;h=544b7344982bdd6a1e1180e1a9964744ff0653b0
commit 544b7344982bdd6a1e1180e1a9964744ff0653b0 Author: Mark Jansen <[email protected]> AuthorDate: Thu Sep 22 23:03:21 2022 +0200 Commit: Mark Jansen <[email protected]> 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);
