Terminal.Gui 2.0→2.4.5 migration + in-process TUI component tests (layer 1)

[BrettKinny]

First of two PRs adding programmatic TUI testing.

What this is

Layer 1 of two-layer TUI testing: in-process component tests that drive the real
Terminal.Gui views/dialogs. Because the component-testing APIs only exist from Terminal.Gui
2.1.0+, this PR also carries the (large) Terminal.Gui 2.0.0 → 2.4.5 upgrade that they
require.

⚠️ This is a framework upgrade, review it as such

Terminal.Gui 2.0.0 → 2.1.0 was a breaking API redesign (not a minor bump); there is no version
with the test APIs that avoids it. Upgrading surfaced ~400 compile errors across 23 files.
Mapped and applied:

• Namespaces split into Terminal.Gui.{App,ViewBase,Views,Drawing,Input,Drivers,…} →
  added GlobalUsings.cs; aliased Attribute → Drawing.Attribute.
• ColorScheme → Scheme; view.ColorScheme = x → view.SetScheme(x); added a
  WithScheme() fluent helper for initializer-style assignment.
• Toplevel → Window, RadioGroup → OptionSelector (Labels/Value/ValueChanged).
• TableView: SelectedRow → Value?.SelectedCell.Y; SelectedCellChanged →
  ValueChanged; MouseClick → MouseEvent(Mouse).
• TreeView ObjectActivated → Activated; ListView OpenSelectedItem → Accepting.
• MenuItem shortcutKey: → positional Key; custom drawing Driver!.X → view-level
  SetAttribute/AddRune/AddStr/Move in OnDrawingContent.
• Application.Top → TopRunnable/TopRunnableView; SizeChanging → SubViewLayout;
  Colors.ColorSchemes[..] → SchemeManager.AddScheme; MessageBox.* now take
  Application.Instance; ShadowStyle.None → ShadowStyles.None; misc.

Rebased onto current main

The branch was rebased onto main (post #171/#173/#174/#175/#176/#177). Code added on main
after the branch point was migrated to 2.4.5 in a follow-up commit:

• TerminalTheme (Add Terminal theme that inherits the terminal's ANSI colour scheme #176): ColorScheme → Scheme; dropped its local Attribute alias
  (now in GlobalUsings.cs).
• ThemeManager: the static Application.Force16Colors no longer exists — the flag now
  lives on Application.Driver only.
• ThemeStyler keeps main's consolidated surface from Streamline codebase: remove dead code and consolidate duplicated logic #177 (CreateAccentButtonScheme/
  CreateFlatBarScheme), migrated to return Scheme.

Known regression — now verified visually (screenshots below)

Terminal.Gui 2.4 made adornments (Border/Margin/Padding) non-View objects without their own
Scheme. The former per-border colouring (distinct grey borders; bright-accent title on the
main window) now inherits the owning view's scheme. Focus-highlight is reapplied via the
FrameView's own scheme (UpdatePanelBorder) — visible in the screenshots as the accent
double-border on the focused pane. Border rendering is intact in all three themes.

Screenshots (live against the in-repo OPC UA test server)

Dark theme, 5 monitored variables	Light theme

Terminal theme (16-colour ANSI, #176 migrated)	Connect dialog (migrated OptionSelector)

Scope with four live signals (braille canvas, custom OnDrawingContent migration):

Note: the "Not Connected" indicator visible in some captures while data flows is a
pre-existing bug on main (reproduced on d3ad3dd with Terminal.Gui 2.0) — the status
label loses the Connected state after config-load connects. Not introduced by this PR;
to be tracked separately.

Layer 1 tests

Tests/Opcilloscope.Tests/Tui/ — real views, asserting component behaviour:

• MonitoredVariablesView: add/remove, scope-selection bookkeeping, idempotency.
• ConnectDialog: endpoint prefixing, interval, auth selector (the migrated OptionSelector),
  credential null-contract.
• Theme/Scheme: guards the ColorScheme → Scheme migration (all three themes) and the
  WithScheme helper.

Non-parallel xUnit collection (Application is global state). Auto-run by dotnet test/CI.

Why state, not rendered cells: Terminal.Gui 2.4.5 stable ships no public headless driver
(Application.Create() leaves Driver null; DriverAssert is develop-only). Rendered-screen
assertions are delivered by layer 2 (separate PR) — a black-box PTY harness against the
published binary. See docs/TESTING.md.

Verification

• Full solution builds clean against Terminal.Gui 2.4.5 (after rebase onto current main).
• 696/696 tests pass (678 pre-existing + 18 new).
• --help exits 0; full TUI launched against the in-repo test server: connect dialog,
  address-space browsing, subscriptions, live value updates, scope rendering, and all three
  themes exercised end-to-end (see screenshots).

🤖 Generated with Claude Code

[claude]

Code Review: Terminal.Gui 2.0 → 2.4.5 migration + TUI component tests

Overall this is a well-scoped, necessary upgrade. The mapping table in the PR description is a genuinely useful artifact. A few specific issues worth addressing before merge, plus a formatting nit that runs through the whole diff.

---

Bugs / correctness

ScopeDialog.cs — null-suppression on GetScheme()

}.WithScheme(GetScheme()!);

GetScheme() can return null if no scheme has been applied to the dialog yet, and calling WithScheme(null!) will produce a NullReferenceException when the child frame calls SetScheme. The old code used the ColorScheme property, which was always non-null. Safe replacement:

}.WithScheme(GetScheme() ?? ThemeManager.Current.MainColorScheme);

