[reactos] 01/01: [SHELL32] Improve CFSDropTarget::CopyItems logic (#5481)

[Huw Campbell]

https://git.reactos.org/?p=reactos.git;a=commitdiff;h=1273bbe417825fa4d6d01…

commit 1273bbe417825fa4d6d012c7d19c58637a02da46
Author:     Huw Campbell <huw.campbell(a)gmail.com>
AuthorDate: Sat Aug 5 20:12:36 2023 +1000
Commit:     GitHub <noreply(a)github.com>
CommitDate: Sat Aug 5 13:12:36 2023 +0300

    [SHELL32] Improve CFSDropTarget::CopyItems logic (#5481)
    
    The previous version resolved the path of the parent then did logic
    assuming simple pidls. This now resolves each source directly.
    
    This change addresses a longstanding TODO in copying and moving to shell folders.
    It's a simple change, but it removes a bit of code and makes things simpler.
    
    Corresponding Wine PR: https://gitlab.winehq.org/wine/wine/-/merge_requests/3360
---
 dll/win32/shell32/droptargets/CFSDropTarget.cpp | 99 ++++++++++---------------
 1 file changed, 41 insertions(+), 58 deletions(-)

diff --git a/dll/win32/shell32/droptargets/CFSDropTarget.cpp
b/dll/win32/shell32/droptargets/CFSDropTarget.cpp
index 7dff8fb3885..10aee5e3532 100644
--- a/dll/win32/shell32/droptargets/CFSDropTarget.cpp
+++ b/dll/win32/shell32/droptargets/CFSDropTarget.cpp
@@ -24,82 +24,65 @@
 
 WINE_DEFAULT_DEBUG_CHANNEL (shell);
 
-/****************************************************************************
- * BuildPathsList
- *
- * Builds a list of paths like the one used in SHFileOperation from a table of
- * PIDLs relative to the given base folder
- */
-static WCHAR* BuildPathsList(LPCWSTR wszBasePath, int cidl, LPCITEMIDLIST *pidls)
-{
-    WCHAR *pwszPathsList = (WCHAR *)HeapAlloc(GetProcessHeap(), 0, MAX_PATH *
sizeof(WCHAR) * cidl + 1);
-    WCHAR *pwszListPos = pwszPathsList;
-
-    for (int i = 0; i < cidl; i++)
-    {
-        FileStructW* pDataW = _ILGetFileStructW(pidls[i]);
-        if (!pDataW)
-        {
-            ERR("Mistreating a pidl:\n");
-            pdump_always(pidls[i]);
-            continue;
-        }
-
-        PathCombineW(pwszListPos, wszBasePath, pDataW->wszName);
-        pwszListPos += wcslen(pwszListPos) + 1;
-    }
-    *pwszListPos = 0;
-    return pwszPathsList;
-}
 
 /****************************************************************************
  * CFSDropTarget::_CopyItems
  *
- * copies items to this folder
- * FIXME: We should not ask the parent folder: 'What is your path', and then
manually build paths assuming everything is a simple pidl!
- * We should be asking the parent folder: Give me a full name for this pidl (for each
child!)
+ * copies or moves items to this folder
  */
 HRESULT CFSDropTarget::_CopyItems(IShellFolder * pSFFrom, UINT cidl,
                                   LPCITEMIDLIST * apidl, BOOL bCopy)
 {
-    LPWSTR pszSrcList;
-    HRESULT hr;
-    WCHAR wszTargetPath[MAX_PATH + 1];
+    HRESULT ret;
+    WCHAR wszDstPath[MAX_PATH + 1] = {0};
+    PWCHAR pwszSrcPathsList = (PWCHAR) HeapAlloc(GetProcessHeap(), 0, MAX_PATH *
sizeof(WCHAR) * cidl + 1);
+    if (!pwszSrcPathsList)
+        return E_OUTOFMEMORY;
 
-    wcscpy(wszTargetPath, m_sPathTarget);
-    //Double NULL terminate.
-    wszTargetPath[wcslen(wszTargetPath) + 1] = '\0';
+    PWCHAR pwszListPos = pwszSrcPathsList;
+    STRRET strretFrom;
+    SHFILEOPSTRUCTW fop;
 
-    TRACE ("(%p)->(%p,%u,%p)\n", this, pSFFrom, cidl, apidl);
+    /* Build a double null terminated list of C strings from source paths */
+    for (UINT i = 0; i < cidl; i++)
+    {
+        ret = pSFFrom->GetDisplayNameOf(apidl[i], SHGDN_FORPARSING, &strretFrom);
+        if (FAILED(ret))
+            goto cleanup;
 
-    STRRET strretFrom;
-    hr = pSFFrom->GetDisplayNameOf(NULL, SHGDN_FORPARSING, &strretFrom);
-    if (hr != S_OK)
-        return hr;
+        ret = StrRetToBufW(&strretFrom, NULL, pwszListPos, MAX_PATH);
+        if (FAILED(ret))
+            goto cleanup;
 
-    pszSrcList = BuildPathsList(strretFrom.pOleStr, cidl, apidl);
-    TRACE("Source file (just the first) = %s, target path = %s, bCopy: %d\n",
debugstr_w(pszSrcList), debugstr_w(m_sPathTarget), bCopy);
-    CoTaskMemFree(strretFrom.pOleStr);
-    if (!pszSrcList)
-        return E_OUTOFMEMORY;
+        pwszListPos += lstrlenW(pwszListPos) + 1;
+    }
+    /* Append the final null. */
+    *pwszListPos = L'\0';
 
-    SHFILEOPSTRUCTW op = {0};
-    op.pFrom = pszSrcList;
-    op.pTo = wszTargetPath;
-    op.hwnd = m_hwndSite;
-    op.wFunc = bCopy ? FO_COPY : FO_MOVE;
-    op.fFlags = FOF_ALLOWUNDO | FOF_NOCONFIRMMKDIR;
+    /* Build a double null terminated target (this path) */
+    ret = StringCchCopyW(wszDstPath, MAX_PATH, m_sPathTarget);
+    if (FAILED(ret))
+        goto cleanup;
 
-    int res = SHFileOperationW(&op);
+    wszDstPath[lstrlenW(wszDstPath) + 1] = L'\0';
 
-    HeapFree(GetProcessHeap(), 0, pszSrcList);
+    ZeroMemory(&fop, sizeof(fop));
+    fop.hwnd = m_hwndSite;
+    fop.wFunc = bCopy ? FO_COPY : FO_MOVE;
+    fop.pFrom = pwszSrcPathsList;
+    fop.pTo = wszDstPath;
+    fop.fFlags = FOF_ALLOWUNDO | FOF_NOCONFIRMMKDIR;
+    ret = S_OK;
 
-    if (res)
+    if (SHFileOperationW(&fop))
     {
-        ERR("SHFileOperationW failed with 0x%x\n", res);
-        return E_FAIL;
+        ERR("SHFileOperationW failed\n");
+        ret = E_FAIL;
     }
-    return S_OK;
+
+cleanup:
+    HeapFree(GetProcessHeap(), 0, pwszSrcPathsList);
+    return ret;
 }
 
 CFSDropTarget::CFSDropTarget():