Fix Replace & find next silently skipping current match#11399
Merged
Conversation
The Replace button on the Edit > Replace dialog (which acts as "Replace and find next") read EditTextBox.SelectedText / EditTextBox.SelectionStart to figure out what to replace and where. Two failure modes followed: 1. If the EditTextBox selection didn't exactly equal the last-found text (focus changes, async TextBox updates, normalized line endings shifting indices on multi-line subtitles, etc.), the `selectedText == savedCurrentTextFound` guard failed, the entire replacement block was skipped, and FindNext still ran — advancing the cursor to the next match without replacing. 2. The non-regex path wrote `EditTextBox.SelectedText = ReplaceText`, which when the selection had collapsed just inserted at the caret instead of replacing. Drive the replace path from the FindService's saved match (line index, text index, found text) instead. These are captured before Initialize wipes them and are the authoritative description of the match the user just navigated to. The replacement is applied to the underlying subtitle text (`Subtitles[line].Text`), and `FindNext` resumes from the position immediately after the replacement. The new code also guards both modes: - For regex it re-runs the match at the saved index and bails if it doesn't start exactly there (so an in-flight edit can't be corrupted). - For non-regex it verifies the slice at the saved index still equals the matched text under the configured case mode before mutating. Fixes #11388 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes issue #11388 where Replace & find next could skip the current match (without replacing) due to relying on EditTextBox selection state instead of the last saved find result.
Changes:
- Drives single-replace from
_findService’s saved match (CurrentLineNumber/CurrentTextIndex/CurrentTextFound) captured before_findService.Initialize(...)resets state. - Applies the replacement directly to the underlying subtitle line (
Subtitles[line].Text) and resumesFindNextfrom immediately after the replacement. - Adds defensive “match still valid” checks for non-regex (and attempts similar for regex) to avoid corrupting text if the subtitle was edited between Find and Replace.
Comment on lines
+9796
to
+9818
| if (result.FindMode == FindMode.RegularExpression) | ||
| { | ||
| try | ||
| { | ||
| var fixedReplaceText = RegexUtils.FixNewLine(result.ReplaceText); | ||
| var regex = new Regex(result.SearchText); | ||
| var match = regex.Match(line, matchIndex); | ||
| if (!match.Success || match.Index != matchIndex) | ||
| { | ||
| newLine = line; | ||
| return null; | ||
| } | ||
|
|
||
| var replaced = match.Result(fixedReplaceText); | ||
| newLine = line.Substring(0, matchIndex) + replaced + line.Substring(matchIndex + match.Length); | ||
| return replaced.Length; | ||
| } | ||
| catch (ArgumentException) | ||
| { | ||
| newLine = line; | ||
| return null; | ||
| } | ||
| } |
Addresses Copilot review feedback on #11399: the regex replace path re-ran the pattern at the saved match index and only checked that the new match started at that exact offset. Two gaps: 1. If the subtitle was edited between Find and Replace, a different substring of the same length could match at the same offset and be silently replaced. 2. The FindService normalizes CRLF -> LF before regex matching and maps the index back; the re-match here runs against the un-normalized line, so for line-ending-sensitive patterns the indices can drift even when the same logical match still exists. Compare match.Value (ordinal) to the saved found text and bail on mismatch. The non-regex path already had an equivalent check. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #11388. In the Edit → Replace dialog, the Replace button (which acts as "Replace & find next") was driven by
EditTextBox.SelectedTextandEditTextBox.SelectionStart. When that UI state didn't perfectly match the last find result — focus changes, line-ending normalization on a multi-line subtitle, an in-flight TextBox text update — the guardselectedText == savedCurrentTextFoundfailed, the replacement block was skipped entirely, andFindNextstill ran and advanced past the unmodified match. The user's reproducer (regex([?!\.,])→$1againstcoming ! Look!) hits exactly this path.What changed
HandleReplaceResultnow drives the replace path from the FindService's saved match —CurrentLineNumber/CurrentTextIndex/CurrentTextFound, captured beforeInitializewipes them. The replacement is applied to the underlying subtitle text (Subtitles[line].Text), andFindNextresumes from the position immediately after the replacement.Both modes also defensively verify the saved match still describes what's actually in the line, so an in-flight edit to the subtitle can't be corrupted:
savedFoundIndexand bails unlessmatch.Success && match.Index == savedFoundIndex.match.Result(replacement)properly expands$1/$2/etc.line.Substring(savedFoundIndex, savedFoundText.Length)to the saved match under the configured case-sensitivity mode and bails on mismatch.Side benefit: the non-regex path previously did
EditTextBox.SelectedText = ReplaceText, which when the selection had collapsed just inserted at the caret instead of replacing. That class of bug goes away too.Test plan
([?!\\.,])→\$1, ensure case-insensitive regex mode. Click Find next, then Replace. Confirm the space is removed and the cursor advances.🤖 Generated with Claude Code