---

OpenConfigDialog.cs / SaveRecordingDialog.cs — null-forgiving on SelectedItem

SelectedFilePath = _files[_fileListView.SelectedItem!.Value].FullName;
var selected = _fileListItems[_fileListView.SelectedItem!.Value];

In TG 2.4 ListView.SelectedItem appears to be int? (hence the .Value call), but the ! suppresses the null check. This is fine as long as the bounds check above always runs first, but if the API changes the semantics, the suppression hides the failure. Use an explicit null check or the same guard pattern already used in MonitoredVariablesView:

var selectedRow = _tableView.Value?.SelectedCell.Y ?? -1;

---

LogView.cs — auto-scroll broken

-   _listView.TopItem = Math.Max(0, _displayedEntries.Count - _listView.Frame.Height);

ListView.SelectedItem = last does not guarantee the list scrolls to show the new entry in TG 2.4. With TopItem gone, new log lines that overflow the visible area will be silently off-screen. The new API appears to be _listView.EnsureSelectedItemVisible() (or equivalent). Needs a follow-up before the log view regresses in real use.

---

AddressSpaceView.cs — TOCTOU in HandleObjectActivated

var activated = _treeView.SelectedObject;   // reads state after the event fires

The selection could have changed between when Activated fired and when SelectedObject is read (e.g. rapid keyboard input). The old code read e.ActivatedObject directly from the event. If TG 2.4's Activated fires on the UI thread with the selection still held this is fine in practice, but it's worth confirming or adding a comment to that effect.

---

Known regression worth flagging explicitly

The PR description mentions border-colour loss as a "Known regression to confirm". The theme system had distinct BorderColorScheme / HighlightTitleBorderColorScheme for the main window and sub-panels — the focused-border accent in particular was a meaningful UX signal. The UpdatePanelBorder change now calls frameView.SetScheme(...) on the frame itself rather than the border adornment:

frameView.SetScheme(isFocused ? theme.FocusedBorderColorScheme : theme.BorderColorScheme);

This applies the accent scheme to the entire frame when focused, not just the border line. That means the frame's text content also picks up the accent colours, which is likely more visible than intended. Worth an eyeball against both themes.

---

Style / formatting

The WithScheme helper is a clean solution for the initializer restriction, but there are several collapsed single-line Scheme literals that are very hard to read:

// OpenConfigDialog.cs, QuickHelpDialog.cs, WriteValueDialog.cs, MonitoredVariablesView.cs:
}.WithScheme(new Scheme { Normal = new Attribute(theme.Accent, theme.Background), Focus = new Attribute(theme.AccentBright, theme.Background), HotNormal = new Attribute(theme.Accent, theme.Background), HotFocus = new Attribute(theme.AccentBright, theme.Background), Disabled = new Attribute(theme.MutedText, theme.Background) });

The same PR also introduces nicely formatted multi-line equivalents elsewhere (e.g. MainWindow.cs). These should be expanded — the multi-line form is the style used everywhere else in the codebase.

---

Tests

The new layer-1 tests are well-structured:

• TuiCollection correctly serialises all TUI tests to avoid Application global-state races
• MonitoredVariablesViewTests tests the right thing (state, not rendered cells) and the IsIdempotentPerClientHandle case is particularly valuable
• ConnectDialogTests directly guards the RadioGroup → OptionSelector migration

A couple of small additions would strengthen coverage:

• ConnectDialogTests.Username_AuthExposesUsername could also assert Password is null when no password arg is passed, since the credential-hiding logic depends on this
• A test for WithScheme itself in ThemeSchemeTests — it's load-bearing migration glue and currently untested directly
• LogView auto-scroll is missing any test (though that may be intentional given the headless-driver limitation)

---

Minor nits

• ScopeView.cs — WantMousePositionReports = false was removed; if TG 2.4 defaults to true the scope will receive every mouse-move event and redraw on each. Worth a comment on whether this was intentional.
• GlobalUsings.cs comment says "Terminal.Gui 2.1+" but the package jumped from 2.0.0 directly to 2.4.5; could just say "2.4.x" to match the package version.
• docs/TESTING.md references Tests/Opcilloscope.E2ETests/README.md which does not exist yet (layer 2 PR); fine to leave but a note that the file is forthcoming would avoid confusion for anyone reading this today.

