[PATCH 0/1] MR3360: shell32: Improve shell copy and delete items functions, notifications.

[Huw Campbell (＠HuwCampbell)]

Logically the change here is that the parent shell folder resolves
each child path in 'FORPARSING' mode, instead of resolving itself
and doing a bunch more string concatenation for assumed simple child
pidls.

This also removes hacks for change notifications in the delete path
by actually performing notifications for deletion and trash operations
within the shell op.

Background:

I noticed a comment in the ReactOS version which suggested the first
change, it makes sense to not query the children in this way here.

The notifications hack also seemed really off. I'm not sure why the shell
operation wasn't performing these notifications already.

Testing:
All tests pass, and loading up wine explorer was easy to test the deletion.
Items were deleted (or trashed) and the shell folder was notified.

Unfortunately, copy and paste seem very broken at the moment, as the
paste_pidls function only calls ISFHelper_CopyItems with one pidl at a time,
and it itself seems to fail to correctly resolve the from path.

I did some hacking so that it did work correctly if the 'from' folder was the
desktop, and it worked fine with these changes (but those hacks wouldn't pass
review).

[PATCH 1/1] shell32: Improve shell copy and delete items functions, notifications.

[Huw Campbell]

From: Huw Campbell

Logically the change here is that the parent shell folder resolves
each child path in 'FORPARSING' mode, instead of resolving itself
and doing a bunch of string concatenation for assumed simple child
pidls.

This also removes hacks for change notifications in the delete path
by actually performing notifications for deletion and trash operations
within the shell op.
---
 dlls/shell32/pidl.c      |   2 +-
 dlls/shell32/shfldr_fs.c | 159 ++++++++++++++-------------------------
 dlls/shell32/shlfileop.c |  10 +--
 3 files changed, 64 insertions(+), 107 deletions(-)

diff --git a/dlls/shell32/pidl.c b/dlls/shell32/pidl.c
index 9878ef9811e..6f831d51c9c 100644
--- a/dlls/shell32/pidl.c
+++ b/dlls/shell32/pidl.c
@@ -1272,7 +1272,7 @@ BOOL WINAPI SHGetPathFromIDListEx(LPCITEMIDLIST pidl, WCHAR *path, DWORD path_si
         IShellFolder_Release(psfFolder);
         return FALSE;
     }
-                
+
     hr = IShellFolder_GetDisplayNameOf(psfFolder, pidlLast, SHGDN_FORPARSING, &strret);
     IShellFolder_Release(psfFolder);
     if (FAILED(hr)) return FALSE;
diff --git a/dlls/shell32/shfldr_fs.c b/dlls/shell32/shfldr_fs.c
index 32de9ab6ef1..f231977471f 100644
--- a/dlls/shell32/shfldr_fs.c
+++ b/dlls/shell32/shfldr_fs.c
@@ -1218,36 +1218,6 @@ ISFHelper_fnAddFolder (ISFHelper * iface, HWND hwnd, LPCWSTR pwszName,
     return hres;
 }

