find-and-replace: ISearchStrategy + ted toggles (stacked on #76)#79
Conversation
Replace Editor.FindReplace.cs's bespoke document.Text.IndexOf engine with the lifted ISearchStrategy seam from PR #76. The string-based overloads (FindNext/FindPrevious/ReplaceNext/ReplaceAll taking a search-text string) remain as convenience wrappers: each builds a SearchMode.Normal strategy from its arguments and delegates to the no-argument property-driven overloads. Callers that want regex, whole-word, or wildcard mode assign their own ISearchStrategy to Editor.SearchStrategy (typically constructed via SearchStrategyFactory.Create) and then call the no-arg overloads. Why the engine swap is worth doing alone: - ReplaceAll on N matches now does ONE document.Text materialization (via SearchStrategy.FindAll) instead of N (one per IndexOf call in the legacy loop). Replacements iterate in reverse so offsets stay valid as later matches shrink/grow the document. Both paths still use Document.RunUpdate() for single-step undo (R5). - ReplaceNext routes through ISearchResult.ReplaceWith, picking up regex $1/$2 backreference substitution for free. - FindPrevious is now real backwards search via FindAll over [0, end) and LastOrDefault, replacing the LastIndexOf-with-clamped-start approach that mis-handled cursor-inside-a-match. Quick-find Stopwatch microbench (benchmarks/.../Program.cs --quick-find, DocSize=100KB): Matches New (ms) Old (ms) Speedup Memory ratio 10 0.53 0.58 ~equal 4.5x less 100 0.73 2.52 3.5x faster 32x less 1000 3.55 14.84 4.2x faster 103x less FindNext is roughly equal between engines (both materialize once); the win is concentrated in ReplaceAll where the legacy path's per- iteration rope materialization dominates GC pressure. specs/find-and-replace/spec.md updated to mark FR-001/002/004 done and FR-003 (SearchHitRenderer) / FR-005 (F3 / Ctrl+F / Ctrl+H keybindings) / FR-006 (hit-highlight invalidation) explicitly deferred to a follow-up slice. specs/public-api.md change log records Editor.SearchStrategy landing. 10 new tests in EditorFindReplaceTests.cs cover: property round-trip, FindNext returning false when strategy not set, regex match through Editor, whole-word match through Editor, FindPrevious finds the rightmost match before the caret, FindPrevious wraparound, regex backreference substitution via ReplaceNext, ReplaceAll via regex strategy, R5 single-step undo with regex, reverse-iteration safety when replacement length differs from match length. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add three CheckBox controls above the Find/Replace tab pane that together build the Editor.SearchStrategy for each Find / Replace action. The handlers route through a single TryBuildStrategy helper that constructs SearchStrategyFactory.Create from the toggle state, assigns to editor.SearchStrategy, and catches SearchPatternException to surface invalid-regex errors on a status label rather than silently failing. This is the ted-facing payoff for the engine swap in the previous commit and the R9 amendment in PR #76: a customer (the user of ted) can now exercise the new ISearchStrategy capabilities — regex \d{3}, whole-word "cat", case-sensitive match — without writing code. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two new benchmark paths in the benchmarks project: - FindBenchmarks.cs: BenchmarkDotNet [ShortRunJob] [MemoryDiagnoser] comparison of FindNext (new vs legacy) and ReplaceAll (new vs legacy) for the engine itself — no Editor wrapper, no event handlers, no visual-line caches. The Editor's per-edit notification cost is real but separate, and would apply to both engines once landed (PR #77 already fixes Editor's all-lines-walk on Document swap incrementally). - Program.cs --quick-find: lightweight Stopwatch microbench that prints the new-vs-old comparison in ~10 seconds. BDN's full statistical rigor is overkill for "is the new path faster or slower?" and adds 5-10 minutes per run due to sub-process spawns and pilot phases for the slow ReplaceAll cases. Sample output (DocSize=100KB, 20 iterations): Matches New (ms) Old (ms) Speedup Memory ratio 10 0.53 0.58 ~equal 4.5x less 100 0.73 2.52 3.5x faster 32x less 1000 3.55 14.84 4.2x faster 103x less The wart this lift fixed: the legacy engine materialized document.Text once per IndexOf call inside the ReplaceAll loop (N rope materializations at 100KB each = 191MB allocated for 1000 matches). The new engine calls FindAll once, materializes once, and iterates the cached result list in reverse. Per-call FindNext cost is roughly equal between engines — the incremental-search per- keystroke materialization remains and would need a rope-walking matcher to truly fix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3fa012ea2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| SelectSearchMatch (match.Offset, match.Length); | ||
|
|
||
| return true; |
There was a problem hiding this comment.
Regex mode can legitimately produce zero-length results (for example ^, $, or \b). Here FindNext reports success after SelectSearchMatch, but zero-length matches are ignored by selection logic, so caret/selection do not move; repeated Find/Replace operations then keep targeting the same position (and ReplaceNext cannot act on it). This makes a class of valid regex searches unusable in the new strategy-driven path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: edcc58027c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var startOffset = HasSelection ? SelectionStart - 1 : CaretOffset - 1; | ||
| var matchOffset = FindBackwardOffset (searchText, startOffset, matchCase); | ||
| var endOffset = HasSelection ? SelectionStart : CaretOffset; | ||
| ISearchResult? match = FindBackward (0, endOffset); |
There was a problem hiding this comment.
FindPrevious now searches with FindBackward(0, endOffset), which limits matches to those fully contained in [0, endOffset). Because RegexSearchStrategy.FindAll drops any result whose end offset exceeds the range end, a match that starts before the caret but extends past it is ignored. For example, in "foo bar foo" with caret at 9 (inside the second foo), FindPrevious("foo") skips offset 8 and jumps to an earlier/wrapped match, whereas the prior LastIndexOf logic returned the in-progress occurrence. This regresses Shift+F3 behavior whenever the caret is inside a multi-character match.
Useful? React with 👍 / 👎.
1 participant
Summary
Stacked on PR #76 (the AvaloniaEdit search lift). This PR wires the lifted engine into
Editor.FindReplace.csand surfaces the new capabilities through ted'sFindReplaceDialog— the first PR held to the amended constitution R9 (lifts must ship with their ted consumer).Editor.FindReplace.csnow drives all find/replace throughISearchStrategy.Editor.SearchStrategyis the seam. String-basedFindNext/FindPrevious/ReplaceNext/ReplaceAlloverloads are kept as convenience wrappers that build aSearchMode.Normalstrategy and delegate.ISearchStrategyfrom toggle state on every Find / Replace action.FindBenchmarks.cs(BDN, engine-only) +Program.cs --quick-find(Stopwatch, ~10s). The new path is ~4× faster and ~100× less allocation onReplaceAllat N=1000 matches;FindNextis roughly equal between engines.specs/find-and-replace/spec.mdmarks FR-001 / FR-002 / FR-004 + scenarios 1–3 + 6–7 done. FR-003 (SearchHitRenderer) / FR-005 (F3 / Ctrl+F / Ctrl+H keybindings) / FR-006 (hit-highlight invalidation) explicitly deferred to a follow-up slice.specs/public-api.mdchange log recordsEditor.SearchStrategy.Compatibility with PR #77
Verified — the two PRs touch disjoint files in their code changes. PR #77's
Editor.csrefactor (incrementalUpdateContentSize,_maxVisualWidthfield) doesn't intersect withEditor.FindReplace.cs. PR #77'sTestEnvironment.cs[ModuleInitializer]forDisableRealDriverIOis the canonical fix for the integration-test hang I hit during development; I had a redundant version inAppFixturector and backed it out.Quick-find benchmark (DocSize=100 KB, 20 iters)
Run with
dotnet run --project benchmarks/Terminal.Gui.Editor.Benchmarks -c Release --no-launch-profile -- --quick-find.The win comes from replacing N rope materializations (one per
IndexOfcall in the legacy loop) with 1 (oneFindAll).FindNextper-call cost is unchanged — both engines materializedocument.Textonce per call. The earlier "10 MB per keystroke on a 10 MB file" concern is still real; fixing it needs a rope-walking matcher and is out of scope for this lift.Validation
dotnet build Terminal.Gui.Text.slnx— 0 warnings, 0 errorsdotnet format --verify-no-changes— cleanTerminal.Gui.Text.Tests— 224 / 0 failTerminal.Gui.Editor.Tests— 88 / 0 fail (10 new inEditorFindReplaceTests.cs)Terminal.Gui.Editor.IntegrationTests— 105 / 0 fail (withDisableRealDriverIO=1set externally; PR perf: scroll-performance optimizations + cleanup profile fix #77's ModuleInitializer handles this automatically on develop)Test plan
dotnet run --project examples/ted, Ctrl+F, toggle Regex, search\d+, see numbers selected one at a time. Toggle Whole word, searchcatagainst a buffer withcat catalog scatter.() surfaces "Regex error: ..." on the dialog's status label instead of silently no-op.🤖 Generated with Claude Code