[BrettKinny]

Regression review done (multi-agent, adversarially verified)

Ran a regression review over the migration diff (7 subsystem reviewers → independent per-finding verification against decompiled Terminal.Gui 2.0 and 2.4 sources → synthesis). It confirmed 4 behavioral regressions, now fixed in 7d2799b:

Sev	File	Regression	Fix
High-impact	SaveRecordingDialog.OnFileListOpenSelected	Unhandled Accepting bubbles Accept to the default Save button → browsing a folder / picking a file saved+closed the dialog mid-browse	set e.Handled = true
Medium	MonitoredVariablesView.HandleMouseClick	e.Handled = true now pre-empts TableView's selection command (TG 2.4 runs MouseEvent before the command pipeline), so a Sel-column click toggled scope but no longer highlighted the row / raised SelectedVariableChanged	drop e.Handled
Medium	LogView (auto-scroll)	TG 2.4 removed ListView.TopItem; the log stopped following new entries	EnsureSelectedItemVisible()
Low	SaveRecordingDialog.NavigateToSelected	ListView.SelectedItem is now int?; OR-form guard didn't catch null → null.Value throws at a CSV-less FS root	explicit null guard

Root pattern: TG 2.4 moved selection/activation out of overridden OnMouseEvent/OpenSelectedItem handlers into the command pipeline, which now runs after the raw MouseEvent/Accepting events — so any carried-over handler's Handled (set or unset) can have the opposite effect on the base command than it did in 2.0.

Still-open known item (by design, not fixed): per-adornment border colouring is lost in TG 2.4 (adornments have no own Scheme); focus-highlight reapplied via the FrameView scheme — worth an eyeball in UAT.

627/627 tests pass.

[claude]

Code Review — Terminal.Gui 2.0→2.4.5 Migration + TUI Component Tests (Layer 1)

This is a large, well-executed migration. The PR description is clear about the scope and the known regressions. The in-line comments explaining why each migration was made are genuinely helpful for future maintainers. A few issues worth addressing before merge.

---

Issues

1. Potential null-dereference in ScopeDialog (ScopeDialog.cs ~line 47)

}.WithScheme(GetScheme()!);

View.GetScheme() returns null if no scheme has been set yet. The null-forgiving ! suppresses the warning but doesn't prevent a NullReferenceException at runtime if the FrameView (button container) is constructed before SetScheme() has been called on the parent dialog. Prefer an explicit fallback:

}.WithScheme(GetScheme() ?? Theme.MainColorScheme);

---

2. Dead code: HighlightTitleBorderColorScheme in AppTheme / DarkTheme / LightTheme

The per-border colouring was correctly removed because Terminal.Gui 2.4 no longer supports per-adornment schemes. However, the HighlightTitleBorderColorScheme property and its backing field remain in all three theme classes and are no longer called anywhere. Either delete it or add a clearly visible TODO comment tied to the planned VisualRoles work so it is not silently swept away later.

---

3. SubViewLayout fires on every layout pass, not just terminal resize

// Before
Application.SizeChanging += (s, e) => UiThread.Run(UpdateConnectionStatusLabelPosition);
// After
SubViewLayout += (s, e) => UiThread.Run(UpdateConnectionStatusLabelPosition);

Application.SizeChanging fired only on terminal resize. SubViewLayout fires on every layout pass, including after any SetNeedsLayout() call. Since UpdateConnectionStatusLabelPosition calls SetNeedsLayout() itself, check whether this could create a re-trigger loop under certain conditions. If Terminal.Gui 2.4 exposes a more targeted resize event, that would be safer.

---

4. Readability: long single-line Scheme initialisers

Several Scheme constructions are now single-line expressions exceeding 200 characters, e.g. in OpenConfigDialog.cs, QuickHelpDialog.cs, WriteValueDialog.cs, and MonitoredVariablesView.cs (three instances). The original multi-line form was more readable. Extracting the scheme to a local variable before the initializer keeps WithScheme and restores legibility:

var accentScheme = new Scheme
{
    Normal    = new Attribute(theme.Accent, theme.Background),
    Focus     = new Attribute(theme.AccentBright, theme.Background),
    HotNormal = new Attribute(theme.Accent, theme.Background),
    HotFocus  = new Attribute(theme.AccentBright, theme.Background),
    Disabled  = new Attribute(theme.MutedText, theme.Background)
};
var openButton = new Button { ... }.WithScheme(accentScheme);

The identical accent-button scheme also appears across six dialogs — it would be worth extracting it into a helper on AppTheme (e.g. AccentButtonScheme) to eliminate the duplication while the code is open.

---

5. Missing teardown in TUI tests