-/****************************************************************************
- * build_paths_list
- *
- * Builds a list of paths like the one used in SHFileOperation from a table of
- * PIDLs relative to the given base folder
- */
-static WCHAR *build_paths_list(LPCWSTR wszBasePath, int cidl, const LPCITEMIDLIST *pidls)
-{
-    WCHAR *wszPathsList;
-    WCHAR *wszListPos;
-    int iPathLen;
-    int i;
-    
-    iPathLen = lstrlenW(wszBasePath);
-    wszPathsList = heap_alloc(MAX_PATH*sizeof(WCHAR)*cidl+1);
-    wszListPos = wszPathsList;
-    
-    for (i = 0; i < cidl; i++) {
-        if (!_ILIsFolder(pidls[i]) && !_ILIsValue(pidls[i]))
-            continue;
-
-        lstrcpynW(wszListPos, wszBasePath, MAX_PATH);
-        /* FIXME: abort if path too long */
-        _ILSimpleGetTextW(pidls[i], wszListPos+iPathLen, MAX_PATH-iPathLen);
-        wszListPos += lstrlenW(wszListPos)+1;
-    }
-    *wszListPos=0;
-    return wszPathsList;
-}
-
 /****************************************************************************
  * ISFHelper_fnDeleteItems
  *
@@ -1257,22 +1227,28 @@ static HRESULT WINAPI
 ISFHelper_fnDeleteItems (ISFHelper * iface, UINT cidl, LPCITEMIDLIST * apidl)
 {
     IGenericSFImpl *This = impl_from_ISFHelper(iface);
-    UINT i;
+    IShellFolder2 *pSFFrom = &This->IShellFolder2_iface;
+
     SHFILEOPSTRUCTW op;
-    WCHAR wszPath[MAX_PATH];
-    WCHAR *wszPathsList;
+    WCHAR *wszPathsList = heap_alloc(MAX_PATH * sizeof(WCHAR) * cidl + 1);
+    WCHAR *wszListPos = wszPathsList;
     HRESULT ret;
-    WCHAR *wszCurrentPath;
+    STRRET strretFrom;

-    TRACE ("(%p)(%u %p)\n", This, cidl, apidl);
-    if (cidl==0) return S_OK;
+    /* Build double null terminated list of C strings */
+    for (UINT i = 0; i < cidl; i++) {
+        ret = IShellFolder2_GetDisplayNameOf(pSFFrom, apidl[i], SHGDN_FORPARSING, &strretFrom);
+        if (FAILED(ret))
+            goto cleanup;

-    if (This->sPathTarget)
-        lstrcpynW(wszPath, This->sPathTarget, MAX_PATH);
-    else
-        wszPath[0] = '\0';
-    PathAddBackslashW(wszPath);
-    wszPathsList = build_paths_list(wszPath, cidl, apidl);
+        ret = StrRetToBufW(&strretFrom, NULL, wszListPos, MAX_PATH);
+        if (FAILED(ret))
+            goto cleanup;
+
+        wszListPos += lstrlenW(wszListPos) + 1;
+    }
+    /* Append final null. */
+    *wszListPos=0;

ZeroMemory(&op, sizeof(op));
     op.hwnd = GetActiveWindow();
@@ -1287,29 +1263,7 @@ ISFHelper_fnDeleteItems (ISFHelper * iface, UINT cidl, LPCITEMIDLIST * apidl)
     else
         ret = S_OK;

-    /* we currently need to manually send the notifies */
-    wszCurrentPath = wszPathsList;
-    for (i = 0; i < cidl; i++)
-    {
-        LONG wEventId;
-
-        if (_ILIsFolder(apidl[i]))
-            wEventId = SHCNE_RMDIR;
-        else if (_ILIsValue(apidl[i]))
-            wEventId = SHCNE_DELETE;
-        else
-            continue;
-
-        /* check if file exists */
-        if (GetFileAttributesW(wszCurrentPath) == INVALID_FILE_ATTRIBUTES)
-        {
-            LPITEMIDLIST pidl = ILCombine(This->pidlRoot, apidl[i]);
-            SHChangeNotify(wEventId, SHCNF_IDLIST, pidl, NULL);
-            ILFree(pidl);
-        }
-
-        wszCurrentPath += lstrlenW(wszCurrentPath)+1;
-    }
+cleanup:
     heap_free(wszPathsList);
     return ret;
 }
