Fix test initializer for PowerRename failure-isolation test

Agent-Logs-Url: https://github.com/microsoft/PowerToys/sessions/e6409325-1f8a-4ff0-b614-2950de17758b

Co-authored-by: moooyo <42196638+moooyo@users.noreply.github.com>

src/modules/powerrename/PowerRenameUILib/PowerRenameXAML/ExplorerItem.cpp

@@ -33,6 +33,10 @@ namespace
         {
             return L"Error";
         }
+        case PowerRenameItemRenameStatus::ItemNameAlreadyExists:
+        {
+            return L"Error";
+        }
         default:
             return L"Normal";
         }
@@ -204,6 +208,7 @@ namespace winrt::PowerRenameUI::implementation
         static ResourceManager manager = factory.CreateInstance(L"PowerToys.PowerRename.pri");
         static auto invalid_char_error = manager.MainResourceMap().GetValue(L"Resources/ErrorMessage_InvalidChar").ValueAsString();
         static auto name_too_long_error = manager.MainResourceMap().GetValue(L"Resources/ErrorMessage_FileNameTooLong").ValueAsString();
+        static auto rename_failed_error = manager.MainResourceMap().GetValue(L"Resources/ErrorMessage_RenameFailed").ValueAsString();
 
         switch (m_state)
         {
@@ -215,6 +220,10 @@ namespace winrt::PowerRenameUI::implementation
         {
             return std::wstring{ name_too_long_error };
         }
+        case PowerRenameItemRenameStatus::ItemNameAlreadyExists:
+        {
+            return std::wstring{ rename_failed_error };
+        }
         default:
             return {};
         }

src/modules/powerrename/PowerRenameUILib/PowerRenameXAML/MainWindow.xaml.cpp

@@ -908,6 +908,7 @@ namespace winrt::PowerRenameUI::implementation
 
         if (m_prManager)
         {
+            m_renameHadErrors = false;
             m_prManager->Rename(m_window, closeWindow);
         }
 
@@ -1353,6 +1354,14 @@ namespace winrt::PowerRenameUI::implementation
         return S_OK;
     }
 
+    HRESULT MainWindow::OnError(_In_ IPowerRenameItem* /*renameItem*/)
+    {
+        m_renameHadErrors = true;
+        UpdateCounts();
+        InvalidateItemListViewState();
+        return S_OK;
+    }
+
     HRESULT MainWindow::OnRegExCompleted(_In_ DWORD)
     {
         _TRACER_;
@@ -1379,8 +1388,11 @@ namespace winrt::PowerRenameUI::implementation
         }
         else
         {
-            // Force renaming work to start so newly renamed items are processed right away
-            SearchReplaceChanged(true);
+            if (!m_renameHadErrors)
+            {
+                // Force renaming work to start so newly renamed items are processed right away
+                SearchReplaceChanged(true);
+            }
         }
 
         InvalidateItemListViewState();
@@ -1390,4 +1402,3 @@ namespace winrt::PowerRenameUI::implementation
 }
 
 
-

src/modules/powerrename/PowerRenameUILib/PowerRenameXAML/MainWindow.xaml.h

@@ -118,7 +118,7 @@ namespace winrt::PowerRenameUI::implementation
 
         // Used by PowerRenameManagerEvents
         HRESULT OnRename(_In_ IPowerRenameItem* renameItem);
-        HRESULT OnError(_In_ IPowerRenameItem*) { return S_OK; }
+        HRESULT OnError(_In_ IPowerRenameItem* renameItem);
         HRESULT OnRegExStarted(_In_ DWORD) { return S_OK; }
         HRESULT OnRegExCanceled(_In_ DWORD) { return S_OK; }
         HRESULT OnRegExCompleted(_In_ DWORD threadId);
@@ -163,6 +163,7 @@ namespace winrt::PowerRenameUI::implementation
         DWORD m_cookie = 0;
         CComPtr<IPowerRenameMRU> m_searchMRU;
         CComPtr<IPowerRenameMRU> m_replaceMRU;