MonitoredVariablesViewTests and ConnectDialogTests construct real Terminal.Gui views but do not Dispose() them. MonitoredVariablesView subscribes to ThemeManager.ThemeChanged at construction time, so undisposed views will leak event handlers across tests in the same collection. Add using or implement IDisposable / call Dispose() in a cleanup method per test.

---

6. docs/TESTING.md forward-references a non-existent project

The doc refers to Tests/Opcilloscope.E2ETests/ (layer 3) and says to see its README.md, but that project is not part of this PR. Fine for layered delivery, but the forward reference will confuse readers today. A quick note — "Layer 3 (not yet delivered — coming in the follow-up PTY PR)" — would make the intent explicit.

---

Positive Observations

• GlobalUsings.cs cleanly avoids per-file using churn after the namespace split — good call.
• SchemeExtensions.WithScheme<T> is a clean adapter for the initializer limitation; the doc comment accurately explains why it exists.
• The SaveRecordingDialog.OnFileListOpenSelected e.Handled = true fix (preventing a bubbled Accepting from triggering Save during folder navigation) is subtle and the comment is exactly what a future maintainer needs.
• Not marking e.Handled in HandleMouseClick is the correct choice for preserving row selection alongside scope-toggle; the comment explains it clearly.
• The _fileListView.SelectedItem >= 0 null-guard in NavigateToSelected is sound: C# lifted comparisons return false for null, so !.Value is safe when reached.
• Non-parallel xUnit collection for TUI tests is the right solution for Terminal.Gui's global Application state.
• 627/627 tests is a strong confidence signal for a migration of this scope.

---

Summary

The migration is solid and the reasoning throughout is well-documented. The blocking issues are (1) the GetScheme()! null-deref risk in ScopeDialog and (2) the dead HighlightTitleBorderColorScheme code that should be removed or annotated. The SubViewLayout frequency concern and the test teardown gap are lower-priority but worth addressing before the draft is lifted. The long-line Scheme initialiser style is an easy cleanup.

[claude]

Code Review — Terminal.Gui 2.0→2.4.5 migration + TUI component tests

Overall this is a well-executed, large-scale framework migration. The mapping document in the PR description, the inline comments at each changed site, and the 627/627 passing tests all indicate solid execution. Below are specific observations grouped by severity.

---

Medium — Readability regressions in inline Scheme initializations

The WithScheme() helper is a clever workaround for SetScheme() not being usable in an object initializer. However, in several places the multi-line ColorScheme { ... } blocks were collapsed onto a single very long line when migrated:

HelpDialog.cs (~line 195):

}.WithScheme(new Scheme { Normal = new Attribute(...), Focus = new Attribute(...), HotNormal = ..., HotFocus = ..., Disabled = ... });

WriteValueDialog.cs (~line 587), MonitoredVariablesView.cs (~lines 1496, 1530, 1547) — same pattern.

The original multi-line form was clearer. Recommend keeping the Scheme { } block multi-line, just moving it inside the WithScheme() call:

}.WithScheme(new Scheme
{
    Normal   = new Attribute(theme.Error, theme.Background),
    Focus    = new Attribute(theme.Error, theme.Background),
    HotNormal = new Attribute(theme.Error, theme.Background),
    HotFocus  = new Attribute(theme.Error, theme.Background),
    Disabled  = new Attribute(theme.Error, theme.Background)
});

---

Minor — ScopeDialog.cs: null-forgiving operator on GetScheme()

}.WithScheme(GetScheme()!);

GetScheme() returning null here would silently NRE inside WithScheme. A short comment explaining when this can safely be non-null (e.g., "scheme is always set by the Dialog base before construction completes") would prevent future confusion.

---

Minor — TOCTOU in AddressSpaceView.HandleObjectActivated

The old event carried the activated object directly:

// Before
private void HandleObjectActivated(object? _, ObjectActivatedEventArgs<BrowsedNode> e)
    => NodeSubscribeRequested?.Invoke(e.ActivatedObject);

The new version reads _treeView.SelectedObject after the fact:

// After
var activated = _treeView.SelectedObject;
if (activated != null && activated.NodeClass == Opc.Ua.NodeClass.Variable)
    NodeSubscribeRequested?.Invoke(activated);

In practice this is fine for a UI app (no concurrent selection changes on the UI thread), but it's worth a comment acknowledging it — the PR description explains this choice but it's not captured at the call site.

---

Minor — TerminalTheme absent from ThemeSchemeTests

ThemeSchemeTests.Themes() covers DarkTheme and LightTheme but not TerminalTheme, which has its own MainColorScheme and MenuColorScheme overrides and notably uses ColorName16 rather than RGB values. Given that the 16-color path has different constraints, it would be the most likely to break silently.

---

Nit — NodeDetailsView.cs uses SetScheme outside the initializer while similar views use WithScheme

// NodeDetailsView.cs
_copyButton = new Button { ..., Enabled = false };
_copyButton.SetScheme(theme.ButtonColorScheme);  // ← two-statement form

