Extract automatic folding orchestration from Ted into Editor#199
Conversation
Introduce IFoldingStrategy interface and move automatic folding lifecycle
management into Editor itself. Consumers now enable full brace-folding
with just:
Editor.FoldingStrategy = new BraceFoldingStrategy ();
Key changes:
- New IFoldingStrategy interface (UpdateFoldings + ChangeMayAffectFoldings)
- BraceFoldingStrategy implements IFoldingStrategy with OffsetChangeMap-aware
structural-change detection (moved from Ted's ~170 lines)
- XmlFoldingStrategy implements IFoldingStrategy
- New Editor.Folding.cs partial with FoldingStrategy, AutomaticFolding, and
MaximumAutomaticFoldingDocumentLength properties
- Editor.Document setter calls InstallAutomaticFolding() automatically
- Ted simplified: deleted ~250 lines of folding orchestration, replaced with
single property assignment
- New AutomaticFoldingTests covering setup, teardown, structural filtering,
large-doc threshold, and document replacement
Closes #178
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.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: 1d00458f6e
ℹ️ 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".
| if (Document.TextLength > _maximumAutomaticFoldingDocumentLength) | ||
| { | ||
| SetFoldingDocument (null); | ||
| FoldingManager = null; |
There was a problem hiding this comment.
When the document is over MaximumAutomaticFoldingDocumentLength, this branch tears down folding and unsubscribes from folding-change events, but there is no subsequent path that reinstalls automatic folding when edits shrink the same document back under the limit (normal edits only hit OnDocumentChanged, which does not call InstallAutomaticFolding). In practice, a large file that gets trimmed below the threshold will keep FoldingManager null until callers manually toggle a folding property or replace the document.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Extracts Ted’s automatic folding orchestration into Terminal.Gui.Editor.Editor, introducing a strategy-based API so consumers can enable automatic folding without duplicating Ted-specific wiring.
Changes:
- Added
IFoldingStrategyand updated existing folding strategies (brace/XML) to implement structural-change filtering. - Introduced
Editor-level automatic folding lifecycle orchestration (strategy assignment, document swap, large-document guard, change/update handling). - Simplified Ted by removing its folding orchestration and relying on
Editorconfiguration; added/updated tests for the new behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Terminal.Gui.Editor.Tests/Folding/AutomaticFoldingTests.cs | Adds coverage for automatic folding setup/teardown and change filtering (currently contains an incomplete/compilation-breaking test). |
| src/Terminal.Gui.Editor/Folding/XmlFoldingStrategy.cs | Implements IFoldingStrategy and adds structural-change detection for XML. |
| src/Terminal.Gui.Editor/Folding/IFoldingStrategy.cs | Introduces a strategy interface for folding computation + “change may affect foldings” filtering. |
| src/Terminal.Gui.Editor/Folding/BraceFoldingStrategy.cs | Implements IFoldingStrategy and ports OffsetChangeMap-aware structural-change detection. |
| src/Terminal.Gui.Editor/Editor.Folding.cs | New Editor partial that owns automatic folding orchestration and large-document threshold behavior. |
| src/Terminal.Gui.Editor/Editor.cs | Hooks folding installation on document assignment and unsubscribes folding document events on dispose. |
| examples/ted/TedApp.FileOperations.cs | Removes Ted-side folding install call (Editor now owns it). |
| examples/ted/TedApp.cs | Removes Ted’s folding orchestration and configures folding via Editor.FoldingStrategy. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public void ChangeMayAffectFoldings_Brace_Returns_True_For_Braces () | ||
| { | ||
| BraceFoldingStrategy strategy = new (); | ||
| TextDocument doc = new ("hello"); | ||
|
|
||
| // Insert a brace. | ||
| doc.Insert (5, "{"); | ||
|
|
||
| // Manually create the event args to test the strategy method. | ||
| // We'll test via a document change that triggers the actual path instead. | ||
| } | ||
|
|
||
| [Fact] |
| public int MaximumAutomaticFoldingDocumentLength | ||
| { | ||
| get => _maximumAutomaticFoldingDocumentLength; | ||
| set | ||
| { | ||
| if (_maximumAutomaticFoldingDocumentLength == value) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| _maximumAutomaticFoldingDocumentLength = value; | ||
| InstallAutomaticFolding (); | ||
| } |
| if (_foldingStrategy is null || !_automaticFolding || Document is null) | ||
| { | ||
| SetFoldingDocument (null); | ||
|
|
||
| // Clear FoldingManager only if automatic folding was previously active (strategy set). | ||
| if (_foldingStrategy is not null || _automaticFolding) | ||
| { | ||
| FoldingManager = null; | ||
| } | ||
|
|
| // etc.). The Document setter unsubscribes on swap; this covers View-teardown. | ||
| _document.Changed -= OnDocumentChanged; | ||
| _document.UndoStack.PropertyChanged -= OnUndoStackPropertyChanged; | ||
| SetFoldingDocument (null); |
- Remove unused 'using Terminal.Gui.Editor' from test file (CI fix) - Add validation on MaximumAutomaticFoldingDocumentLength (ThrowIfLessThan 0) - Track FoldingManager ownership (_automaticFoldingOwnsFoldingManager) to avoid wiping consumer-provided FoldingManager when automatic folding is disabled - Clear FoldingManager in Dispose to prevent event handler leaks - Replace empty ChangeMayAffectFoldings test with two proper tests - Fix Large_Document test to use multi-line content (avoids visual line OOM) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12dd02e732
ℹ️ 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".
|
|
||
| FoldingManager fm = new (Document); | ||
| FoldingManager = fm; | ||
| _automaticFoldingOwnsFoldingManager = true; |
There was a problem hiding this comment.
InstallAutomaticFolding marks the manager as owned (_automaticFoldingOwnsFoldingManager = true) whenever auto-folding installs one, but there is no path that clears ownership if a consumer later assigns editor.FoldingManager manually. In that scenario, a subsequent auto-folding state change (e.g., AutomaticFolding = false, strategy unset, or threshold exceeded) will treat the consumer-provided manager as auto-owned and clear it unexpectedly, even though the comment says externally set managers must not be wiped. This regresses manual folding-manager integration after auto-folding has been enabled once.
Useful? React with 👍 / 👎.
- Add explicit 'using System.Text' instead of fully-qualified StringBuilder - Use var for built-in string init - Use target-typed new for StringBuilder Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
When a consumer sets FoldingManager directly after auto-folding has installed one, the ownership flag is now cleared in the setter. This prevents subsequent auto-folding teardown (strategy unset, AutomaticFolding=false, threshold exceeded) from wiping a consumer-provided manager. Added test: External_FoldingManager_Not_Cleared_By_AutomaticFolding_Teardown Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c1b661ba2
ℹ️ 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".
| public bool ChangeMayAffectFoldings (DocumentChangeEventArgs e) | ||
| { | ||
| return ContainsXmlStructuralCharacter (e.InsertedText) | ||
| || ContainsXmlStructuralCharacter (e.RemovedText); |
There was a problem hiding this comment.
ChangeMayAffectFoldings only returns true when inserted/removed text contains <, >, /, or newlines, so edits like renaming a tag (foo→bar) or changing attribute text are treated as non-structural and skip UpdateFoldings. In automatic mode this leaves stale fold regions/titles (and can preserve invalid folds after name mismatches) until some later structural character edit happens. For XML, these plain-text edits can change parse results and folded labels, so they need to trigger a re-scan.
Useful? React with 👍 / 👎.
Summary
Extracts ~250 lines of folding orchestration from
examples/ted/TedApp.csintoEditoritself, implementing Phase 1 of #178.New API on
EditorProperties added:
IFoldingStrategy? FoldingStrategy— when assigned (andAutomaticFoldingis true), creates aFoldingManager, subscribes to document changes, and refreshes foldings on structural editsbool AutomaticFolding— auto-enabled whenFoldingStrategyis first assigned; can be toggled independentlyint MaximumAutomaticFoldingDocumentLength— documents exceeding this threshold skip automatic folding (default: 1,000,000)New Interface
What changed
src/.../Folding/IFoldingStrategy.cssrc/.../Folding/BraceFoldingStrategy.csIFoldingStrategy; absorbs Ted'sOffsetChangeMap-aware structural-change detectionsrc/.../Folding/XmlFoldingStrategy.csIFoldingStrategysrc/.../Editor.Folding.cssrc/.../Editor.csInstallAutomaticFolding()fromDocumentsetter andDisposeexamples/ted/TedApp.csexamples/ted/TedApp.FileOperations.csInstallFolding()call (Editor handles it)tests/.../AutomaticFoldingTests.csTest results
Closes #178