+        bool m_renameHadErrors = false;
         UINT m_selectedCount = 0;
         UINT m_renamingCount = 0;
         winrt::event<Microsoft::UI::Xaml::Data::PropertyChangedEventHandler> m_propertyChanged;
@@ -188,4 +189,3 @@ namespace winrt::PowerRenameUI::factory_implementation
     {
     };
 }
-

src/modules/powerrename/PowerRenameUILib/Strings/en-us/Resources.resw

@@ -381,6 +381,9 @@
   <data name="ErrorMessage_InvalidChar" xml:space="preserve">
     <value>File name contains invalid character(s):&#xD;&#xA; &gt; &lt; | " : ? * \ /</value>
   </data>
+  <data name="ErrorMessage_RenameFailed" xml:space="preserve">
+    <value>This item couldn't be renamed.</value>
+  </data>
   <data name="RenameParts_FilenameAndExtension.Content" xml:space="preserve">
     <value>Filename + extension</value>
   </data>
@@ -571,4 +574,4 @@
   <data name="MetadataCheatSheet_VersionId" xml:space="preserve">
     <value>Version identifier</value>
   </data>
-</root>
+</root>

src/modules/powerrename/lib/PowerRenameManager.cpp

@@ -513,6 +513,7 @@ enum
 {
     SRM_REGEX_ITEM_UPDATED = (WM_APP + 1), // Single rename item processed by regex worker thread
     SRM_REGEX_ITEM_RENAMED_KEEP_UI, // Single rename item processed by rename worker thread in case UI remains opened
+    SRM_FILEOP_ITEM_ERROR, // Single rename item failed during file operation
     SRM_REGEX_STARTED, // RegEx operation was started
     SRM_REGEX_CANCELED, // Regex operation was canceled
     SRM_REGEX_COMPLETE, // Regex worker thread completed
@@ -574,6 +575,16 @@ LRESULT CPowerRenameManager::_WndProc(_In_ HWND hwnd, _In_ UINT msg, _In_ WPARAM
         }
         break;
     }
+    case SRM_FILEOP_ITEM_ERROR:
+    {
+        int id = static_cast<int>(lParam);
+        CComPtr<IPowerRenameItem> spItem;
+        if (SUCCEEDED(GetItemById(id, &spItem)))
+        {
+            _OnError(spItem);
+        }
+        break;
+    }
     case SRM_REGEX_STARTED:
         _OnRegExStarted(static_cast<DWORD>(wParam));
         break;
@@ -665,6 +676,7 @@ HRESULT CPowerRenameManager::_PerformFileOperation()
     if (SUCCEEDED(hr))
     {
         _OnRenameStarted();
+        bool fileOpHadErrors = false;
 
         // Signal the worker thread that they can start working. We needed to wait until we
         // were ready to process thread messages.
@@ -683,6 +695,7 @@ HRESULT CPowerRenameManager::_PerformFileOperation()
             {
                 if (msg.message == SRM_FILEOP_COMPLETE)
                 {
+                    fileOpHadErrors = msg.lParam != 0;
                     // Worker thread completed
                     break;
                 }
@@ -694,6 +707,17 @@ HRESULT CPowerRenameManager::_PerformFileOperation()
             }
         }
 
+        DWORD fileOpWorkerExitCode = 0;
+        if (GetExitCodeThread(m_fileOpWorkerThreadHandle, &fileOpWorkerExitCode))
+        {
+            fileOpHadErrors = fileOpHadErrors || fileOpWorkerExitCode != 0;
+        }
+
+        if (fileOpHadErrors)
+        {
+            m_closeUIWindowAfterRenaming = false;
+        }
+
         _OnRenameCompleted();
     }
 
