Skip to content

Commit 41a3cc3

Browse files
tigCopilot
andcommitted
Fix CR feedback: move validation to OnTextChanging, protect RaiseTextChanged
- 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>
1 parent 5346be2 commit 41a3cc3

7 files changed

Lines changed: 241 additions & 50 deletions

File tree

Terminal.Gui/ViewBase/View.Text.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,7 @@ protected virtual void OnTextChanged () { }
153153
/// call this method after mutating text to participate in the CWP workflow.
154154
/// </para>
155155
/// </remarks>
156-
internal void RaiseTextChanged ()
156+
protected internal void RaiseTextChanged ()
157157
{
158158
OnTextChanged ();
159159
TextChanged?.Invoke (this, EventArgs.Empty);

Terminal.Gui/Views/Color/ColorPicker.cs

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -152,6 +152,18 @@ protected override bool OnDrawingContent (DrawContext? context)
152152
/// </summary>
153153
protected virtual void OnValueChanged (ValueChangedEventArgs<Color?> args) { }
154154

155+
/// <inheritdoc/>
156+
protected override bool OnTextChanging (string newText)
157+
{
158+
// Reject text that cannot be parsed as a valid color
159+
if (!_colorNameResolver.TryParseColor (newText, out _))
160+
{
161+
return true;
162+
}
163+
164+
return base.OnTextChanging (newText);
165+
}
166+
155167
/// <inheritdoc/>
156168
protected override void OnTextChanged ()
157169
{

Terminal.Gui/Views/DatePicker.cs

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,18 @@ public CultureInfo? Culture
4646
}
4747
}
4848

49+
/// <inheritdoc/>
50+
protected override bool OnTextChanging (string newText)
51+
{
52+
// Reject text that cannot be parsed as a valid DateTime
53+
if (!DateTime.TryParse (newText, out _))
54+
{
55+
return true;
56+
}
57+
58+
return base.OnTextChanging (newText);
59+
}
60+
4961
/// <inheritdoc/>
5062
protected override void OnTextChanged ()
5163
{

Terminal.Gui/Views/TextInput/TextField/TextField.Text.cs

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -437,7 +437,14 @@ protected override bool OnTextChanging (string newText)
437437
// Stash the (possibly subscriber-modified) result for OnTextChanged
438438
_pendingText = args.Result;
439439

440-
return base.OnTextChanging (newText);
440+
if (base.OnTextChanging (newText))
441+
{
442+
_pendingText = null;
443+
444+
return true;
445+
}
446+
447+
return false;
441448
}
442449

443450
/// <inheritdoc/>

Terminal.Gui/Views/TextInput/TextValidateField.cs

Lines changed: 48 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -203,10 +203,52 @@ protected virtual void OnValueChanged (ValueChangedEventArgs<string?> args) { }
203203

204204
#endregion
205205

