Skip to content

Commit 6be9832

Browse files
johnml1135johnml1135
committed
details-controls-testability plan
1 parent 334eb72 commit 6be9832

File tree

8 files changed

+425
-0
lines changed

8 files changed

+425
-0
lines changed

‎openspec/changes/detail-controls-testability/.openspec.yaml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
schema: spec-driven
2+
created: 2026-02-12

‎openspec/changes/detail-controls-testability/design.md‎

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
1+
## Context
2+
3+
The `Src/Common/Controls/DetailControls/` subsystem is the property-editing backbone of FLEx. It comprises three parallel class hierarchies — Slices (rows in the property editor), Launchers (chooser-button controls), and Views (value renderers) — all hosted inside a `DataTree` container. The architecture predates modern testability patterns: classes inherit from WinForms `UserControl`, there are no interfaces, and logic is deeply entangled with UI lifecycle.
4+
5+
Current state:
6+
- **~65 production files, ~31 tests** (7 test files)
7+
- **11 key classes with zero test coverage** (ButtonLauncher, ReferenceLauncher, all Slice subclasses, all Views, both Choosers)
8+
- **Deep inheritance** — `MorphTypeAtomicLauncher` is 5 levels deep from `UserControl`
9+
- **Bidirectional coupling** — Launchers → Slice → DataTree → Slices → Launchers
10+
- **Ad-hoc refresh protocol** — `DoNotRefresh`, `RefreshListNeeded`, `m_postponePropChanged` are independent booleans with complex interactions
11+
- **No interfaces** — everything depends on concrete classes
12+
13+
The class diagram is maintained separately in [detail-controls-class-diagram.mmd](detail-controls-class-diagram.mmd).
14+
15+
## Goals / Non-Goals
16+
17+
**Goals:**
18+
- Make launcher business logic testable without WinForms
19+
- Document the architecture so agents and developers understand the composition pattern
20+
- Incrementally introduce testability seams without breaking existing code
21+
- Cover the refresh protocol (source of LT-22414 and similar bugs) with tests
22+
23+
**Non-Goals:**
24+
- Rewriting or replacing WinForms with another framework
25+
- Achieving high test coverage for all 65 files in one pass
26+
- Changing user-visible behavior
27+
- Introducing new runtime dependencies
28+
29+
## Decisions
30+
31+
### 1. Incremental seams over big-bang rewrite
32+
33+
**Decision:** Use Legacy Seam (Feathers) and Branch By Abstraction (Fowler) patterns. Introduce `IDataTreeServices` interface implemented by `DataTree`. Existing code continues to use `DataTree` directly; new test code uses the interface.
34+
35+
**Rationale:** The codebase works. Rewriting carries regression risk with no user-visible benefit. Seams can be introduced one at a time, validated by builds, with zero behavior change.
36+
37+
**Alternatives considered:**
38+
- Full MVP/MVVM rewrite — too risky, too much churn, no user-visible benefit
39+
- Test only through UI automation — slow, flaky, doesn't catch logic bugs
40+
- Leave untested — unacceptable given the LT-22414 class of bugs
41+
42+
### 2. Humble Object extraction for launcher logic
43+
44+
**Decision:** Extract a `MorphTypeSwapLogic` POCO class from `MorphTypeAtomicLauncher` containing `SwapValues`, `IsStemType`, `CheckForAffixDataLoss`, `ChangeAffixToStem`, `ChangeStemToAffix`. The launcher delegates to this class.
45+
46+
**Rationale:** These methods contain business-critical data-mutation logic that should never have lived in a `UserControl` subclass. Extraction follows the Humble Object pattern (Feathers/Meszaros) — the logic class needs only `LcmCache`, not Forms/Graphics/Mediator.
47+
48+
**Alternatives considered:**
49+
- Make methods `internal` and test via `InternalsVisibleTo` — already done for `SwapValues` but doesn't reduce coupling
50+
- Test through DataTree integration tests only — works (we did this for LT-22414) but is slow and can't cover edge cases without full UI setup
51+
52+
### 3. Strangler Fig for new slice types
53+
54+
**Decision:** Any new slice type or field editor written from this point forward SHALL use the testable architecture (interfaces, extracted logic). Existing slices are migrated only when they need bug fixes.
55+
56+
**Rationale:** Fowler's Strangler Fig pattern — new growth uses the better architecture, old code is migrated opportunistically. This avoids a big migration project while ensuring quality improves over time.
57+
58+
### 4. Diagram as standalone Mermaid file
59+
60+
**Decision:** Maintain the class diagram as a standalone `.mmd` file alongside the design doc, referenced from `AGENTS.md`. Not embedded inline.
61+
62+
**Rationale:** Standalone `.mmd` files render in GitHub, can be validated by Mermaid CLI, and are referenceable from multiple documents without duplication.
63+
64+
## Risks / Trade-offs
65+
66+
| Risk | Mitigation |
67+
|------|------------|
68+
| `IDataTreeServices` interface becomes a "god interface" | Keep it narrow — only the services that tests actually need. Split into finer interfaces as needed. |
69+
| Humble Object extraction introduces delegation overhead | Negligible — these methods run once per user gesture, not in tight loops. |
70+
| Architecture docs go stale | Reference from `AGENTS.md` which has automated change-log tracking. The Mermaid diagram is machine-parseable and can be validated. |
71+
| Incremental approach leaves some classes permanently untested | Track coverage gaps in the architecture doc. Prioritize testing when classes are modified for bug fixes. |
72+
73+
## Open Questions
74+
75+
1. Should `IDataTreeServices` live in `DetailControls/` or in a shared interface assembly?
76+
2. Should the `RefreshCoordinator` state machine be a separate class or a formalization within `DataTree`?
77+
3. How much of the `ReferenceLauncher.HandleChooser` flow is worth extracting given the dialog dependencies?

