BREAKING CHANGE: Fixes #5366. Make View.Text notifications CWP-compliant and non-virtual#5371
Open
tig wants to merge 12 commits into
Open
BREAKING CHANGE: Fixes #5366. Make View.Text notifications CWP-compliant and non-virtual#5371tig wants to merge 12 commits into
tig wants to merge 12 commits into
Conversation
Add signal-only CWP-compliant text-change workflow to View.Text: - Add protected virtual bool OnTextChanging() pre-change hook - Add public event EventHandler<CancelEventArgs>? TextChanging - Refactor OnTextChanged() from public void to protected virtual void (raises TextChanged event; subclasses can override) - Text setter now follows CWP flow: early-return on same value, OnTextChanging, TextChanging event, mutate, OnTextChanged - Canceling TextChanging prevents text mutation and suppresses TextChanged - Add 'new' keyword to TextField.TextChanging (intentional hiding with richer ResultEventArgs<string> semantics) - Add comprehensive unit tests for CWP behavior 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: c316b31a44
ℹ️ 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".
… CWP events Remove �irtual from View.Text property and add SetTextDirect() for derived views to update base text without re-entering CWP flow. Migration patterns: - Simple views (Label, Button, CheckBox, TitleView, Code, Shortcut): Override OnTextChanged() to sync with internal models. - Domain adapters (DatePicker, ColorPicker, ProgressBar, ArrangerButton, LinearRangeViewBase): Override OnTextChanged() for parsing + SetTextDirect() for reverse sync. - Markdown views: Override OnTextChanged() + UpdateTextFormatterText() to suppress raw text rendering. - TextField/TextView: Use 'new' keyword (independent storage) + SetTextDirect() to keep base in sync. - TextValidateField: Override OnTextChanged() + SetTextDirect() after provider transforms. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
tig
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ad0d8a869
ℹ️ 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".
- ReactiveExample: Replace .Events().TextChanged on TextField with Observable.FromEventPattern to avoid ReactiveMarbles source generator producing conflicting handler code for the new/base TextChanging event pair (CS0123) - DateEditor/TimeEditor: Fix CS1574 by changing cref from TextValidateField.Text to View.Text Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…llback 1. OnTextChanging() now raises TextChanging event (CWP: virtual raises event) 2. TextField: override OnTextChanged() to sync internal _text from base.Text when set polymorphically via a View reference 3. TextView: override OnTextChanged() to sync internal TextModel from base.Text when set polymorphically via a View reference 4. ProgressBar: initialize Text to '0%' in constructor so SimplePlusPercentage renders on first draw Added 4 regression tests (verified failing before fixes, passing after). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
TextField's ew Text setter now calls �ase.OnTextChanging() after its own validation passes (ValueChanging, TextField.TextChanging), so subscribers holding a View reference see the pre-change notification and can cancel. TextView's ew Text setter now calls OnTextChanging() at the start, so View.TextChanging is raised before the model is modified. This ensures both controls participate in the base-level CWP flow when their Text property is set directly (not just polymorphically). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
TextField now uses View.Text directly instead of hiding it with 'new'. This fixes the polymorphism issue: setting Text through a View reference now correctly goes through TextField's validation and model sync. Changes: - View.OnTextChanging now accepts 'string proposedValue' so overrides can validate the incoming text - TextField overrides OnTextChanging to sanitize (strip tabs/newlines), fire IValue<string>.ValueChanging, and fire TextField.TextChanging - TextField overrides OnTextChanged to sync the internal grapheme-list model, track history, fire ValueChanged, and adjust cursor - ProgressBar.OnTextChanging updated for new signature - TextView.OnTextChanging call updated for new signature Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The event is now raised directly inside the CWP override, as intended. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replaces direct field and UI updates in the View.Text property setter with a call to SetTextDirect(value). This centralizes text update logic, improving maintainability and reducing code duplication.
Reset TextField.Text.cs to original member order and apply only semantic changes (CWP overrides, remove RaiseTextChanging, shadow TextChanging event). Revert cosmetic change in TextField.Mouse.cs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements the Cancellable Work Pattern (CWP) for View.Text notifications (adding cancellable TextChanging, making TextChanged part of a CWP flow), and makes View.Text non-virtual by migrating existing overrides to OnTextChanged/OnTextChanging + SetTextDirect.
Changes:
- Added CWP-compliant
TextChanging(cancellable) and refactoredTextChangedraising to follow CWP hooks inView.Text. - Removed
virtualfromView.Text, introducingSetTextDirectand migrating derived views (including Markdown, ProgressBar, and text input views). - Added new unit tests covering base CWP behavior and several regression scenarios (including polymorphic
View.Textsets onTextField/TextView).
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/UnitTestsParallelizable/ViewBase/TextCwpTests.cs | Adds unit tests for View.Text CWP semantics and regressions. |
| Tests/UnitTestsParallelizable/ViewBase/Keyboard/KeyboardEventTests.cs | Updates a test helper to hide (not override) Text after View.Text becomes non-virtual. |
| Terminal.Gui/Views/TextInput/TimeEditor.cs | Fixes XML doc reference to View.Text. |
| Terminal.Gui/Views/TextInput/TextView/TextView.Text.cs | Migrates TextView.Text to new + uses CWP hooks and sync behavior. |
| Terminal.Gui/Views/TextInput/TextView/TextView.cs | Initializes TextView model/history to a consistent empty state. |
| Terminal.Gui/Views/TextInput/TextValidateField.cs | Migrates TextValidateField from overriding Text to syncing via OnTextChanged/SetTextDirect. |
| Terminal.Gui/Views/TextInput/TextField/TextField.Text.cs | Refactors TextField text workflow to use base View.Text CWP with internal model syncing. |
| Terminal.Gui/Views/TextInput/TextField/TextField.Mouse.cs | Removes unused return value from a helper call. |
| Terminal.Gui/Views/TextInput/DateEditor.cs | Fixes XML doc reference to View.Text. |
| Terminal.Gui/Views/Shortcut.cs | Migrates help text propagation from overriding Text to OnTextChanged + SetTextDirect. |
| Terminal.Gui/Views/ProgressBar.cs | Keeps Text synced with percentage and restricts external Text changes via OnTextChanging. |
| Terminal.Gui/Views/Markdown/MarkdownTable.cs | Migrates table parsing/sync to OnTextChanged and keeps Text aligned with TableData. |
| Terminal.Gui/Views/Markdown/MarkdownCodeBlock.cs | Migrates parsing to OnTextChanged and prevents raw text rendering via formatter clearing. |
| Terminal.Gui/Views/Markdown/Markdown.cs | Migrates markdown updates to OnTextChanged and prevents raw text rendering via formatter clearing. |
| Terminal.Gui/Views/LinearRange/LinearRangeViewBase.cs | Migrates CSV parsing to OnTextChanged and keeps Text synced with Options. |
| Terminal.Gui/Views/Label.cs | Migrates Text/Title coupling to OnTextChanged + SetTextDirect. |
| Terminal.Gui/Views/DatePicker.cs | Migrates value parsing to OnTextChanged and syncs formatted text via SetTextDirect. |
| Terminal.Gui/Views/Color/ColorPicker.cs | Migrates color parsing to OnTextChanged and syncs formatted text via SetTextDirect. |
| Terminal.Gui/Views/Code.cs | Migrates styled-line updates to OnTextChanged. |
| Terminal.Gui/Views/CheckBox.cs | Migrates Text/Title coupling to OnTextChanged + SetTextDirect. |
| Terminal.Gui/Views/Button.cs | Migrates Text/Title coupling to OnTextChanged + SetTextDirect. |
| Terminal.Gui/ViewBase/View.Text.cs | Implements TextChanging, makes Text non-virtual, adds SetTextDirect and RaiseTextChanged. |
| Terminal.Gui/ViewBase/Adornment/TitleView.cs | Migrates Text/Title coupling to OnTextChanged. |
| Terminal.Gui/ViewBase/Adornment/ArrangerButton.cs | Removes Text override; derives glyph text via SetTextDirect on ButtonType changes. |
| Examples/ReactiveExample/LoginView.cs | Updates Rx subscriptions to TextChanged using FromEventPattern. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f53a59ea1
ℹ️ 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".
…Changed - TextValidateField: move ValueChanging cancellation from OnTextChanged to OnTextChanging so rejected edits never reach View.Text - DatePicker: add OnTextChanging override rejecting unparseable dates - ColorPicker: add OnTextChanging override rejecting unparseable colors - TextField: clear _pendingText when base.OnTextChanging cancels - TextView: add _ownSetterActive flag to skip redundant model sync - View.Text: change RaiseTextChanged from internal to protected internal - Add 5 regression tests in TextCwpTests Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
BDisp
approved these changes
May 23, 2026
Collaborator
BDisp
left a comment
BDisp
left a comment
There was a problem hiding this comment.
That sounds good to me. However, it's necessary to ensure consistency between the altered text value and any other property that might be directly linked to it, such as converting that property's value to match the string value. To ensure this, unit tests in these cases simply need to guarantee the correct value based on whether or not the value of that property is accepted and synchronized with the text.
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
Makes
View.Textnotifications follow the Cancellable Work Pattern (CWP) and removesvirtualfrom theTextproperty, migrating all 17 subclass overrides.Changes
CWP Events (Commit 1)
TextChanging(EventHandler<CancelEventArgs>?): Cancellable pre-change event (signal-only, no text values in args per issue spec)TextChanged(EventHandler?): Post-change notification (unchanged signature, now raised via CWP flow)OnTextChanging()(protected virtual bool): Override point; returntrueto cancelOnTextChanged()(protected virtual void): Override point for post-change logicNon-Virtual Migration (Commit 2)
virtualfromView.TextpropertySetTextDirect(string value)for derived views to update base_textwithout re-entering CWPOnTextChanged()overrideOnTextChanged()+SetTextDirect()OnTextChanged()+UpdateTextFormatterText()overridenewkeyword (independent storage) +SetTextDirect()OnTextChanged()+SetTextDirect()after provider transformsBreaking Changes
View.Textis no longervirtual— subclasses cannot override itOnTextChanged()changed frompublic voidtoprotected virtual voidTextChangingevent may cause unexpected cancellation if subscribedTesting
Fixes #5366