Other views chain .WithScheme(). Either is correct, but the inconsistency is slightly jarring. Could be new Button { ..., Enabled = false }.WithScheme(theme.ButtonColorScheme) for uniformity.

---

What's done well

• Inline migration comments at each non-obvious change site (SubViewLayout, TopItem → EnsureSelectedItemVisible, MouseEvent event order, e.Handled reasoning in HandleMouseClick) are exactly the kind of context that prevents future regressions.
• GlobalUsings.cs cleanly handles the namespace explosion and the Attribute ambiguity — the comment explaining the alias is appreciated.
• SaveRecordingDialog.NavigateToSelected null-guarding and OpenConfigDialog.Confirm's _fileListView.SelectedItem!.Value both correctly address the int? change in ListView.
• TuiCollection non-parallel xUnit collection is the right approach for global Application state.
• The test for AddVariable idempotency is a nice catch for a subtle edge case.
• The PR description's "Known regression to confirm" note about adornment-level coloring is honest and appropriately scoped.

---

Summary

The migration is solid. The four issues above are all low-risk; the readability item in the inline Scheme blocks is the most impactful to fix before merge. Everything else is either a minor polish or a documentation gap.

[claude]

Code Review — PR #165: Terminal.Gui 2.0 → 2.4.5 migration + TUI component tests

This is a thorough, well-executed migration. The PR description maps every breaking change to its fix, the 14 new tests guard the migration, and 692/692 tests pass. Overall I'd call this ready to merge with a few minor items worth addressing.

---

What the PR does

• Upgrades Terminal.Gui from 2.0.0 to 2.4.5 (a semver-minor bump that was actually a breaking redesign)
• Migrates ~400 compile errors across 23 files from the changed API surface
• Adds GlobalUsings.cs to absorb the namespace restructuring centrally
• Introduces SchemeExtensions.WithScheme<T>() to recover the object-initializer style lost when ColorScheme = became SetScheme()
• Adds 14 in-process TUI component tests under Tests/Opcilloscope.Tests/Tui/

---

Positives

• GlobalUsings.cs + Attribute alias — the right approach; absorbs all namespace churn in one place and keeps the per-file using count the same.
• WithScheme<T>() helper — clean generic solution; it's tested (WithScheme_AppliesTheSchemeAndReturnsTheSameView) and the TuiCollection properly serialises the global-state tests.
• Null-safety improvements — ListView.SelectedItem is now int? in TG 2.4; the sel is null || sel < 0 guards in OpenConfigDialog and SaveRecordingDialog are correct and the comments explain why sel < 0 alone would miss null.
• e.Handled = true in SaveRecordingDialog.OnFileListOpenSelected — this is a real correctness fix, well-explained in the comment.
• HandleObjectActivated race justification — the note about the Activated event being raised synchronously on the UI thread before selection can move is the right reasoning.
• Application.Force16Colors removal handled in tests — the ThemeManager_SetTerminalTheme_TogglesTerminalColorMode test correctly falls back to asserting the theme property since Driver is null in headless tests.

---

Issues

Bug risk — HighlightTitleBorderColorScheme is dead code but still allocated

App/Themes/AppTheme.cs, line ~193

The property is cached with a ??= backing field and the class is abstract, so every concrete theme instantiates this scheme eagerly on first access. It's never applied post-migration (as the TODO comment says). It's minor allocations, but the real concern is that any test or future caller that inspects HighlightTitleBorderColorScheme will get a stale non-null object and incorrectly infer that the feature still works.

Suggestion: either mark it [Obsolete] now, or open a follow-up issue and add a comment cross-referencing it here.

---

Inconsistent scheme application — NodeDetailsView._copyButton

App/Views/NodeDetailsView.cs, around line 36–50

// All other views use the new object-initializer form:
}.WithScheme(theme.ButtonColorScheme);

// But _copyButton uses a two-step form:
_copyButton = new Button { ..., Enabled = false };
_copyButton.SetScheme(theme.ButtonColorScheme);   // ← separate statement

This is the only widget in the file that uses the two-statement form. It's not wrong, but it breaks the pattern everyone else follows. If Enabled can co-exist in the initializer, the trailing .SetScheme() call could be changed to .WithScheme() as well.

---

Missing test — Password getter null behaviour with UserName auth

App/Dialogs/ConnectDialog.cs, line ~35

The Password getter changed from:

? _passwordField.Text : null;

to:

&& !string.IsNullOrEmpty(_passwordField.Text) ? _passwordField.Text : null;

This is a meaningful semantic change (an untouched password box now returns null instead of ""). The existing Username_AuthExposesUsername test asserts Assert.Null(dialog.Password) with no password set, which covers the constructor-default path. But there is no test for the case where UserName auth is selected and a password is provided — that path returns the non-null string and is the most likely regression site if someone refactors this property.

---

Missing test — SaveRecordingDialog.Accepting bubbling fix

