mirror of
https://github.com/microsoft/PowerToys.git
synced 2026-04-05 10:46:33 +02:00
[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.
<!-- Enter a brief description/summary of your PR here. What does it
fix/what does it change/how was it tested (even manually, if necessary)?
-->
## 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`:
<img width="1002" height="759" alt="image"
src="https://github.com/user-attachments/assets/2766f448-adc3-4fe7-9c13-f4c5505ae1d9"
/>
This fix increments the counter irrespective of these coincidences.
<!-- Please review the items on the PR checklist before submitting-->
## 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
<!-- Provide a more detailed description of the PR, other things fixed,
or any additional comments/features here -->
## 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:
<img width="1509" height="506" alt="Screenshot 2025-09-25 063611"
src="https://github.com/user-attachments/assets/14dbf817-b1d3-456d-80f2-abcd28266b8d"
/>
and with the fix, all tests pass:
<img width="897" height="492" alt="Screenshot 2025-09-25 063749"
src="https://github.com/user-attachments/assets/9a587495-d54c-47d3-bc55-ccc64a805a88"
/>
This commit is contained in:
@@ -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)
|
||||
|
||||
@@ -611,6 +611,42 @@ TEST_METHOD (VerifyRandomizerRegExAllBackToBack)
|
||||
CoTaskMemFree(result);
|
||||
}
|
||||
|
||||
TEST_METHOD(VerifyCounterIncrementsWhenResultIsUnchanged)
|
||||
{
|
||||
CComPtr<IPowerRenameRegEx> 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
|
||||
};
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user