‎openspec/changes/detail-controls-testability/detail-controls-class-diagram.mmd‎

Lines changed: 150 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,150 @@
1+
%% DetailControls Class Hierarchy — Slices, Launchers, Views
2+
%% Maintained as part of the detail-controls-testability openspec change.
3+
%% Render with: mmdc -i detail-controls-class-diagram.mmd -o detail-controls-class-diagram.svg
4+
5+
classDiagram
6+
direction TB
7+
8+
class UserControl {
9+
<<WinForms>>
10+
}
11+
12+
class Slice {
13+
+Install(DataTree)
14+
+FinishInit()
15+
+Expand()
16+
+Collapse()
17+
+HandleDeleteCommand()
18+
+ContainingDataTree
19+
}
20+
21+
class FieldSlice {
22+
<<abstract>>
23+
+UpdateDisplayFromDatabase()
24+
}
25+
26+
class ViewSlice
27+
28+
class ReferenceSlice {
29+
<<abstract>>
30+
+FinishInit()
31+
+Install(DataTree)
32+
+DisplayNameProperty
33+
}
34+
35+
class AtomicReferenceSlice {
36+
+FinishInit()
37+
+ShowSubControls()
38+
+RefreshTree()
39+
}
40+
41+
class ReferenceVectorSlice
42+
43+
class PossibilityAtomicReferenceSlice
44+
class MorphTypeAtomicReferenceSlice
45+
class DerivMSAReferenceSlice
46+
class InflMSAReferenceSlice
47+
class PossibilityReferenceVectorSlice
48+
class PhoneEnvReferenceSlice
49+
class SemanticDomainReferenceVectorSlice
50+
51+
class ButtonLauncher {
52+
+Initialize(cache, obj, flid)
53+
#HandleChooser()
54+
#OnClick()
55+
+MainControl
56+
+Slice
57+
}
58+
59+
class ReferenceLauncher {
60+
#HandleChooser()
61+
#GetChooser() SimpleListChooser
62+
+AddItem(ICmObject)
63+
+SetItems(IEnumerable)
64+
+UpdateDisplayFromDatabase()
65+
}
66+
67+
class AtomicReferenceLauncher {
68+
+Target ICmObject
69+
+AddItem()
70+
#GetChooser()
71+
#CreateAtomicReferenceView()
72+
}
73+
74+
class VectorReferenceLauncher {
75+
+AddItem()
76+
+SetItems()
77+
#CreateVectorReferenceView()
78+
}
79+
80+
class PossibilityAtomicReferenceLauncher
81+
class PossibilityVectorReferenceLauncher
82+
83+
class MorphTypeAtomicLauncher {
84+
+SwapValues()
85+
+IsStemType()
86+
+HandleChooser()
87+
}
88+
89+
class PhoneEnvReferenceLauncher
90+
class SemanticDomainReferenceLauncher
91+
class GenDateLauncher
92+
93+
class ReferenceViewBase {
94+
<<RootSiteControl>>
95+
}
96+
97+
class AtomicReferenceView
98+
class VectorReferenceView
99+
class PossibilityAtomicReferenceView
100+
class PossibilityVectorReferenceView
101+
class PhoneEnvReferenceView
102+
103+
class DataTree {
104+
+ShowObject()
105+
+DoNotRefresh
106+
+RefreshListNeeded
107+
+RefreshList()
108+
+PropChanged()
109+
+Slices
110+
}
111+
112+
%% Slice hierarchy
113+
UserControl <|-- Slice
114+
Slice <|-- FieldSlice
115+
Slice <|-- ViewSlice
116+
FieldSlice <|-- ReferenceSlice
117+
ReferenceSlice <|-- AtomicReferenceSlice
118+
ReferenceSlice <|-- ReferenceVectorSlice
119+
AtomicReferenceSlice <|-- PossibilityAtomicReferenceSlice
120+
AtomicReferenceSlice <|-- DerivMSAReferenceSlice
121+
AtomicReferenceSlice <|-- InflMSAReferenceSlice
122+
PossibilityAtomicReferenceSlice <|-- MorphTypeAtomicReferenceSlice
123+
ReferenceVectorSlice <|-- PossibilityReferenceVectorSlice
124+
ReferenceVectorSlice <|-- PhoneEnvReferenceSlice
125+
PossibilityReferenceVectorSlice <|-- SemanticDomainReferenceVectorSlice
126+
127+
%% Launcher hierarchy
128+
UserControl <|-- ButtonLauncher
129+
ButtonLauncher <|-- ReferenceLauncher
130+
ButtonLauncher <|-- GenDateLauncher
131+
ReferenceLauncher <|-- AtomicReferenceLauncher
132+
ReferenceLauncher <|-- VectorReferenceLauncher
133+
AtomicReferenceLauncher <|-- PossibilityAtomicReferenceLauncher
134+
PossibilityAtomicReferenceLauncher <|-- MorphTypeAtomicLauncher
135+
VectorReferenceLauncher <|-- PossibilityVectorReferenceLauncher
136+
PossibilityVectorReferenceLauncher <|-- SemanticDomainReferenceLauncher
137+
ReferenceLauncher <|-- PhoneEnvReferenceLauncher
138+
139+
%% View hierarchy
140+
ReferenceViewBase <|-- AtomicReferenceView
141+
ReferenceViewBase <|-- VectorReferenceView
142+
AtomicReferenceView <|-- PossibilityAtomicReferenceView
143+
VectorReferenceView <|-- PossibilityVectorReferenceView
144+
145+
%% Composition
146+
DataTree *-- Slice : hosts
147+
AtomicReferenceSlice *-- AtomicReferenceLauncher : creates
148+
ReferenceVectorSlice *-- VectorReferenceLauncher : creates
149+
AtomicReferenceLauncher *-- AtomicReferenceView : MainControl
150+
VectorReferenceLauncher *-- VectorReferenceView : MainControl

