From 3b4007d29958d5dd782968011e0e5431110e25fe Mon Sep 17 00:00:00 2001 From: Dave Rayment Date: Mon, 29 Sep 2025 03:05:21 +0100 Subject: [PATCH] [PowerRename] Fix issue where counter does not update if the filename and replacement result matched (#42006) This PR fixes an issue which has been reported a number of times under slightly different guises. The bug manifests as a counter "stall" or "skip" under certain circumstances, not advancing the counter if the result of the rename operation happens to match the original filename. ## Summary of the Pull Request This issue occurs if all the following are true: - Enumeration features are enabled - Regular expression matching is enabled - The replacement string includes a counter, for example `${}` or `${start=1}` or `${start=10,increment=10}` etc. - There are one or more original filenames which coincide with the result of the renaming operations Previously, the counter was not updated when the renaming operation result was the same as the original filename. For example, here the first rename result matches the original filename and the counter remains at `1` for the second file, whereas it should be `2`: image This fix increments the counter irrespective of these coincidences. ## PR Checklist - [x] Closes: #41950, #31950, #33884 - [ ] **Communication:** I've discussed this with core contributors already. If the work hasn't been agreed, this work might be rejected - [x] **Tests:** Added/updated and all pass - [ ] **Localization:** All end-user-facing strings can be localized - [ ] **Dev docs:** Added/updated - [ ] **New binaries:** Added on the required places - [ ] [JSON for signing](https://github.com/microsoft/PowerToys/blob/main/.pipelines/ESRPSigning_core.json) for new binaries - [ ] [WXS for installer](https://github.com/microsoft/PowerToys/blob/main/installer/PowerToysSetup/Product.wxs) for new binaries and localization folder - [ ] [YML for CI pipeline](https://github.com/microsoft/PowerToys/blob/main/.pipelines/ci/templates/build-powertoys-steps.yml) for new test projects - [ ] [YML for signed pipeline](https://github.com/microsoft/PowerToys/blob/main/.pipelines/release.yml) - [ ] **Documentation updated:** If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/windows-uwp/tree/docs/hub/powertoys) and link it here: #xxx ## Detailed Description of the Pull Request / Additional comments **Before discussing the detail of the fix, I'd like to acknowledge the incredible coincidence that it resolves two issues (31950 and 41950) _which are exactly 10,000 issues (and 18 months) apart_.** Now, back to the issue at hand, if you'll forgive the pun... The original flawed code is here: ```cpp bool replacedSomething = false; if (m_flags & UseRegularExpressions) { replaceTerm = regex_replace(replaceTerm, zeroGroupRegex, L"$1$$$0"); replaceTerm = regex_replace(replaceTerm, otherGroupsRegex, L"$1$0$4"); res = RegexReplaceDispatch[_useBoostLib](source, m_searchTerm, replaceTerm, m_flags & MatchAllOccurrences, !(m_flags & CaseSensitive)); replacedSomething = originalSource != res; } ``` Here `replacedSomething` controls whether the counter variable is incremented later in the code. The problem lies in the assumption that only a replacement result which differs from the original string implies a match. This is incorrect, as a successful match and replace operation can still produce a result which coincides with `originalSource` (the source filename), i.e. `originalSource == res`. The solution is to separate the regex matching from the replacement operation, and to advance the counter when the match is successful irrespective of whether the result is the same as the filename. This is what the fix does: ```cpp bool shouldIncrementCounter = false; if (m_flags & UseRegularExpressions) { replaceTerm = regex_replace(replaceTerm, zeroGroupRegex, L"$1$$$0"); replaceTerm = regex_replace(replaceTerm, otherGroupsRegex, L"$1$0$4"); res = RegexReplaceDispatch[_useBoostLib](source, m_searchTerm, replaceTerm, m_flags & MatchAllOccurrences, !(m_flags & CaseSensitive)); // Use regex search to determine if a match exists. This is the basis for incrementing // the counter. if (_useBoostLib) { boost::wregex pattern(m_searchTerm, boost::wregex::ECMAScript | (!(m_flags & CaseSensitive) ? boost::wregex::icase : boost::wregex::normal)); shouldIncrementCounter = boost::regex_search(sourceToUse, pattern); } else { auto regexFlags = std::wregex::ECMAScript; if (!(m_flags & CaseSensitive)) { regexFlags |= std::wregex::icase; } std::wregex pattern(m_searchTerm, regexFlags); shouldIncrementCounter = std::regex_search(sourceToUse, pattern); } } ``` The `regex_search()` call on both the boost and std paths tests whether the regex pattern can be found in the original filename (`sourceToUse`). `shouldIncrementCounter` tracks whether the counter will be incremented later, renamed from `replacedSomething` to reflect the change in behaviour. ## Validation Steps Performed The fix includes an additional unit test for both the std and boost regex paths. Without the fix, the test fails: Screenshot 2025-09-25 063611 and with the fix, all tests pass: Screenshot 2025-09-25 063749 --- .../powerrename/lib/PowerRenameRegEx.cpp | 40 +++++++++++++------ .../powerrename/unittests/CommonRegExTests.h | 36 +++++++++++++++++ 2 files changed, 64 insertions(+), 12 deletions(-) diff --git a/src/modules/powerrename/lib/PowerRenameRegEx.cpp b/src/modules/powerrename/lib/PowerRenameRegEx.cpp index 76136ff39c..34d3cc5c0c 100644 --- a/src/modules/powerrename/lib/PowerRenameRegEx.cpp +++ b/src/modules/powerrename/lib/PowerRenameRegEx.cpp @@ -154,8 +154,7 @@ HRESULT CPowerRenameRegEx::_OnEnumerateOrRandomizeItemsChanged() std::find_if( m_randomizer.begin(), m_randomizer.end(), - [option](const Randomizer& r) -> bool { return r.options.replaceStrSpan.offset == option.replaceStrSpan.offset; } - )) + [option](const Randomizer& r) -> bool { return r.options.replaceStrSpan.offset == option.replaceStrSpan.offset; })) { // Only add as enumerator if we didn't find a randomizer already at this offset. // Every randomizer will also be a valid enumerator according to the definition of enumerators, which allows any string to mean the default enumerator, so it should be interpreted that the user wanted a randomizer if both were found at the same offset of the replace string. @@ -395,11 +394,8 @@ HRESULT CPowerRenameRegEx::Replace(_In_ PCWSTR source, _Outptr_ PWSTR* result, u } std::wstring sourceToUse; - std::wstring originalSource; sourceToUse.reserve(MAX_PATH); - originalSource.reserve(MAX_PATH); sourceToUse = source; - originalSource = sourceToUse; std::wstring searchTerm(m_searchTerm); std::wstring replaceTerm; @@ -487,27 +483,46 @@ HRESULT CPowerRenameRegEx::Replace(_In_ PCWSTR source, _Outptr_ PWSTR* result, u } } - bool replacedSomething = false; + bool shouldIncrementCounter = false; + const bool isCaseInsensitive = !(m_flags & CaseSensitive); + if (m_flags & UseRegularExpressions) { replaceTerm = regex_replace(replaceTerm, zeroGroupRegex, L"$1$$$0"); replaceTerm = regex_replace(replaceTerm, otherGroupsRegex, L"$1$0$4"); - res = RegexReplaceDispatch[_useBoostLib](source, m_searchTerm, replaceTerm, m_flags & MatchAllOccurrences, !(m_flags & CaseSensitive)); - replacedSomething = originalSource != res; + res = RegexReplaceDispatch[_useBoostLib](source, m_searchTerm, replaceTerm, m_flags & MatchAllOccurrences, isCaseInsensitive); + + // Use regex search to determine if a match exists. This is the basis for incrementing + // the counter. + if (_useBoostLib) + { + boost::wregex pattern(m_searchTerm, boost::wregex::ECMAScript | (isCaseInsensitive ? boost::wregex::icase : boost::wregex::normal)); + shouldIncrementCounter = boost::regex_search(sourceToUse, pattern); + } + else + { + auto regexFlags = std::wregex::ECMAScript; + if (isCaseInsensitive) + { + regexFlags |= std::wregex::icase; + } + std::wregex pattern(m_searchTerm, regexFlags); + shouldIncrementCounter = std::regex_search(sourceToUse, pattern); + } } else { - // Simple search and replace + // Simple search and replace. size_t pos = 0; do { - pos = _Find(sourceToUse, searchTerm, (!(m_flags & CaseSensitive)), pos); + pos = _Find(sourceToUse, searchTerm, isCaseInsensitive, pos); if (pos != std::string::npos) { res = sourceToUse.replace(pos, searchTerm.length(), replaceTerm); pos += replaceTerm.length(); - replacedSomething = true; + shouldIncrementCounter = true; } if (!(m_flags & MatchAllOccurrences)) { @@ -516,7 +531,8 @@ HRESULT CPowerRenameRegEx::Replace(_In_ PCWSTR source, _Outptr_ PWSTR* result, u } while (pos != std::string::npos); } hr = SHStrDup(res.c_str(), result); - if (replacedSomething) + + if (shouldIncrementCounter) enumIndex++; } catch (regex_error e) diff --git a/src/modules/powerrename/unittests/CommonRegExTests.h b/src/modules/powerrename/unittests/CommonRegExTests.h index b1b2b8d731..1b0ad30b92 100644 --- a/src/modules/powerrename/unittests/CommonRegExTests.h +++ b/src/modules/powerrename/unittests/CommonRegExTests.h @@ -611,6 +611,42 @@ TEST_METHOD (VerifyRandomizerRegExAllBackToBack) CoTaskMemFree(result); } +TEST_METHOD(VerifyCounterIncrementsWhenResultIsUnchanged) +{ + CComPtr renameRegEx; + Assert::IsTrue(CPowerRenameRegEx::s_CreateInstance(&renameRegEx) == S_OK); + DWORD flags = EnumerateItems | UseRegularExpressions; + Assert::IsTrue(renameRegEx->PutFlags(flags) == S_OK); + + renameRegEx->PutSearchTerm(L"(.*)"); + renameRegEx->PutReplaceTerm(L"NewFile-${start=1}"); + + PWSTR result = nullptr; + unsigned long index = 0; + + renameRegEx->Replace(L"DocA", &result, index); + Assert::AreEqual(1ul, index, L"Counter should advance to 1 on first match."); + Assert::AreEqual(L"NewFile-1", result, L"First file should be renamed correctly."); + CoTaskMemFree(result); + + renameRegEx->Replace(L"DocB", &result, index); + Assert::AreEqual(2ul, index, L"Counter should advance to 2 on second match."); + Assert::AreEqual(L"NewFile-2", result, L"Second file should be renamed correctly."); + CoTaskMemFree(result); + + // The original term and the replacement are identical. + renameRegEx->Replace(L"NewFile-3", &result, index); + Assert::AreEqual(3ul, index, L"Counter must advance on a match, even if the new name is identical to the old one."); + Assert::AreEqual(L"NewFile-3", result, L"Filename should be unchanged on a coincidental match."); + CoTaskMemFree(result); + + // Test that there wasn't a "stall" in the numbering. + renameRegEx->Replace(L"DocC", &result, index); + Assert::AreEqual(4ul, index, L"Counter should continue sequentially after the coincidental match."); + Assert::AreEqual(L"NewFile-4", result, L"The subsequent file should receive the correct next number."); + CoTaskMemFree(result); +} + #ifndef TESTS_PARTIAL }; }