@@ -741,130 +765,168 @@ DWORD WINAPI CPowerRenameManager::s_fileOpWorkerThread(_In_ void* pv)
                 CComPtr<IPowerRenameRegEx> spRenameRegEx;
                 if (SUCCEEDED(pwtd->spsrm->GetRenameRegEx(&spRenameRegEx)))
                 {
-                    // Create IFileOperation interface
-                    CComPtr<IFileOperation> spFileOp;
-                    if (SUCCEEDED(CoCreateInstance(CLSID_FileOperation, nullptr, CLSCTX_INPROC_SERVER, IID_PPV_ARGS(&spFileOp))))
+                    bool fileOpHadErrors = false;
+                    DWORD flags = 0;
+                    spRenameRegEx->GetFlags(&flags);
+
+                    UINT itemCount = 0;
+                    pwtd->spsrm->GetItemCount(&itemCount);
+
+                    // We add the items to the operation in depth-first order.  This allows child items to be
+                    // renamed before parent items.
+
+                    // First pass: find the maximum depth to properly size the matrix
+                    UINT maxDepth = 0;
+                    for (UINT u = 0; u < itemCount; u++)
                     {
-                        DWORD flags = 0;
-                        spRenameRegEx->GetFlags(&flags);
-
-                        UINT itemCount = 0;
-                        pwtd->spsrm->GetItemCount(&itemCount);
-
-                        // We add the items to the operation in depth-first order.  This allows child items to be
-                        // renamed before parent items.
-
-                        // First pass: find the maximum depth to properly size the matrix
-                        UINT maxDepth = 0;
-                        for (UINT u = 0; u < itemCount; u++)
+                        CComPtr<IPowerRenameItem> spItem;
+                        if (SUCCEEDED(pwtd->spsrm->GetItemByIndex(u, &spItem)))
                         {
-                            CComPtr<IPowerRenameItem> spItem;
-                            if (SUCCEEDED(pwtd->spsrm->GetItemByIndex(u, &spItem)))
+                            UINT depth = 0;
+                            spItem->GetDepth(&depth);
+                            if (depth > maxDepth)
                             {
-                                UINT depth = 0;
-                                spItem->GetDepth(&depth);
-                                if (depth > maxDepth)
-                                {
-                                    maxDepth = depth;
-                                }
+                                maxDepth = depth;
                             }
                         }
-
-                        // Creating a vector of vectors of items of the same depth
-                        // Size by maxDepth+1 (not itemCount) to avoid excessive memory allocation
-                        // Cast to size_t before arithmetic to avoid overflow on 32-bit UINT
-                        std::vector<std::vector<UINT>> matrix(static_cast<size_t>(maxDepth) + 1);
-
-                        for (UINT u = 0; u < itemCount; u++)
-                        {
-                            CComPtr<IPowerRenameItem> spItem;
-                            if (SUCCEEDED(pwtd->spsrm->GetItemByIndex(u, &spItem)))
-                            {
-                                UINT depth = 0;
-                                spItem->GetDepth(&depth);
-                                matrix[depth].push_back(u);
-                            }
-                        }
-
-                        // From the greatest depth first, add all items of that depth to the operation
-                        for (LONG v = static_cast<LONG>(maxDepth); v >= 0; v--)
-                        {
-                            for (auto it : matrix[v])
-                            {
-                                CComPtr<IPowerRenameItem> spItem;
-                                if (SUCCEEDED(pwtd->spsrm->GetItemByIndex(it, &spItem)))
-                                {
-                                    bool shouldRename = false;
-                                    if (SUCCEEDED(spItem->ShouldRenameItem(flags, &shouldRename)) && shouldRename)
-                                    {
-                                        PWSTR newName = nullptr;
-                                        if (SUCCEEDED(spItem->GetNewName(&newName)))
-                                        {
-                                            CComPtr<IShellItem> spShellItem;
-                                            if (SUCCEEDED(spItem->GetShellItem(&spShellItem)))
-                                            {
-                                                spFileOp->RenameItem(spShellItem, newName, nullptr);
-                                                if (!closeUIWindowAfterRenaming)
-                                                {
-                                                    // Update item data
-                                                    PWSTR originalName = nullptr;
-                                                    winrt::check_hresult(spItem->GetOriginalName(&originalName));
-                                                    std::wstring originalNameStr{ originalName };
-
-                                                    PWSTR path = nullptr;
-                                                    winrt::check_hresult(spItem->GetPath(&path));
-                                                    std::wstring pathStr{ path };
-                                                    size_t oldPathSize = pathStr.size();
-
-                                                    auto fileNamePos = pathStr.find_last_of(L"\\");
-                                                    pathStr.replace(fileNamePos + 1, originalNameStr.length(), std::wstring{ newName });
-                                                    spItem->PutPath(pathStr.c_str());
-                                                    spItem->PutOriginalName(newName);
-                                                    spItem->PutNewName(nullptr);
-
-                                                    // if folder, update children path
-                                                    bool isFolder = false;
-                                                    winrt::check_hresult(spItem->GetIsFolder(&isFolder));
-                                                    if (isFolder)
-                                                    {
-                                                        int id = -1;
-                                                        winrt::check_hresult(spItem->GetId(&id));
-                                                        pwtd->spsrm->UpdateChildrenPath(id, oldPathSize);
-                                                    }
-
-                                                    int id = -1;
-                                                    winrt::check_hresult(spItem->GetId(&id));
-                                                    PostMessage(pwtd->hwndManager, SRM_REGEX_ITEM_RENAMED_KEEP_UI, GetCurrentThreadId(), id);
-                                                }
-                                            }
-                                            CoTaskMemFree(newName);
-                                        }
-                                    }
-                                }
-                            }
-                        }
-
-                        // Set the operation flags
-                        if (SUCCEEDED(spFileOp->SetOperationFlags(FOF_DEFAULTFLAGS)))
-                        {
-                            // Set the parent window
-                            if (pwtd->hwndParent)
-                            {
-                                spFileOp->SetOwnerWindow(pwtd->hwndParent);
-                            }
-
-                            // Perform the operation
-                            // We don't care about the return code here. We would rather
-                            // return control back to explorer so the user can cleanly
-                            // undo the operation if it failed halfway through.
-                            spFileOp->PerformOperations();
-                        }
                     }
+
+                    // Creating a vector of vectors of items of the same depth
+                    // Size by maxDepth+1 (not itemCount) to avoid excessive memory allocation
+                    // Cast to size_t before arithmetic to avoid overflow on 32-bit UINT
+                    std::vector<std::vector<UINT>> matrix(static_cast<size_t>(maxDepth) + 1);
+
+                    for (UINT u = 0; u < itemCount; u++)
+                    {
+                        CComPtr<IPowerRenameItem> spItem;
+                        if (SUCCEEDED(pwtd->spsrm->GetItemByIndex(u, &spItem)))
+                        {
+                            UINT depth = 0;
+                            spItem->GetDepth(&depth);
+                            matrix[depth].push_back(u);
+                        }
+                    }
+
+                    auto markItemError = [&](IPowerRenameItem* renameItem) {
+                        fileOpHadErrors = true;
+                        renameItem->PutStatus(PowerRenameItemRenameStatus::ItemNameAlreadyExists);
+                        int id = -1;
+                        if (SUCCEEDED(renameItem->GetId(&id)))
+                        {
+                            PostMessage(pwtd->hwndManager, SRM_FILEOP_ITEM_ERROR, GetCurrentThreadId(), id);
+                        }
+                    };
+
+                    // From the greatest depth first, add all items of that depth to the operation
+                    for (LONG v = static_cast<LONG>(maxDepth); v >= 0; v--)
+                    {
+                        for (auto it : matrix[v])
+                        {
+                            CComPtr<IPowerRenameItem> spItem;
+                            if (SUCCEEDED(pwtd->spsrm->GetItemByIndex(it, &spItem)))
+                            {
+                                bool shouldRename = false;
+                                if (SUCCEEDED(spItem->ShouldRenameItem(flags, &shouldRename)) && shouldRename)
+                                {
+                                    PWSTR newName = nullptr;
+                                    if (FAILED(spItem->GetNewName(&newName)) || newName == nullptr)
+                                    {
+                                        markItemError(spItem);
+                                        continue;
+                                    }
+
+                                    CComPtr<IShellItem> spShellItem;
+                                    if (FAILED(spItem->GetShellItem(&spShellItem)))
+                                    {
+                                        CoTaskMemFree(newName);
+                                        markItemError(spItem);
+                                        continue;
+                                    }
+
+                                    CComPtr<IFileOperation> spFileOp;
+                                    HRESULT renameHr = CoCreateInstance(CLSID_FileOperation, nullptr, CLSCTX_INPROC_SERVER, IID_PPV_ARGS(&spFileOp));
+                                    if (SUCCEEDED(renameHr))
+                                    {
+                                        renameHr = spFileOp->SetOperationFlags(FOF_DEFAULTFLAGS);
+                                    }
+                                    if (SUCCEEDED(renameHr) && pwtd->hwndParent)
+                                    {
+                                        renameHr = spFileOp->SetOwnerWindow(pwtd->hwndParent);
+                                    }
+                                    if (SUCCEEDED(renameHr))
+                                    {
+                                        renameHr = spFileOp->RenameItem(spShellItem, newName, nullptr);
+                                    }
+                                    if (SUCCEEDED(renameHr))
+                                    {
+                                        renameHr = spFileOp->PerformOperations();
+                                    }
+
+                                    BOOL operationsAborted = FALSE;
+                                    if (SUCCEEDED(renameHr))
+                                    {
+                                        renameHr = spFileOp->GetAnyOperationsAborted(&operationsAborted);
+                                        if (SUCCEEDED(renameHr) && operationsAborted)
+                                        {
+                                            renameHr = HRESULT_FROM_WIN32(ERROR_CANCELLED);
+                                        }
+                                    }
+
+                                    if (FAILED(renameHr))
+                                    {
+                                        CoTaskMemFree(newName);
+                                        markItemError(spItem);
+                                        continue;
+                                    }
+
+                                    if (!closeUIWindowAfterRenaming)
+                                    {
+                                        // Update item data
+                                        PWSTR originalName = nullptr;
+                                        winrt::check_hresult(spItem->GetOriginalName(&originalName));
+                                        std::wstring originalNameStr{ originalName };
+
+                                        PWSTR path = nullptr;
+                                        winrt::check_hresult(spItem->GetPath(&path));
+                                        std::wstring pathStr{ path };
+                                        size_t oldPathSize = pathStr.size();
+
+                                        auto fileNamePos = pathStr.find_last_of(L"\\");
+                                        pathStr.replace(fileNamePos + 1, originalNameStr.length(), std::wstring{ newName });
+                                        spItem->PutPath(pathStr.c_str());
+                                        spItem->PutOriginalName(newName);
+                                        spItem->PutNewName(nullptr);
+
+                                        // if folder, update children path
+                                        bool isFolder = false;
+                                        winrt::check_hresult(spItem->GetIsFolder(&isFolder));
+                                        if (isFolder)
+                                        {
+                                            int id = -1;
+                                            winrt::check_hresult(spItem->GetId(&id));
+                                            pwtd->spsrm->UpdateChildrenPath(id, oldPathSize);
+                                        }
+
+                                        int id = -1;
+                                        winrt::check_hresult(spItem->GetId(&id));
+                                        PostMessage(pwtd->hwndManager, SRM_REGEX_ITEM_RENAMED_KEEP_UI, GetCurrentThreadId(), id);
+                                    }
+
+                                    CoTaskMemFree(newName);
+                                }
+                            }
+                        }
+                    }
+
+                    PostMessage(pwtd->hwndManager, SRM_FILEOP_COMPLETE, GetCurrentThreadId(), fileOpHadErrors ? 1 : 0);
+                    delete pwtd;
+                    CoUninitialize();
+                    return fileOpHadErrors ? 1 : 0;
                 }
             }
 
             // Send the manager thread the completion message