App/Dialogs/SaveRecordingDialog.cs, line ~487

The e.Handled = true fix prevents folder navigation from triggering the default Save button. This is a subtle, regression-prone interaction (the bubbling behaviour depends on TG internals). There are currently no TUI tests for SaveRecordingDialog at all. A test that constructs the dialog, navigates the list, and asserts !dialog.Confirmed would pin this behaviour.

---

Spurious blank line in ScopeView.OnDrawingContent

App/Views/ScopeView.cs, around line 302

 protected override bool OnDrawingContent(DrawContext? context)
 {
-        if (Driver is null) return false;
 
     AppTheme theme;

The removal left a blank line between the method signature and the first statement. Cosmetic only, but easy to clean up.

---

e.Position is not { } pos null pattern — verify intent

App/Views/MonitoredVariablesView.cs, line ~668

if (!e.IsSingleClicked || e.Position is not { } pos)
    return;

This null-pattern-matches Mouse.Position. If Position is a non-nullable struct in TG 2.4's API, the is not { } branch can never be true and the guard only serves as documentation. Worth a quick check against the TG 2.4 API to confirm whether this should be e.Position directly or the nullable form — either way, add a comment if the type is nullable so readers don't strip it.

---

Minor suggestions

• SchemeExtensions.cs lacks explicit usings — it uses Scheme and View but relies entirely on GlobalUsings.cs. That's fine by project convention, but worth a one-line comment explaining the dependency (especially since this is a new file that future readers may encounter in isolation).
• TableView.Value?.SelectedCell.Y ?? -1 appears identically in three places (SelectedVariable getter, OnSelectedCellChanged, implicit in the test). A private GetSelectedRow() helper would make the null-pattern explicit and reduce repetition.
• ThemeManagerTests — driver not asserted: the updated test correctly documents why it can't assert on driver.Force16Colors in a headless context, but it's worth adding a // TODO: assert driver.Force16Colors when a public test driver ships comment to keep the gap visible.

---

Known regression (acknowledged)

The per-adornment ColorScheme loss (border vs content separation) is correctly documented and UpdatePanelBorder still applies focus colouring to the FrameView itself. The visual regression should be eyeballed before merge, as the PR notes.

---

Summary

The migration is correct and complete. The primary ask before merge is:

1. Add a test for Password null behaviour with UserName auth.
2. Track or [Obsolete] the dead HighlightTitleBorderColorScheme.
3. Fix the trailing blank line in ScopeView.OnDrawingContent.

Items 2 and 3 could reasonably be follow-ups. Item 1 is a real coverage gap on a changed code path.

[claude]

Code Review — PR #165: Terminal.Gui 2.0→2.4.5 Migration + TUI Component Tests (Layer 1)

This is a well-executed, large-scope framework upgrade. The migration mapping is thorough and the inline documentation of why each change was made is excellent. Notes below are roughly priority-ordered.

---

Potential Bugs / Issues

1. SubViewLayout fires on every layout, not just terminal resize

In MainWindow.cs:

SubViewLayout += (s, e) => UiThread.Run(UpdateConnectionStatusLabelPosition);

The original Application.SizeChanging only fired on terminal resize. SubViewLayout fires on every layout pass (theme changes, dialog open/close, etc.), so UpdateConnectionStatusLabelPosition will now run much more frequently. If it's purely a label re-position (cheap math + property set) this is fine — but worth verifying it's not doing anything heavier. If it becomes a source of visual jitter, a resize-specific event (or debouncing) may be worth tracking.

2. TopRunnable vs TopRunnableView — two different properties used in two places

• FocusManager.cs: Application.TopRunnableView?.MostFocused
• MainWindow.cs: Application.TopRunnable != this

These seem intentionally distinct (one returns the View, the other returns the IRunnable). Confirm both are the right call for each site — the original Application.Top was used for both, so there's subtle semantic divergence here.

3. GetScheme() called during ScopeDialog constructor

}.WithScheme(GetScheme() ?? Theme.MainColorScheme);

GetScheme() is called on this (the ScopeDialog being constructed) before any scheme has been applied. In 2.4.x GetScheme() returns null when no scheme is set, so the ?? Theme.MainColorScheme fallback kicks in — which appears to be the intent. Just worth a comment noting this is intentional, since it reads like it might be a bug.

---

Code Quality

SchemeExtensions.WithScheme<T> — good migration glue, but consider trimming the XML doc

The new helper in SchemeExtensions.cs is the right call. The class-level doc comment explaining the constraint is valuable. The method-level <summary> on WithScheme repeats most of it — one location for the explanation is enough.

HighlightTitleBorderColorScheme is [Obsolete] but still cached and computed

The property is marked obsolete and no call site in this PR uses it, but the lazy-init backing field still computes and caches a Scheme on first access. The [Obsolete] attribute doesn't stop internal code from accidentally calling it. Consider an #pragma warning disable CS0618 guard at any remaining internal call sites, or simply leave a TODO to delete it once the VisualRoles path is explored.