206+
/// <summary>
207+
/// Stashes the final text value determined during <see cref="OnTextChanging"/> (after possible
208+
/// subscriber modification) for use in <see cref="OnTextChanged"/>.
209+
/// </summary>
210+
private string? _pendingValidatedText;
211+
212+
/// <inheritdoc/>
213+
protected override bool OnTextChanging (string newText)
214+
{
215+
if (_provider is null || SuppressValueEvents)
216+
{
217+
return base.OnTextChanging (newText);
218+
}
219+
220+
string oldValue = _provider.Text;
221+
222+
if (oldValue == newText)
223+
{
224+
return true;
225+
}
226+
227+
ValueChangingEventArgs<string?> args = new (oldValue, newText);
228+
229+
if (OnValueChanging (args) || args.Handled)
230+
{
231+
return true;
232+
}
233+
234+
ValueChanging?.Invoke (this, args);
235+
236+
if (args.Handled)
237+
{
238+
return true;
239+
}
240+
241+
// Allow subscribers to modify the new value
242+
_pendingValidatedText = args.NewValue ?? string.Empty;
243+
244+
return base.OnTextChanging (newText);
245+
}
246+
206247
/// <summary>Text</summary>
207248
protected override void OnTextChanged ()
208249
{
209-
string value = Text;
250+
string value = _pendingValidatedText ?? Text;
251+
_pendingValidatedText = null;
210252

211253
if (_provider is null)
212254
{
@@ -224,35 +266,10 @@ protected override void OnTextChanged ()
224266
return;
225267
}
226268

227-
if (!SuppressValueEvents)
269+
// Apply subscriber-modified value if different from what the base stored
270+
if (value != Text)
228271
{
229-
ValueChangingEventArgs<string?> args = new (oldValue, value);
230-
231-
if (OnValueChanging (args) || args.Handled)
232-
{
233-
// Revert Text to the provider's current value
234-
SetTextDirect (oldValue);
235-
236-
return;
237-
}
238-
239-
ValueChanging?.Invoke (this, args);
240-
241-
if (args.Handled)
242-
{
243-
// Revert Text to the provider's current value
244-
SetTextDirect (oldValue);
245-
246-
return;
247-
}
248-
249-
// Allow subscribers to modify the new value
250-
value = args.NewValue ?? string.Empty;
251-
252-
if (value != Text)
253-
{
254-
SetTextDirect (value);
255-
}
272+
SetTextDirect (value);
256273
}
257274

258275
_lastKnownText = value;
@@ -264,11 +281,10 @@ protected override void OnTextChanged ()
264281

265282
if (!SuppressValueEvents)
266283
{
267-
string oldVal = oldValue;
268-
ValueChangedEventArgs<string?> changedArgs = new (oldVal, _provider.Text);
284+
ValueChangedEventArgs<string?> changedArgs = new (oldValue, _provider.Text);
269285
OnValueChanged (changedArgs);
270286
ValueChanged?.Invoke (this, changedArgs);
271-
ValueChangedUntyped?.Invoke (this, new ValueChangedEventArgs<object?> (oldVal, _provider.Text));
287+
ValueChangedUntyped?.Invoke (this, new ValueChangedEventArgs<object?> (oldValue, _provider.Text));
272288
}
273289

274290
SetNeedsDraw ();

Terminal.Gui/Views/TextInput/TextView/TextView.Text.cs

Lines changed: 24 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -197,41 +197,49 @@ public int TabWidth
197197
// Keep base View._text in sync
198198
SetTextDirect (value);
199199

200+
_ownSetterActive = true;
200201
RaiseTextChanged ();
202+
_ownSetterActive = false;
201203
SetNeedsDraw ();
202204

203205
_historyText.Clear (_model.GetAllLines ());
204206
}
205207
}
206208

209+
/// <summary>Tracks whether the <c>new Text</c> setter is active to avoid redundant sync in <see cref="OnTextChanged"/>.</summary>
210+
private bool _ownSetterActive;
211+
207212
/// <inheritdoc/>
208213
/// <remarks>
209214
/// Syncs the internal <see cref="TextModel"/> when <see cref="View.Text"/> is set through a
210215
/// polymorphic (<see cref="View"/>) reference, ensuring the TextView's editing model stays consistent.
211216
/// </remarks>
212217
protected override void OnTextChanged ()
213218
{
214-
string baseText = base.Text;
215-
216-
// Only sync when the internal model diverges (polymorphic setter case).
217-
// When TextView's own `new Text` setter runs, it already calls LoadString
218-
// and SetTextDirect, so base.Text matches _model — this is a no-op.
219-
if (_model.ToString () != baseText)
219+
// Skip sync when called from our own `new Text` setter — it already updated the model.
220+
if (_ownSetterActive)
220221
{
221-
ResetPosition ();
222-
_model.LoadString (baseText);
222+
base.OnTextChanged ();
223223

224-
if (_wordWrap)
225-
{
226-
_wrapManager = new WordWrapManager (_model);
227-
_model = _wrapManager.WrapModel (Viewport.Width, out _, out _, out _, out _);
228-
_lastWrapWidth = Viewport.Width;
229-
}
224+
return;
225+
}
230226

231-
_historyText.Clear (_model.GetAllLines ());
232-
SetNeedsDraw ();
227+
string baseText = base.Text;
228+
229+
// Sync when the internal model diverges (polymorphic setter case).
230+
ResetPosition ();
231+
_model.LoadString (baseText);
232+
233+
if (_wordWrap)
234+
{
235+
_wrapManager = new WordWrapManager (_model);
236+
_model = _wrapManager.WrapModel (Viewport.Width, out _, out _, out _, out _);
237+
_lastWrapWidth = Viewport.Width;
233238
}
234239

240+
_historyText.Clear (_model.GetAllLines ());
241+
SetNeedsDraw ();
242+
235243
base.OnTextChanged ();
236244
}
237245

