Conversation
…PI-break port Temporarily pin to Terminal.Gui 2.5.0-tig-remove-cm-followup.2 from tui-cs/Terminal.Gui#5416 (built from 5416 head e8aba423) via a vendored nupkg in local_packages/, until 2.5.x ships on nuget.org (pattern: tig/winprint#272). Part of the v2.5.0 ecosystem-validation gate (tui-cs/Terminal.Gui#5630). Port for 2.5.0 breaking changes: - View.Text is no longer virtual (CWP, TG #5371): Editor.Text becomes a `new` property mirroring TextView's pattern — setter raises TextChanging, writes the Document, syncs base via SetTextDirect, and raises TextChanged; OnTextChanged override syncs the Document when Text is set through a base View reference. OnDrawingText suppresses the base text pass (Editor owns rendering). - ConfigurationManager/[ConfigurationProperty]/AppSettingsScope deleted: drop the legacy CM attributes from ted's EditorSettings (MEC is the only read path) and rewrite ConfigTests against TuiConfigurationBuilder with the nested "EditorSettings" RuntimeConfig shape. - Popover is now a screen-filling transparent overlay positioning its ContentView: completion popup hit-testing uses the ListView's screen frame, and mouse-driven Accept is handled from the ListView's Accepted event (clicks no longer reach Editor.OnMouseEvent); tests assert against ContentView geometry and pin the driver size (ANSI driver now sizes asynchronously behind the startup gate). - TG installs its main-loop SynchronizationContext at Init (TG #5588): ted's sync-over-async bridges (OpenFile/SaveFile/SaveFileAs and the startup file load) now clear the ambient context while blocking so continuations run on the thread pool as before (no deadlock). - Selector width change: FileMenu_Shortcuts_Snapshot golden regenerated (theme dropdown pads its label to a fixed width in 2.5). - MEC package floors raised to 10.0.11 to satisfy Terminal.Gui 2.5.0. Known TG 2.5 regression (test skipped, to file upstream): AnsiInputProcessor's 50ms printable-suppression window swallows a real Tab arriving within 50ms of a parsed Shift+Tab (ESC[Z). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BVbLXNzbus2MTXmiJDBErL
![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: 54b4efb850
ℹ️ 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".
| // Keep base View._text in sync so a polymorphic getter sees the same value. | ||
| SetTextDirect (value); |
There was a problem hiding this comment.
The mirror is updated only when this new setter is used. If text changes through normal typing, editor.Document.Text, or replacement of Document, neither OnDocumentChanged nor the Document setter calls SetTextDirect, so ((View)editor).Text continues returning the previous value (often the initial empty string). Before this commit the virtual override always returned the live document text, so generic consumers holding the editor as a View now observe stale content; update the mirror on every document change and document swap.
Useful? React with 👍 / 👎.
| // Raise View.TextChanging so subscribers holding a View reference can cancel. | ||
| if (OnTextChanging (value)) |
There was a problem hiding this comment.
When editor.Text is assigned its current value, this setter invokes OnTextChanging, replaces the entire document, and raises TextChanged. This violates the inherited View.Text contract that identical assignments raise neither event, so CWP subscribers now receive a change that did not occur and the document unnecessarily records a full replacement. Compare against the current text before starting the notification workflow.
Useful? React with 👍 / 👎.
| _ownTextSetterActive = true; | ||
| RaiseTextChanged (); | ||
| _ownTextSetterActive = false; |
There was a problem hiding this comment.
If an overridden OnTextChanged or any TextChanged subscriber throws from RaiseTextChanged, _ownTextSetterActive remains true. Subsequent assignments through a View reference then take the early branch in OnTextChanged and stop synchronizing the document, leaving the editor permanently inconsistent until another successful assignment through Editor.Text; reset this guard in a finally block.
Useful? React with 👍 / 👎.
Code review of the tg25 port found the `new Text` setter cleared its _ownTextSetterActive re-entrancy flag outside a finally: a TextChanged subscriber that throws left the flag stuck true, silently disabling Document sync for every later polymorphic (base View reference) Text set. Reset the flag in a finally. (TextView upstream has the same shape — worth fixing there too.) Adds EditorTextCwpTests covering the full CWP contract of the property- hiding pattern both ways: direct and base-reference set/get round-trips, TextChanging/TextChanged raised exactly once per path, cancellation leaving Document and mirror untouched, and the throwing-subscriber regression (written first; failed before the fix). Also stamps the skipped Tab-after-Shift+Tab test with the now-filed upstream issue tui-cs/Terminal.Gui#5638. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BVbLXNzbus2MTXmiJDBErL
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84c597a40a
ℹ️ 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 (_ownTextSetterActive) | ||
| { | ||
| base.OnTextChanged (); | ||
|
|
||
| return; |
There was a problem hiding this comment.
When a TextChanged subscriber triggered by editor.Text = "first" reentrantly assigns a different value through a View reference, _ownTextSetterActive is still true, so this branch skips copying that nested base value into the document. The nested setter then raises TextChanged while ((View)editor).Text contains the new value but editor.Text/Document.Text still contains the outer value, leaving the two models inconsistent; suppress the sync only when the base value already matches the document rather than for every callback during the outer setter.
Useful? React with 👍 / 👎.
1 participant
This PR pins the repo to a vendored pre-release —
Terminal.Gui 2.5.0-tig-remove-cm-followup.2, a nupkg built from tui-cs/Terminal.Gui#5416 (heade8aba423, sha embedded inInformationalVersion) and committed underlocal_packages/. Before merging:<TerminalGuiVersion>inDirectory.Build.props(the only central pin — every csproj references$(TerminalGuiVersion)).local_packages/and thetgui-pr-5416package source innuget.config(delete the file if nothing else was added to it)..gitignoreexception (!local_packages/*.nupkg/!local_packages/*.snupkgunder the# NuGetblock).dotnet restore+ build + all three CI test suites and confirm Terminal.Gui restores from nuget.org.What this is
Part of the Terminal.Gui v2.5.0 ecosystem-validation gate (tui-cs/Terminal.Gui#5630). tui-cs/Terminal.Gui#5416 (ConfigurationManager removal) is not yet merged/published, so this PR vendors a nupkg built from that PR branch and ports Editor/ted to the 2.5.0 API — the same TEMP-vendoring pattern as tig/winprint#272.
API breaks fixed
View.Textis no longer virtual (CWP-compliant, Fixes #5366 - BREAKING CHANGE - Make View.Text notifications CWP-compliant and non-virtual Terminal.Gui#5371).Editor.Textwaspublic override; it is now anewproperty followingTextView's pattern: the setter raisesTextChanging(cancellable), writes theDocument, mirrors into baseViewviaSetTextDirect, and raisesTextChanged; anOnTextChangedoverride syncs theDocumentwhenTextis set through a baseViewreference.Editoralso overridesOnDrawingTextto suppress the base text pass — the Editor owns all content rendering.ConfigurationManager/[ConfigurationProperty]/AppSettingsScope/ConfigLocationsare deleted. ted'sEditorSettingsdrops the legacy CM attributes (Microsoft.Extensions.Configuration viaTuiConfigurationBuilderwas already the primary read path); theConfigTestsproject's CM end-to-end test is rewritten againstTuiConfigurationBuilder+RuntimeConfigwith the nested"EditorSettings"shape.Popoveris now a screen-filling transparent overlay that positions itsContentView. The completion popup's mouse hit-testing now uses theListView's screen frame (not the popover's), and mouse-driven accept is handled from theListView.Acceptedevent (MouseBindingcontext) because popup clicks route to the ListView and never reachEditor.OnMouseEventanymore. Unit tests assert againstContentViewgeometry.Driver.SetScreenSize (…)afterInit(the patternAppFixturealready used).SynchronizationContextatInit(Fixes #5579. SynchronizationContext is not correctly implemented in v2 Terminal.Gui#5588). ted's sync-over-async bridges (OpenFile/SaveFile/SaveFileAs, plus the startup file load inProgram.cs) deadlocked: awaits posted continuations to the very thread being blocked. They now clear the ambient context while blocking (RunSyncBridge), restoring pre-2.5 thread-pool continuation behavior while keepingTextDocumentowner-thread handoff on the calling thread.FileMenu_Shortcuts_SnapshotANSI golden is regenerated (only that glyph run changed).Known Terminal.Gui 2.5 regression (skipped test — filed as tui-cs/Terminal.Gui#5638)
EditorTabTests.RawAnsi_Tab_After_ShiftTab_Reindents_Line_On_First_Keypressis skipped with a documented reason:AnsiInputProcessor's 50 ms printable-suppression window (dedup of dual-reported keys) swallows a real\tarriving within 50 ms of a parsed Shift+Tab (ESC[Z) —GetPrintableText ()is"\t"for both Tab and Shift+Tab. Filed as tui-cs/Terminal.Gui#5638 (the skip annotation references it); re-enable the test when fixed.Test results
Terminal.Gui.Editor.TestsTerminal.Gui.Editor.IntegrationTestsTerminal.Gui.Editor.ConfigTestsPost-review:
EditorTextCwpTests(8 tests) locks thenew TextCWP contract both ways — direct and base-View-reference set/get round-trips, single-fire TextChanging/TextChanged, cancellation — and a review fix hardens the setter's re-entrancy flag with afinally(a throwingTextChangedsubscriber previously disabled Document sync for all later polymorphic sets; TextView upstream shares this shape).Debug and Release builds clean;
.claude/hooks/cleanup-cs.ps1(dotnet format + jb cleanupcode) run.Follow-up
RawAnsi_Tab_After_ShiftTab_Reindents_Line_On_First_Keypresswhen ANSI printable-suppression window swallows real Tab within 50ms of Shift+Tab Terminal.Gui#5638 is fixed.Refs: tui-cs/Terminal.Gui#5416, tui-cs/Terminal.Gui#5630. Vendoring pattern: tig/winprint#272.
🤖 Generated with Claude Code
https://claude.ai/code/session_01BVbLXNzbus2MTXmiJDBErL