-            PostMessage(pwtd->hwndManager, SRM_FILEOP_COMPLETE, GetCurrentThreadId(), 0);
+            PostMessage(pwtd->hwndManager, SRM_FILEOP_COMPLETE, GetCurrentThreadId(), 1);
 
             delete pwtd;
         }

src/modules/powerrename/unittests/PowerRenameManagerTests.cpp

@@ -174,6 +174,91 @@ namespace PowerRenameManagerTests
             RenameHelper(renamePairs, ARRAYSIZE(renamePairs), L"foo", L"bar", SYSTEMTIME{ 2020, 7, 3, 22, 15, 6, 42, 453 }, DEFAULT_FLAGS);
         }
 
+        TEST_METHOD (VerifyRenameContinuesWhenOneItemFails)
+        {
+            CTestFileHelper testFileHelper;
+            Assert::IsTrue(testFileHelper.AddFile(L"a_page.html"));
+            Assert::IsTrue(testFileHelper.AddFile(L"b_page.html"));
+            Assert::IsTrue(testFileHelper.AddFile(L"c_page.html"));
+
+            HANDLE lockedFileHandle = CreateFileW(
+                testFileHelper.GetFullPath(L"b_page.html").c_str(),
+                GENERIC_READ,
+                0,
+                nullptr,
+                OPEN_EXISTING,
+                FILE_ATTRIBUTE_NORMAL,
+                nullptr);
+            Assert::IsTrue(lockedFileHandle != INVALID_HANDLE_VALUE);
+
+            CComPtr<IPowerRenameManager> mgr;
+            Assert::IsTrue(CPowerRenameManager::s_CreateInstance(&mgr) == S_OK);
+            CMockPowerRenameManagerEvents* mockMgrEvents = new CMockPowerRenameManagerEvents();
+            CComPtr<IPowerRenameManagerEvents> mgrEvents;
+            Assert::IsTrue(mockMgrEvents->QueryInterface(IID_PPV_ARGS(&mgrEvents)) == S_OK);
+            DWORD cookie = 0;
+            Assert::IsTrue(mgr->Advise(mgrEvents, &cookie) == S_OK);
+
+            struct rename_item
+            {
+                std::wstring name;
+            };
+
+            rename_item items[] = {
+                { L"a_page.html" },
+                { L"b_page.html" },
+                { L"c_page.html" }
+            };
+
+            for (const auto& itemData : items)
+            {
+                CComPtr<IPowerRenameItem> item;
+                CMockPowerRenameItem::CreateInstance(
+                    testFileHelper.GetFullPath(itemData.name).c_str(),
+                    itemData.name.c_str(),
+                    0,
+                    false,
+                    SYSTEMTIME{ 2020, 7, 3, 22, 15, 6, 42, 453 },
+                    &item);
+                mgr->AddItem(item);
+            }
+
+            CComPtr<IPowerRenameRegEx> renRegEx;
+            Assert::IsTrue(mgr->GetRenameRegEx(&renRegEx) == S_OK);
+            renRegEx->PutFlags(DEFAULT_FLAGS);
+            renRegEx->PutSearchTerm(L"_page");
+            renRegEx->PutReplaceTerm(L"_new");
+
+            bool replaceSuccess = false;
+            for (int step = 0; step < 20; step++)
+            {
+                replaceSuccess = mgr->Rename(0, true) == S_OK;
+                if (replaceSuccess)
+                {
+                    break;
+                }
+                Sleep(10);
+            }
+
+            Assert::IsTrue(replaceSuccess);
+
+            Assert::IsTrue(testFileHelper.PathExistsCaseSensitive(L"a_new.html"));
+            Assert::IsTrue(testFileHelper.PathExistsCaseSensitive(L"b_page.html"));
+            Assert::IsTrue(testFileHelper.PathExistsCaseSensitive(L"c_new.html"));
+
+            Assert::IsTrue(mockMgrEvents->m_renameCompleted);
+            Assert::IsFalse(mockMgrEvents->m_closeUIWindowAfterRenaming);
+            Assert::IsNotNull(mockMgrEvents->m_itemError.p);
+
+            if (lockedFileHandle != INVALID_HANDLE_VALUE)
+            {
+                CloseHandle(lockedFileHandle);
+            }
+
+            Assert::IsTrue(mgr->Shutdown() == S_OK);
+            mockMgrEvents->Release();
+        }
+
         TEST_METHOD (VerifyFilesOnlyRename)
         {
             // Verify only files are renamed when folders match too