Tests/UnitTestsParallelizable/ViewBase/TextCwpTests.cs

Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -227,4 +227,140 @@ protected override bool OnTextChanging (string newText)
227227
return BaseReturnedCancel;
228228
}
229229
}
230+
231+
// --- CR feedback regression tests ---
232+
233+
/// <summary>
234+
/// Verifies that when TextField's TextChanging subscriber modifies text, and then a
235+
/// subsequent base TextChanging event cancels, the stale modified text does not leak
236+
/// into the next successful text change.
237+
/// </summary>
238+
[Fact]
239+
public void TextField_PendingText_ClearedOnBaseCancel ()
240+
{
241+
// Copilot
242+
TextField tf = new () { Width = 20, Height = 1 };
243+
tf.Text = "initial";
244+
245+
// First: a subscriber modifies text via ResultEventArgs
246+
tf.TextChanging += (_, args) =>
247+
{
248+
if (args.Result == "modified")
249+
{
250+
args.Result = "subscriber_changed";
251+
}
252+
};
253+
254+
tf.Text = "modified";
255+
Assert.Equal ("subscriber_changed", tf.Text);
256+
257+
// Now subscribe to base View.TextChanging to cancel the NEXT change
258+
var cancelOnce = true;
259+
260+
((View)tf).TextChanging += (_, e) =>
261+
{
262+
if (cancelOnce)
263+
{
264+
e.Cancel = true;
265+
cancelOnce = false;
266+
}
267+
};
268+
269+
// This should be cancelled by the base event
270+
tf.Text = "blocked";
271+
Assert.Equal ("subscriber_changed", tf.Text);
272+
273+
// Next change should succeed with fresh text, NOT stale _pendingText
274+
tf.Text = "final";
275+
Assert.Equal ("final", tf.Text);
276+
}
277+
278+
/// <summary>
279+
/// Verifies that TextValidateField does not raise View.TextChanged when
280+
/// ValueChanging is cancelled (CWP semantics: cancel suppresses TextChanged).
281+
/// </summary>
282+
[Fact]
283+
public void TextValidateField_ValueChangingCancel_SuppressesTextChanged ()
284+
{
285+
// Copilot
286+
TextValidateField field = new () { Width = 20, Height = 1 };
287+
288+
// Set a provider that accepts any text
289+
field.Provider = new TextRegexProvider (".*");
290+
field.Text = "initial";
291+
292+
// Cancel ValueChanging
293+
field.ValueChanging += (_, args) => args.Handled = true;
294+
295+
bool textChangedRaised = false;
296+
((View)field).TextChanged += (_, _) => textChangedRaised = true;
297+
298+
field.Text = "blocked";
299+
300+
Assert.False (textChangedRaised, "TextChanged should not fire when ValueChanging cancels");
301+
Assert.Equal ("initial", field.Text);
302+
}
303+
304+
/// <summary>
305+
/// Verifies that DatePicker rejects invalid (unparseable) text: Text should not
306+
/// persist an invalid string that cannot round-trip through DateTime.
307+
/// </summary>
308+
[Fact]
309+
public void DatePicker_InvalidText_DoesNotPersist ()
310+
{
311+
// Copilot
312+
DatePicker dp = new () { Width = 20, Height = 1 };
313+
DateTime originalValue = dp.Value;
314+
string originalText = dp.Text;
315+
316+
// Set invalid date text
317+
dp.Text = "not-a-date";
318+
319+
// Value should remain unchanged
320+
Assert.Equal (originalValue, dp.Value);
321+
322+
// Text should NOT hold the invalid string — it should revert or be rejected
323+
Assert.NotEqual ("not-a-date", dp.Text);
324+
}
325+
326+
/// <summary>
327+
/// Verifies that ColorPicker rejects invalid (unparseable) text: Text should not
328+
/// persist an invalid string that cannot round-trip through Color.
329+
/// </summary>
330+
[Fact]
331+
public void ColorPicker_InvalidText_DoesNotPersist ()
332+
{
333+
// Copilot
334+
ColorPicker cp = new () { Width = 20, Height = 3 };
335+
cp.SelectedColor = new Color (255, 0, 0);
336+
string originalText = cp.Text;
337+
338+
// Set invalid color text
339+
cp.Text = "not-a-color";
340+
341+
// SelectedColor should remain unchanged
342+
Assert.Equal (new Color (255, 0, 0), cp.SelectedColor);
343+
344+
// Text should NOT hold the invalid string — it should revert or be rejected
345+
Assert.NotEqual ("not-a-color", cp.Text);
346+
}
347+
348+
/// <summary>
349+
/// Verifies that setting View.Text on a word-wrapped TextView via a View reference
350+
/// does not corrupt or redundantly re-process the model.
351+
/// </summary>
352+
[Fact]
353+
public void TextView_WordWrap_PolymorphicSet_DoesNotCorruptModel ()
354+
{
355+
// Copilot
356+
TextView tv = new () { Width = 10, Height = 5, WordWrap = true };
357+
tv.Text = "Hello World this wraps";
358+
359+
// Set via polymorphic View reference
360+
View viewRef = tv;
361+
viewRef.Text = "Short";
362+
363+
Assert.Equal ("Short", tv.Text);
364+
Assert.Equal ("Short", viewRef.Text);
365+
}
230366
}

0 commit comments

Comments
 (0)