@@ -1323,48 +1277,51 @@ static HRESULT WINAPI
 ISFHelper_fnCopyItems (ISFHelper * iface, IShellFolder * pSFFrom, UINT cidl,
                        LPCITEMIDLIST * apidl)
 {
-    HRESULT ret=E_FAIL;
-    IPersistFolder2 *ppf2 = NULL;
-    WCHAR wszSrcPathRoot[MAX_PATH],
-      wszDstPath[MAX_PATH+1];
-    WCHAR *wszSrcPathsList;
     IGenericSFImpl *This = impl_from_ISFHelper(iface);
-
+    HRESULT ret;
+    WCHAR wszDstPath[MAX_PATH+1];
+    WCHAR *wszSrcPathsList = heap_alloc(MAX_PATH * sizeof(WCHAR) * cidl + 1);
+    WCHAR *wszListPos = wszSrcPathsList;
+    STRRET strretFrom;
     SHFILEOPSTRUCTW fop;

-    TRACE ("(%p)->(%p,%u,%p)\n", This, pSFFrom, cidl, apidl);
+    /* Build double null terminated list of C strings */
+    for (UINT i = 0; i < cidl; i++) {
+        ret = IShellFolder_GetDisplayNameOf(pSFFrom, apidl[i], SHGDN_FORPARSING, &strretFrom);
+        if (FAILED(ret))
+            goto cleanup;

-    IShellFolder_QueryInterface (pSFFrom, &IID_IPersistFolder2,
-     (LPVOID *) & ppf2);
-    if (ppf2) {
-        LPITEMIDLIST pidl;
+        ret = StrRetToBufW(&strretFrom, NULL, wszListPos, MAX_PATH);
+        if (FAILED(ret))
+            goto cleanup;

-        if (SUCCEEDED (IPersistFolder2_GetCurFolder (ppf2, &pidl))) {
-            SHGetPathFromIDListW (pidl, wszSrcPathRoot);
-            if (This->sPathTarget)
-                lstrcpynW(wszDstPath, This->sPathTarget, MAX_PATH);
-            else
-                wszDstPath[0] = 0;
-            PathAddBackslashW(wszSrcPathRoot);
-            PathAddBackslashW(wszDstPath);
-            wszSrcPathsList = build_paths_list(wszSrcPathRoot, cidl, apidl);
-            ZeroMemory(&fop, sizeof(fop));
-            fop.hwnd = GetActiveWindow();
-            fop.wFunc = FO_COPY;
-            fop.pFrom = wszSrcPathsList;
-            fop.pTo = wszDstPath;
-            fop.fFlags = FOF_ALLOWUNDO;
-            ret = S_OK;
-            if(SHFileOperationW(&fop))
-            {
-                WARN("Copy failed\n");
-                ret = E_FAIL;
-            }
-            heap_free(wszSrcPathsList);
-        }
-        SHFree(pidl);
-        IPersistFolder2_Release(ppf2);
+        wszListPos += lstrlenW(wszListPos) + 1;
     }
+    /* Append final null. */
+    *wszListPos=0;
+
+    if (This->sPathTarget)
+      lstrcpynW(wszDstPath, This->sPathTarget, MAX_PATH);
+    else
+      wszDstPath[0] = 0;
+
+    PathAddBackslashW(wszDstPath);
+
+    ZeroMemory(&fop, sizeof(fop));
+    fop.hwnd = GetActiveWindow();
+    fop.wFunc = FO_COPY;
+    fop.pFrom = wszSrcPathsList;
+    fop.pTo = wszDstPath;
+    fop.fFlags = FOF_ALLOWUNDO;
+    ret = S_OK;
+
+    if(SHFileOperationW(&fop)) {
+        WARN("Copy failed\n");
+        ret = E_FAIL;
+    }
+
+cleanup:
+    heap_free(wszSrcPathsList);
     return ret;
 }

diff --git a/dlls/shell32/shlfileop.c b/dlls/shell32/shlfileop.c
index 17a8e7046c7..7ad95de6136 100644
--- a/dlls/shell32/shlfileop.c
+++ b/dlls/shell32/shlfileop.c
@@ -1353,9 +1353,10 @@ static int delete_files(LPSHFILEOPSTRUCTW lpFileOp, const FILE_LIST *flFrom)
         if (bTrash)
         {
             BOOL bDelete;
-            if (trash_file(fileEntry->szFullPath))
+            if (trash_file(fileEntry->szFullPath)) {
+                SHChangeNotify(SHCNE_DELETE, SHCNF_PATHW, fileEntry->szFullPath, NULL);
                 continue;
-
+            }
             /* Note: Windows silently deletes the file in such a situation, we show a dialog */
             if (!(lpFileOp->fFlags & FOF_NOCONFIRMATION) || (lpFileOp->fFlags & FOF_WANTNUKEWARNING))
                 bDelete = SHELL_ConfirmDialogW(lpFileOp->hwnd, ASK_CANT_TRASH_ITEM, fileEntry->szFullPath, NULL);
@@ -1368,11 +1369,10 @@ static int delete_files(LPSHFILEOPSTRUCTW lpFileOp, const FILE_LIST *flFrom)
                 break;
             }
         }
-        
+
         /* delete the file or directory */
         if (IsAttribFile(fileEntry->attributes))
-            ret = DeleteFileW(fileEntry->szFullPath) ?
-                    ERROR_SUCCESS : GetLastError();
+            ret = SHNotifyDeleteFileW(fileEntry->szFullPath);
         else
             ret = SHELL_DeleteDirectoryW(lpFileOp->hwnd, fileEntry->szFullPath, FALSE);