HandleObjectActivated — race condition comment is correct but could be tighter

// Safe from activate/select races: Activated is raised synchronously on the UI
// thread by the command that acted on the selection, so it still matches.
var activated = _treeView.SelectedObject;

The reasoning is sound. Small suggestion: move this comment to the method rather than inside the body so it's visible when reading the event subscription.

OnFileListOpenSelected — e.Handled = true comment is excellent

The explanation for why the event must not be marked handled (to preserve row-selection after Sel-column toggle) is exactly the kind of non-obvious invariant that belongs in the code. No changes needed; calling it out as a model for the rest of the migration comments.

---

Test Coverage

New Tui/ tests — mostly good, one gap

• MonitoredVariablesViewTests covers add/remove/idempotency well.
• ConnectDialogTests covers endpoint prefixing, interval, and auth state.
• ThemeSchemeTests covers the WithScheme glue and ThemeStyler.ApplyToFrame — smart regression guard.

Missing: MonitoredVariablesViewTests has no test for SelectedVariable after AddVariable. The view initializes with SelectedVariable == null (tested), but there's no test asserting the selected variable updates after a row is added. This may be intentional given the note about headless Driver, but worth a comment explaining why it's omitted.

ConnectDialogTests.Username_AuthExposesTypedPassword directly sets passwordField.Text = "hunter2". This bypasses the TextChanged event that normally drives OnTextChanged. Verify the Password property reads the field text directly (not from event state) — looks like it does from the diff, so this should be fine.

ThemeManager_SetTerminalTheme_TogglesTerminalColorMode — test now asserts ThemeManager.Current.UseTerminalColors rather than Application.Force16Colors. The comment explains this (Driver is null in headless). The assertion is weaker but correct given the constraint. A comment on what would be needed to test the driver flag (e.g., "a real driver, covered by E2E tests") would connect this to the three-layer testing doc.

---

Minor / Nits

