Replace All with empty string fails for multiple hits on same line

[vlakn]

Replace All misses matches when you run it right after Find All with an empty replace string. It only happens when a replaced match is followed closely by another one, usually on the same line.

The bug doesn't show up without Find All, or with a non-empty replace string.

Before:

Expected: all matches are removed.
Actual: some matches stay in the document.

Root cause

Replace All opens a document rewrite session, which turns off redraw in TextViewer. While redraw is off, TextViewer.ViewerState keeps the selection as Position objects that are registered with the document, so they move with every edit.

1. Search All leaves a multi-selection with N regions. ViewerState.connect() registers all N positions.
2. The first find sets a single selection. ViewerState.updateSelection() sees that the number of selections changed and creates new Position objects, but never registers them with the document.
3. Replacing with "" doesn't set a new selection in TextViewer.FindReplaceTarget.replaceSelection(). getSelection() then returns the old match length instead of 0. The search continues at offset + length and skips any match inside that range.

With a single selection, the existing position gets updated in place and stays tracked. With a non-empty replace string, replaceSelection() sets the selection explicitly. That's why both of those cases work.

Fix

ViewerState.updateSelection() now removes the old positions from the document and registers the new ones whenever the number of selections changes. The selection keeps following document changes during a rewrite session, and old positions no longer stay registered with the document.

After:

[github-actions]

Test Results

   867 files  ±0     867 suites  ±0   51m 48s ⏱️ - 2m 3s
 8 465 tests +2   8 223 ✅ +2  242 💤 ±0  0 ❌ ±0 
21 273 runs  +6  20 590 ✅ +6  683 💤 ±0  0 ❌ ±0 

Results for commit cb6c820. ± Comparison against base commit 2738b40.

♻️ This comment has been updated with latest results.

[HeikoKlare]

Note: we should check whether/how this may be related to:

• Fix regex Replace All rematching line-start anchors #4373

[vlakn]

@HeikoKlare thanks for the pointer. I checked how the two PRs relate.

With #4373 applied, the reported symptom is gone: Find All followed by Replace All with an empty replace string now removes all matches, including several matches on the same line.

Why #4373 hides the bug: The old replaceAll() loop used the selection after each replacement as the offset for the next search. Both bugs come from that:

• Regular expression replacement with an anchor improperly recurses on each line #2820: the next search starts at a position where ^ matches again, so too much is replaced.
• This PR: the stale selection length moves the next search too far, so matches are skipped.

#4373 collects all matches first and replaces them from last to first at stored offsets. The selection no longer drives the search, so Replace All itself produces the correct document.

The root cause in the viewer remains: TextViewer.ViewerState.updateSelection() creates new Position objects when the number of selections changes while redraw is disabled, but doesn't register them with the document. They stop following document changes. When the rewrite session ends, ViewerState.restore() applies that stale selection.

I reproduced this manually with #4373 applied and without this PR:

1. Open a text editor with foo bar foo baz foo.
2. Open the Find/Replace overlay, search for foo, and click Find All.
3. Leave the replace field empty and click Replace All.

Expected: The document reads bar baz and the caret sits at offset 0 with no selection.
Actual: The document is correct, but the editor selects ba, the first three characters, matching the length of foo.

Without Find All, or with a non-empty replace string, the selection is correct.

I'd therefore keep both changes.

[Copilot]

🟢 Approval recommended

The focused fix addresses the position-tracking defect and includes representative regression coverage.

0 open findings

What changed in this PR

Fixes Replace All with empty replacements after Find All by keeping selection positions document-tracked during rewrite sessions.

Changes:

• Re-registers selection positions when selection count changes.
• Adds regression coverage for adjacent matches and empty replacements.

File	Description
TextViewer.java	Synchronizes replacement selection positions with document tracking.
MultiSelectionTest.java	Tests empty Replace All after multi-selection.

🧠 Review effort: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[HeikoKlare]

Thank you for proposing this fix. The change looks sound to me and fixes the described issue. I also agree that it should be processed in addition to the referenced other fix that would simply hide the underlying issue addressed here.

I have one comment on a possible extension of the fix and another on the test design.

Beyond that, the commit is currently lacking a proper commit message. It only contains a headline (that accidentally reaches into the body). Please add a descriptive commit message body that explain the original issue and the overall concept of the fix.

[HeikoKlare]

Should this do a replaceTrackedSelection() as well, so that the current positions are removed just like in the replacement case?

[vlakn]

I looked into this, and the null branch is never reached. updateSelection() has a single caller, setSelectedRanges(), which passes Arrays.stream(...).toArray(Position[]::new). That's never null, at most an empty array, and an empty array already goes through replaceTrackedSelections() in the else branch.

ViewerState is a private inner class of TextViewer, so this isn't public API, and no other caller can pass null.

I'd therefore suggest to keep this PR focused on the fix and remove the unnecessary null guard in a follow-up PR.

[vlakn]

Thanks for the review, @HeikoKlare!
I addressed both points and reworded the commit message. Please take another look.