‎openspec/changes/detail-controls-testability/proposal.md‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
## Why
2+
3+
The DetailControls subsystem (`Src/Common/Controls/DetailControls/`) is the property-editing backbone of FLEx — every field the user edits lives inside a DataTree of Slices, Launchers, and Views. Yet it has only ~31 tests across ~65 production files, with 11 key classes at zero coverage. The recent LT-22414 bug (Morph Type field disappearing after type change) was caused by a missing `RefreshListNeeded = true` in a single method — a defect that trivial tests would have caught. The subsystem also lacks a `DetailControls/AGENTS.md` (the parent AGENTS.md has a dangling link to it), so agents working in this area have no architectural guidance.
4+
5+
## What Changes
6+
7+
- **Create `DetailControls/AGENTS.md`** documenting the three-hierarchy composition pattern (Slices, Launchers, Views), lifecycles, refresh protocol, and testing strategy.
8+
- **Add unit tests for pure-logic methods** — `IsStemType()`, `CheckForAffixDataLoss`, `CheckForStemDataLoss` — that require zero WinForms infrastructure.
9+
- **Add integration tests for the refresh protocol** — expand `DataTree.PropChanged`/`RefreshList` coverage beyond the single LT-22414 scenario.
10+
- **Introduce seam interfaces** (`IDataTreeServices`) to break tight coupling between Slices/Launchers and concrete DataTree/Cache/Mediator, enabling test doubles.
11+
- **Extract launcher logic** into POCO classes (e.g., `MorphTypeSwapLogic`) following the Humble Object pattern, making business-critical morph-type-swap logic independently testable.
12+
- **Document the architecture** with a Mermaid class diagram showing all inheritance hierarchies and composition relationships.
13+
14+
## Non-goals
15+
16+
- Rewriting or replacing the WinForms UI framework.
17+
- Changing any user-visible behavior or UI layout.
18+
- Adding Avalonia or cross-platform support.
19+
- Modifying the XML layout/parts configuration schema.
20+
- Native C++ changes — this is purely managed (C#) code.
21+
22+
## Capabilities
23+
24+
### New Capabilities
25+
26+
- `detail-controls-architecture`: Architectural documentation of the DetailControls subsystem — class hierarchies, composition patterns, lifecycles, refresh protocol, and the Mermaid class diagram.
27+
- `detail-controls-testability`: Seam interfaces, Humble Object extractions, and test infrastructure improvements enabling unit testing of launcher and slice logic without WinForms dependencies.
28+
29+
### Modified Capabilities
30+
31+
- `architecture/ui-framework/winforms-patterns`: Add cross-reference to the new DetailControls architecture spec, since DetailControls is the primary consumer of the WinForms patterns documented there.
32+
33+
## Impact
34+
35+
- **Affected code:** `Src/Common/Controls/DetailControls/` — launchers, slices, views, DataTree
36+
- **Test project:** `Src/Common/Controls/DetailControls/DetailControlsTests/`
37+
- **Documentation:** New `DetailControls/AGENTS.md`, new architecture spec, updated `winforms-patterns.md`
38+
- **Dependencies:** No new NuGet packages or external dependencies
39+
- **Risk:** Low — seam interfaces are additive; Humble Object extractions preserve existing behavior behind delegation

‎openspec/changes/detail-controls-testability/specs/architecture/ui-framework/winforms-patterns/spec.md‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
## ADDED Requirements
2+
3+
### Requirement: DetailControls architecture is cross-referenced
4+
5+
The `winforms-patterns` spec SHALL include a cross-reference to the DetailControls architecture documentation, since DetailControls is the primary consumer of the WinForms composition patterns.
6+
7+
#### Scenario: Cross-reference exists
8+
9+
- **WHEN** a developer reads `architecture/ui-framework/winforms-patterns`
10+
- **THEN** it SHALL reference `detail-controls-architecture` for the specific class hierarchy and composition pattern used in the property editor

‎openspec/changes/detail-controls-testability/specs/detail-controls-architecture/spec.md‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
## ADDED Requirements
2+
3+
### Requirement: Architecture documentation exists for DetailControls
4+
5+
The DetailControls subsystem SHALL have an `AGENTS.md` file documenting the three-hierarchy composition pattern (Slices, Launchers, Views), key lifecycles, and the refresh notification protocol. The parent `Src/Common/Controls/AGENTS.md` currently links to `DetailControls/AGENTS.md` which does not exist.
6+
7+
#### Scenario: AGENTS.md is present and linked
8+
9+
- **WHEN** an agent or developer navigates to `Src/Common/Controls/DetailControls/`
10+
- **THEN** an `AGENTS.md` file SHALL exist documenting the Slice, Launcher, and View hierarchies, the DataTree composition pattern, and the `DoNotRefresh`/`RefreshListNeeded`/`PropChanged` refresh protocol
11+
12+
#### Scenario: Parent link resolves
13+
14+
- **WHEN** the parent `Src/Common/Controls/AGENTS.md` references `DetailControls/AGENTS.md`
15+
- **THEN** the link SHALL resolve to a valid file
16+
17+
### Requirement: Class hierarchy diagram is maintained
18+
19+
A Mermaid class diagram SHALL be maintained as a standalone file showing the complete inheritance and composition relationships for all Slice, Launcher, and View classes in DetailControls.
20+
21+
#### Scenario: Diagram renders all three hierarchies
22+
23+
- **WHEN** the diagram file is rendered
24+
- **THEN** it SHALL show the Slice hierarchy (Slice → FieldSlice → ReferenceSlice → AtomicReferenceSlice → ...), the Launcher hierarchy (ButtonLauncher → ReferenceLauncher → AtomicReferenceLauncher → ...), and the View hierarchy (ReferenceViewBase → AtomicReferenceView → ...) with composition relationships to DataTree
25+
26+
#### Scenario: Diagram is referenced from AGENTS.md
27+
28+
- **WHEN** a developer reads `DetailControls/AGENTS.md`
29+
- **THEN** it SHALL reference the class diagram file
30+
31+
### Requirement: Refresh protocol documentation
32+
33+
The `AGENTS.md` SHALL document the `DoNotRefresh` / `RefreshListNeeded` / `PropChanged` / `m_postponePropChanged` refresh protocol including the state transitions and the known bug patterns (as demonstrated by LT-22414).
34+
35+
#### Scenario: Refresh protocol pitfalls are documented
36+
37+
- **WHEN** a developer modifies code that sets `DoNotRefresh = false`
38+
- **THEN** the documentation SHALL warn that `RefreshListNeeded` MUST be set to `true` before releasing `DoNotRefresh` if data was changed during the guarded window

‎openspec/changes/detail-controls-testability/specs/detail-controls-testability/spec.md‎

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
## ADDED Requirements
2+
3+
### Requirement: Pure-logic launcher methods have unit tests
4+
5+
All pure-logic methods (no WinForms dependencies) on launcher classes SHALL have unit tests. At minimum this covers `MorphTypeAtomicLauncher.IsStemType()`, `CheckForAffixDataLoss()`, and `CheckForStemDataLoss()`.
6+
7+
#### Scenario: IsStemType correctly classifies all morph type GUIDs
8+
9+
- **WHEN** `IsStemType()` is called with each of the `MoMorphTypeTags.kguidMorphType*` GUIDs
10+
- **THEN** it SHALL return `true` for stem, root, bound-root, bound-stem, clitic, enclitic, proclitic, particle, and phrase types, and `false` for all affix types (prefix, suffix, infix, etc.)
11+
12+
#### Scenario: CheckForAffixDataLoss detects affix data
13+
14+
- **WHEN** `CheckForAffixDataLoss` is called on a morph with affix-type data (e.g., inflection features, affix slots)
15+
- **THEN** it SHALL return `true` indicating data would be lost
16+
17+
#### Scenario: CheckForStemDataLoss detects stem data
18+
19+
- **WHEN** `CheckForStemDataLoss` is called on a morph with stem-type data
20+
- **THEN** it SHALL return `true` indicating data would be lost
21+
22+
### Requirement: DataTree refresh protocol has integration tests
23+
24+
The `DoNotRefresh` → `RefreshListNeeded` → `RefreshList` protocol SHALL have integration tests covering the key interaction patterns.
25+
26+
#### Scenario: Refresh occurs after DoNotRefresh release with RefreshListNeeded
27+
28+
- **WHEN** `DoNotRefresh` is set to `true`, data is changed, `RefreshListNeeded` is set to `true`, and then `DoNotRefresh` is set to `false`
29+
- **THEN** slices SHALL reflect the updated data (e.g., `ifdata` slices disappear when their data is cleared)
30+
31+
#### Scenario: No refresh without RefreshListNeeded
32+
33+
- **WHEN** `DoNotRefresh` is set to `true`, data is changed, and `DoNotRefresh` is set to `false` WITHOUT setting `RefreshListNeeded`
34+
- **THEN** slices SHALL remain stale (this documents the bug pattern)
35+
36+
#### Scenario: Multiple PropChanged during DoNotRefresh
37+
38+
- **WHEN** multiple `PropChanged` notifications arrive while `DoNotRefresh` is `true`
39+
- **THEN** a single `RefreshList` call SHALL process all accumulated changes when `DoNotRefresh` is released
40+
41+
### Requirement: Seam interfaces enable test doubles
42+
43+
An `IDataTreeServices` interface SHALL be introduced to abstract the dependency bundle (LcmCache, Mediator, PropertyTable) that Slices and Launchers currently access through concrete `DataTree` references.
44+
45+
#### Scenario: Interface is consumable by existing code
46+
47+
- **WHEN** the `IDataTreeServices` interface is introduced
48+
- **THEN** existing Slice and Launcher code SHALL compile without changes (the interface is additive, implemented by `DataTree`)
49+
50+
#### Scenario: Tests can use mock services
51+
52+
- **WHEN** a test needs to verify launcher logic in isolation
53+
- **THEN** it SHALL be able to provide a test implementation of `IDataTreeServices` without constructing a full DataTree or WinForms control hierarchy
54+
55+
### Requirement: Launcher logic is extractable via Humble Object pattern
56+
57+
Business-critical logic in launchers (morph type swap, data loss checking) SHALL be extractable into POCO classes that can be tested without WinForms dependencies. The launcher class becomes a thin shell delegating to the logic class.
58+
59+
#### Scenario: MorphTypeSwapLogic is independently testable
60+
61+
- **WHEN** `MorphTypeSwapLogic` (or equivalent) is created
62+
- **THEN** it SHALL contain `SwapValues`, `IsStemType`, `CheckForAffixDataLoss`, `ChangeAffixToStem`, and `ChangeStemToAffix` logic
63+
- **AND** it SHALL be testable with only `LcmCache` (via `MemoryOnlyBackendProvider`) and no WinForms controls
64+
65+
#### Scenario: Existing launcher behavior is preserved
66+
67+
- **WHEN** the Humble Object extraction is applied to `MorphTypeAtomicLauncher`
68+
- **THEN** all existing `DetailControlsTests` SHALL continue to pass
69+
- **AND** the user-visible behavior SHALL be identical

0 commit comments

Comments
 (0)