• _statusBar.Subviews → _statusBar.SubViews (casing): correct, non-obvious rename — worth a one-line comment in the migration notes section of the PR body (it's currently not listed explicitly there).
• TableView.Style.ShowHorizontalBottomline → ShowHorizontalBottomLine: same — looks like a corrected casing in the API, good catch.
• Password property null-vs-empty change: the comment is correct that downstream treats them identically (Password ?? string.Empty), but the change is a semantic improvement. No action needed.
• docs/TESTING.md: well-written. The note that Layer 3 (PTY/E2E) doesn't exist yet is appropriately honest.

---

Summary

This is a solid, well-documented breaking-framework migration. The three main things I'd ask about before merging:

1. Confirm SubViewLayout firing frequency is acceptable for UpdateConnectionStatusLabelPosition.
2. Confirm TopRunnable vs TopRunnableView are the right properties for their respective call sites.
3. Add a note on the ScopeDialog GetScheme() initializer call to make the intent explicit.

The new test layer is a good foundation. The architecture decision (state assertions now, rendered-cell assertions in the PTY PR) is clearly reasoned and documented.

🤖 Generated with Claude Code

[claude]

Code Review — Terminal.Gui 2.0→2.4.5 Migration + TUI Component Tests (Layer 1)

This is a well-executed, large-scale framework migration. The PR is systematic, well-documented, and lands cleanly. Notes below are ordered by significance.

---

Overview

A breaking Terminal.Gui upgrade (~400 compile errors across 23 files) bundled with the first layer of in-process TUI component tests. The PR body is unusually thorough about what changed and why — the mapping table is genuinely useful for reviewing. Screenshots confirm visual parity across all three themes.

---

What's done well

• GlobalUsings.cs cleanly absorbs the namespace churn without polluting per-file using lists. The Attribute alias at the bottom is exactly right — without it System.Attribute vs Terminal.Gui.Drawing.Attribute would be a constant source of confusion.

• WithScheme<T>() extension is a neat solution to the "can't call a method inside an object initializer" problem. Returning T (not View) is the right call: callers keep the concrete type without a cast.

• e.Handled = true in SaveRecordingDialog.OnFileListOpenSelected — the comment explaining why the event must be consumed is exactly the kind of non-obvious constraint that belongs in code. Good.

• Explicit null guard in NavigateToSelected — correctly identifies that null < 0 evaluates to false under C# lifted comparisons, making the OR-guard insufficient. The fix and the explanation are both correct.

• Test collection design — [CollectionDefinition("Tui", DisableParallelization = true)] with a dedicated TuiCollection.cs is the right way to handle Terminal.Gui's global Application state. The explanation in MonitoredVariablesViewTests (and docs/TESTING.md) about why rendered-cell assertions aren't here yet is honest and useful.

• [Obsolete] on HighlightTitleBorderColorScheme rather than a silent delete — leaves a breadcrumb for when VisualRoles lands, without hiding the regression.

---

Issues and suggestions

1. SubViewLayout fires much more often than SizeChanging (low impact, worth noting)

// MainWindow.cs
SubViewLayout += (s, e) => UiThread.Run(UpdateConnectionStatusLabelPosition);

SubViewLayout fires on every layout pass, not only on terminal resize. UpdateConnectionStatusLabelPosition appears idempotent so there's no correctness issue, but it adds UI-thread work on every layout cycle. If Terminal.Gui 2.4 exposes a resize-specific event (e.g. on the driver or via Application.SizeChanged), prefer that. If not, this is acceptable as-is — just flag it for the follow-up.

2. HandleObjectActivated relies on a synchronous selection invariant

// AddressSpaceView.cs
private void HandleObjectActivated(object? _, EventArgs<ICommandContext?> e)
{
    var activated = _treeView.SelectedObject;  // reads current selection
    ...
}

The comment says: "Safe from activate/select races: Activated is raised synchronously on the UI thread by the command that acted on the selection." That reasoning is correct today, but it depends on Terminal.Gui's internal command ordering, which isn't a public contract. If a future version makes activation asynchronous or raises the event before updating SelectedObject, this silently does nothing (or, worse, acts on the wrong node). Consider opening a tracking issue noting the dependency.

3. Null-forgiving operator in OpenConfigDialog.Confirm() — guard is sufficient but opaque

// OpenConfigDialog.cs
SelectedFilePath = _files[_fileListView.SelectedItem!.Value].FullName;

The ! is technically sound here: the preceding _fileListView.SelectedItem >= 0 lifts to false when SelectedItem is null, so the guard already protects the dereference. But a reader who doesn't know C# lifted-comparison semantics may think null >= 0 guards null (it does — but only by coincidence with the false branch). SaveRecordingDialog made the explicit null check (sel is null || ...); applying the same pattern here would be more readable:

var sel = _fileListView.SelectedItem;
if (sel is null || sel < 0 || sel >= _files.Count) return;
SelectedFilePath = _files[sel.Value].FullName;

Minor, but consistent with how it's done elsewhere in this PR.

4. ThemeManager_SetTerminalTheme_TogglesTerminalColorMode tests the wrong layer

// ThemeManagerTests.cs
Assert.True(ThemeManager.Current.UseTerminalColors);

Asserting on ThemeManager.Current.UseTerminalColors tests that the theme property has the expected value — it doesn't verify that ThemeManager actually propagated it to Application.Driver. Since Driver is null in headless tests, the propagation path can't be exercised here, which is fine. The test name and comment both acknowledge this. Consider adding an inline comment at the assertion site (not just above the try) so future readers don't question whether the assertion is a bug.

5. Redundant five-role muted-color Scheme repeated in multiple places

This is pre-existing, not introduced here, but the migration touched it in several spots:

}.WithScheme(new Scheme
{
    Normal = new Attribute(theme.MutedText, theme.Background),
    Focus  = new Attribute(theme.MutedText, theme.Background),
    HotNormal = new Attribute(theme.MutedText, theme.Background),
    HotFocus  = new Attribute(theme.MutedText, theme.Background),
    Disabled  = new Attribute(theme.MutedText, theme.Background)
});

AppTheme already has a CreateAccentButtonScheme / CreateFlatBarScheme pattern. A CreateMutedLabelScheme() on AppTheme (or ThemeStyler) would reduce this four-site duplication and make theme-wide color changes single-touch. Worth a follow-up but not a blocker for this PR.

---

Test coverage assessment

The 18 new tests cover the migration-critical contracts:

Test class	What it guards
ConnectDialogTests	Endpoint prefixing, interval, OptionSelector auth, password null-contract
MonitoredVariablesViewTests	Add/remove, scope bookkeeping, idempotency by client handle
ThemeSchemeTests	ColorScheme→Scheme migration (all 3 themes), WithScheme chaining

The scope is appropriate for the "state, not rendered cells" limitation imposed by the stable 2.4.5 release. docs/TESTING.md honestly describes the gap and where layer 2 will close it.

One gap: there are no tests for the SaveRecordingDialog Accepting → e.Handled = true fix (item 2 in the bullet list above). That's a subtle behaviour change; a regression would be silent. Even a unit test that calls NavigateToSelected via a subview find and verifies the dialog isn't closed would add meaningful coverage.

---

Security / performance

No concerns. This is pure UI and test infrastructure. The Driver! null-forgiving accesses have all been replaced with view-level methods — that's a correctness improvement.

---

Summary

The migration is thorough and the new tests are well-targeted. The items above are all low-to-medium severity; none are blockers. Approachable as-is with the SubViewLayout frequency and the HandleObjectActivated invariant called out as follow-up tracking items.

🤖 Generated with Claude Code