Skip to content

Commit a8d7f32

Browse files
committed
LT-22717: tighten comments to the repo commenting standard
Move the file-header explanation into the fixture's own doc comment, replace decorative section banners with one-line statements, drop a pointer to a working document, and state the AcceptsReturn contract as current behaviour rather than as a before-and-after.
1 parent 1c21a07 commit a8d7f32

3 files changed

Lines changed: 32 additions & 54 deletions

File tree

Src/LexText/Interlinear/ComplexConcPatternVc.cs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -427,7 +427,8 @@ public override ITsString DisplayVariant(IVwEnv vwenv, int tag, int frag)
427427
private void DisplayFeatures(IVwEnv vwenv, ComplexConcPatternNode node)
428428
{
429429
// Every line here (Type, Form, Entry, Category, Gloss, Infl Features) is a computed
430-
// summary of the synthetic pattern node, not free text; see UpdateProp and SetNotEditable.
430+
// summary of the synthetic pattern node, not free text; see UpdateProp and
431+
// SetNotEditable.
431432
SetNotEditable(vwenv);
432433
vwenv.AddProp(ktagType, this, kfragFeatureLine);
433434
var morphNode = node as ComplexConcMorphNode;

Src/LexText/Interlinear/ITextDllTests/ComplexConcPatternVcDirectEditTests.cs

Lines changed: 26 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -1,21 +1,7 @@
11
// Copyright (c) 2026 SIL International
22
// This software is licensed under the LGPL, version 2.1 or later
33
// (http://www.gnu.org/licenses/lgpl-2.1.html)
4-
//
5-
// Reproduction and regression coverage for the Complex Concordance pattern-builder crash.
6-
// ComplexConcControl and the phonological rule formula editor share PatternView/
7-
// PatternVcBase. ComplexConcPatternVc has no UpdateProp override, so an edit that reaches
8-
// the view engine without passing through PatternView.OnKeyPress (IME composition,
9-
// drag-and-drop, or any direct IVwSelection.ReplaceWithTsString call) falls through to
10-
// VwBaseVc.UpdateProp, which throws NotImplementedException. Unlike the sibling rule-formula
11-
// bug, ComplexConcPatternVc binds no real domain fields via AddStringAltMember (verified by
12-
// inspection: zero occurrences in ComplexConcPatternVc.cs), so this is a crash, not a silent
13-
// corruption/rename.
14-
//
15-
// These tests drive a real IVwRootBox (PatternView/ComplexConcPatternVc) against a real
16-
// in-memory LcmCache and call IVwSelection.ReplaceWithTsString directly -- the same low-level
17-
// entry point IME composition or drag-and-drop would use, and one PatternView.OnKeyPress never
18-
// sees because it only reacts to Windows key events, not to ReplaceWithTsString.
4+
195
using System.Collections.Generic;
206
using System.Reflection;
217
using System.Windows.Forms;
@@ -32,6 +18,16 @@
3218

3319
namespace SIL.FieldWorks.IText
3420
{
21+
/// <summary>
22+
/// Covers the Complex Concordance pattern builder's response to an edit that reaches the
23+
/// view engine without passing through PatternView.OnKeyPress, as IME composition and
24+
/// drag-and-drop do. Without an UpdateProp override such an edit falls through to
25+
/// VwBaseVc.UpdateProp, which throws. The pattern builder binds no real domain fields, so
26+
/// the failure is a crash rather than a silent rename of shared project data.
27+
///
28+
/// These tests drive a real IVwRootBox against an in-memory LcmCache and call
29+
/// IVwSelection.ReplaceWithTsString directly.
30+
/// </summary>
3531
[TestFixture]
3632
public class ComplexConcPatternVcDirectEditTests : MemoryOnlyBackendProviderTestBase
3733
{
@@ -251,11 +247,8 @@ public void MakeSelOnFragment_DiscriminatesBetweenFragments_OnTheSameNode()
251247
Assert.That(tss.Text, Is.EqualTo("Gloss: myGloss"));
252248
}
253249

254-
// ------------------------------------------------------------------
255-
// Angle 1: breadth of the crash across the fragments ComplexConcPatternVc renders.
256-
// Each of these encodes the DESIRED end state (no crash, content unchanged) and must
257-
// fail against current code, which throws NotImplementedException instead.
258-
// ------------------------------------------------------------------
250+
// Breadth of the crash across every fragment ComplexConcPatternVc renders. Each case
251+
// asserts no crash and unchanged content.
259252

260253
[Test]
261254
public void ReplaceWithTsString_OnWordNodeTypeLine_DoesNotThrow()
@@ -342,10 +335,8 @@ public void ReplaceWithTsString_OnMorphNodeCategoryLine_DoesNotThrow_AndRealPart
342335

343336
Assert.DoesNotThrow(() => AttemptEdit(sel, "HACKED", Cache.DefaultAnalWs),
344337
"a direct edit on the morph node's Category line must not crash the view engine");
345-
// This is the specific check for the bug doc's claim that this is a crash, not a
346-
// Bug-1-style corruption: the category line displays a REAL, shared IPartOfSpeech's
347-
// Abbreviation, so if this bug were the same class as Bug 1, a botched edit here
348-
// could rename it project-wide. Confirm it does not.
338+
// The category line displays a real, shared IPartOfSpeech's Abbreviation, so a
339+
// botched edit here would rename it project-wide.
349340
Assert.That(noun.Abbreviation.BestAnalysisAlternative.Text, Is.EqualTo("N"),
350341
"an edit attempt on the Category line must not rename the real, shared PartOfSpeech");
351342
}
@@ -458,15 +449,8 @@ public void ReplaceWithTsString_OnNodeMinimum_DoesNotThrow_AndMinimumUnchanged()
458449
"the synthetic pattern node's Minimum must not be mutated by a discarded edit");
459450
}
460451

461-
// ------------------------------------------------------------------
462-
// Angle 2: is the crash reachable through PatternView's own input handling (keystrokes),
463-
// or only through paths that bypass it (IME composition, drag-and-drop, or any other
464-
// direct ReplaceWithTsString caller)? PatternView.OnKeyPress unconditionally sets
465-
// e.Handled = true and returns without calling base.OnKeyPress for anything but
466-
// Backspace/Delete, so ordinary WM_CHAR-driven typing never reaches the engine at all.
467-
// This test is expected to PASS today: it documents that the keystroke path is already
468-
// safe, which is what makes the ReplaceWithTsString bypass above the actual bug.
469-
// ------------------------------------------------------------------
452+
// Ordinary typing never reaches the view engine: OnKeyPress handles everything except
453+
// Backspace and Delete, so only paths that bypass it can crash.
470454

471455
[Test]
472456
public void SimulateTyping_ViaOnKeyPress_DoesNotReachEngine_AndDoesNotCrash()
@@ -486,10 +470,7 @@ public void SimulateTyping_ViaOnKeyPress_DoesNotReachEngine_AndDoesNotCrash()
486470
"a plain keystroke must not reach the engine and alter content -- PatternView.OnKeyPress swallows it before that");
487471
}
488472

489-
// ------------------------------------------------------------------
490-
// Angle 3: insert/delete must keep working. PatternView.OnKeyDown raises
491-
// RemoveItemsRequested for the Delete key; this must survive whatever fix is applied.
492-
// ------------------------------------------------------------------
473+
// Insert and delete must keep working: OnKeyDown raises RemoveItemsRequested for Delete.
493474

494475
// ------------------------------------------------------------------
495476
// Ablation evidence for the fix's layers.
@@ -559,20 +540,14 @@ public void ComplexConcControl_WiresViewAsReadOnly()
559540
}
560541

561542
/// <summary>
562-
/// SimpleRootSite.ReadOnlyView's setter forces AcceptsReturn = AcceptsTab = false when set
563-
/// to true (SimpleRootSite.cs), which happens AFTER the Designer's own explicit
564-
/// AcceptsReturn = true / AcceptsTab = false assignments in InitializeComponent's
565-
/// generated-code ordering. This pins the actual, real behaviour change: AcceptsTab was
566-
/// already false before this fix (Designer-set, independent of ReadOnlyView) so Tab
567-
/// navigation out of the pane is unchanged; AcceptsReturn flips from true to false, which
568-
/// is new. Since PatternView.OnKeyPress already swallows Return either way (it is not
569-
/// Backspace/Delete), the observable difference is only where the key is disposed of: it
570-
/// used to reach the control and be silently swallowed there; now IsInputKey(Return)
571-
/// returns false and the key is never delivered to the control at all, so it is processed
572-
/// as an ordinary dialog/navigation key by whatever contains this pane. This control is
573-
/// hosted as a Words-area tool pane (DistFiles/.../Concordance/toolConfiguration.xml), not
574-
/// inside a modal dialog with an AcceptButton, so no default-button activation is expected
575-
/// in practice -- but that is unverified live; see the review doc.
543+
/// SimpleRootSite.ReadOnlyView's setter forces AcceptsReturn and AcceptsTab to false,
544+
/// after the Designer's own assignments in InitializeComponent. AcceptsTab is false
545+
/// either way, so Tab navigation out of the pane is unchanged. AcceptsReturn becomes
546+
/// false, and since PatternView.OnKeyPress swallows Return regardless, the only
547+
/// difference is where the key is disposed of: IsInputKey(Return) returns false, so the
548+
/// key is not delivered to the control and whatever hosts the pane treats it as an
549+
/// ordinary navigation key. The pane is a Words-area tool rather than a modal dialog
550+
/// with an AcceptButton, so no default-button activation is expected. Unverified live.
576551
/// </summary>
577552
[Test]
578553
public void ComplexConcControl_AcceptsTabUnchanged_AcceptsReturnNowFalse()

Src/LexText/LexTextControls/PatternView.cs

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -70,8 +70,10 @@ protected override EditingHelper CreateEditingHelper()
7070
}
7171

7272
/// <summary>
73-
/// Activate() is suppressed by default in ReadOnlyViews (SimpleRootSite.AllowDisplaySelection
74-
/// defaults to IsEditable), but both PatternView consumers are pattern builders whose chooser
73+
/// Activate() is suppressed by default in ReadOnlyViews
74+
/// (SimpleRootSite.AllowDisplaySelection
75+
/// defaults to IsEditable), but both PatternView consumers are pattern builders whose
76+
/// chooser
7577
/// insert/delete needs the user to see the current selection even when the view itself is
7678
/// read-only.
7779
/// </summary>

0 commit comments

Comments
 (0)