Centralize Terminal.Gui static Application access behind helpers

[BrettKinny]

Route all UI-layer access to the deprecated static Application object
through a small helper surface in Utilities/, alongside the existing
UiThread.Run wrapper:

• TerminalUi.AddTimeout/RemoveTimeout for main-loop timers
• TerminalUi.RunModal/RequestStop for modal dialogs
• TerminalUi.Query/ErrorQuery so call sites stop passing
  Application.Instance to MessageBox
• TerminalUi.TrySetClipboardData for the static Clipboard class
• TerminalUi.TopRunnableView/IsTopRunnable for top-level view queries

All call sites in App/ now use the helpers; behavior is unchanged.
The remaining direct uses of the deprecated statics are confined to
TerminalUi/UiThread plus the entry points that need the application
lifecycle itself (Program.cs Init/Run/Shutdown, MainWindow
Application.KeyDown, ThemeManager Application.Driver), which the next
commit migrates to the instance-based IApplication API.

CS0618 count: 101 -> 27, now concentrated in 5 files.

Co-Authored-By: Claude Fable 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01Cb6n3buYyULFMq4AursDkF

[claude]

Review

Reviewed the full diff plus the resulting repo state (this PR's commits are already merged into the checked-out tree, so I could grep/cross-check every call site rather than just read the patch).

Summary

Clean, well-scoped mechanical migration off the deprecated Terminal.Gui static Application surface, split into sensible commits (introduce helpers → migrate → enforce via TreatWarningsAsErrors → update docs). I verified there are zero remaining direct call sites of Application.Invoke/AddTimeout/RemoveTimeout/RequestStop/Run/KeyDown/TopRunnable/Instance, MessageBox.Query/ErrorQuery, or the static Clipboard outside of Utilities/TerminalUi.cs itself and the documented entry points (Program.cs, ThemeManager driver access) — matches the PR description exactly.

Things that look right

• Nullability of timer tokens is consistent: all timer fields (_pollTimer, _startupStatusTimer, _connectingAnimationTimer, _recordingStatusTimer, _updateTimer, _timerToken) are already declared object?, so TerminalUi.AddTimeout returning a nullable token doesn't introduce new nullable-warning fallout under the new TreatWarningsAsErrors.
• HelpDialog's #pragma warning disable/restore CS0618 is scoped tightly around just the new TextView { ... } expression — the later contentView.Text = ... line doesn't re-reference the TextView symbol by name, so it wouldn't re-trigger the warning. Scoping is correct.
• Fire-and-forget vs. throw split in TerminalUi (Invoke/timers/clipboard no-op when App is null; RunModal/RequestStop/Query/ErrorQuery throw) is a reasonable, clearly documented design — headless unit tests never exercise the throwing paths (confirmed via grep: no test references TerminalUi, RunModal, RequestStop, or MessageBox directly), so this doesn't break the existing suite.
• Program.cs's app?.Dispose() in finally being reachable even if app.Init() itself throws (since app is already assigned by Application.Create()) is actually safer than the old bool initialized gate, which skipped Shutdown() entirely if Init() faulted.

Suggestions (non-blocking)

1. No direct unit tests for Utilities/TerminalUi.cs. It's a small static class, but it now encodes real branching behavior (no-op when headless vs. throw for interactive calls) that is only implicitly exercised (by never hitting the throwing paths) rather than verified. A few cheap tests would lock this in and guard the headless-test story going forward, e.g.:
   • Invoke/AddTimeout/TrySetClipboardData are no-ops (don't throw) when TerminalUi.App is null.
   • RunModal/RequestStop/Query/ErrorQuery throw InvalidOperationException when TerminalUi.App is null.
     This is in the same spirit as the existing Opcilloscope.Tests/Utilities tests for the other small helpers (Logger, CsvRecordingManager, etc.) — UiThread itself also has no dedicated test, so this would be a good opportunity to close both gaps at once.
2. TreatWarningsAsErrors=true is a good guardrail per the stated goal, and it's well-commented, but worth calling out explicitly in review: any future NuGet bump (Terminal.Gui, OPC Foundation SDK) that introduces any new warning — not just deprecations — will now hard-fail the build rather than just warn. That's presumably intentional here, just flagging it as a policy change with real CI blast radius beyond this PR's stated motivation.

Bugs / correctness / security / performance

None found. This is a pure delegation/indirection refactor (call-site renames behind a thin static wrapper); no behavior, threading, or data-flow changes, and no security-sensitive surface is touched. Performance impact is a single extra static-method hop, negligible for a TUI app.

Nice, focused PR — the "why" in the commit messages (CS0618 counts going 101 → 27 → 1, confining the remainder to 5 files) makes the intent and progress easy to follow.

[claude]

Review: Centralize Terminal.Gui static Application access behind helpers

Nice, low-risk refactor — this is a clean, mechanical migration off the deprecated static Application surface, and it clearly continues the pattern already established by UiThread. Went through the full diff (23 files, +561/-119) plus the new TerminalUi.cs/UiThreadTests.cs in detail.

What it does

Introduces Utilities/TerminalUi.cs as the single access point for main-loop timers, modal dialogs, message boxes, clipboard, and top-level view queries, all backed by the instance-based IApplication (Application.Create()). Every App/ call site is swapped 1:1 from the static Application/MessageBox/Clipboard statics to the new helpers, Program.cs now owns the IApplication instance lifecycle explicitly, and Opcilloscope.csproj gets TreatWarningsAsErrors=true so the remaining CS0618 deprecation warnings can't silently creep back in. CLAUDE.md is updated in the same commit to document the new convention for future contributors — good practice, matches this repo's norm of keeping CLAUDE.md current.

Strengths

• Mechanical 1:1 replacement. Every call site swap (Application.RequestStop() → TerminalUi.RequestStop(), Application.Invoke → UiThread.Run, MessageBox.Query(Application.Instance, ...) → TerminalUi.Query(...), etc.) preserves the exact same call semantics, which keeps regression risk very low for a 23-file change.
• Thoughtful no-op vs. throw split. Fire-and-forget members (Invoke, timers, clipboard, TopRunnableView) degrade to no-ops when App is null (headless tests), while interactive members (RunModal, RequestStop, Query, ErrorQuery) throw InvalidOperationException via RequireApp(). That's the right call — a silent no-op on a modal dialog request would be a much worse failure mode than a loud exception.
• Solid new test coverage. TerminalUiTests and UiThreadTests cover both the no-app and with-app (via Moq) branches for every member, and correctly flag TerminalUi.App as process-global mutable state by sharing the non-parallel "Tui" xUnit collection and resetting it in Dispose() — this is exactly the kind of test hygiene that global static state needs.
• TreatWarningsAsErrors + targeted #pragma convention. The one remaining unavoidable CS0618 (obsolete TextView, since EditorView isn't shipped yet) is suppressed narrowly around just the TextView construction with a comment explaining why and a note to revisit later — exactly matches the guidance the PR adds to the .csproj comment.

Questions / things worth double-checking

1. Program.cs — Dispose() now runs even if Init() throws. Previously, Application.Shutdown() was gated behind a local initialized flag that was only set true after Application.Init() returned successfully, so a failed Init() meant Shutdown() was never called. Now:

   app = Application.Create();
   TerminalUi.App = app;
   app.Init();       // if this throws...
   ...
   finally { app?.Dispose(); }   // ...this still runs, since app is non-null

   If Init() throws partway through (e.g. terminal driver setup fails), app is non-null and Dispose() will still be invoked on an instance that was Create()d but never fully Init()d. Worth confirming IApplication.Dispose() is documented/guaranteed safe to call in that state — if not, it could throw a secondary exception that masks the original fatal error message the catch block is trying to print, or otherwise misbehave. This is a behavior change from the old gated pattern, even though it reads as more natural C# disposal.

2. Removed #pragma warning disable IL2026 around Application.Init(). The old code suppressed the reflection/AOT-trim warning specifically for the static Application.Init() call. Now that's app.Init() on the instance API, and the pragma is gone. Given TreatWarningsAsErrors=true is added in this very PR, if the instance IApplication.Init() still triggers IL2026 under trim/AOT analysis for this project's settings, that would be a hard build failure rather than a warning. Presumably CI already validates this (and green CI would settle it), but flagging in case it wasn't rebuilt with a clean cache after both changes landed together.

3. Minor/nit — TerminalUi.Query/ErrorQuery return int? with a doc comment "...or null if the message box was dismissed without a choice". Worth double-checking that MessageBox.Query/ErrorQuery in Terminal.Gui 2.4.5 can actually return a null-equivalent for a dismissed dialog (vs. always returning a concrete index/-1) — if it can't, the nullable-return doc comment is aspirational rather than reachable. Not a functional bug either way since callers already handle it safely (result != 0 / result == 0 both treat null as "not confirmed"), just worth confirming the doc matches actual behavior.

4. TerminalUi.App is public, globally mutable ({ get; set; }). This mirrors the risk profile of the static Application.Instance it replaces, so it's not a new problem introduced by this PR, but since the whole point of the refactor is to concentrate/control access, consider whether only Program.cs should be able to set it (e.g. internal set plus a [InternalsVisibleTo] for the test assembly) to make "only the entry point owns the lifecycle" a compiler-enforced invariant rather than a convention. Low priority — the CLAUDE.md doc already states this convention clearly, and test infra deliberately needs to set/reset it.

Test coverage

Good — new unit tests directly cover the new helper surface (TerminalUi, UiThread) for both the null-app and running-app paths. No existing tests appear to break given the changes are behavior-preserving 1:1 swaps. Nothing exercises Program.cs's lifecycle change directly (item #1 above), but that's consistent with the rest of the codebase — Program.Main isn't unit tested elsewhere either.

Security / performance

No security-relevant changes (purely internal UI plumbing), and no performance implications — this is a compile-time API surface change, not a behavioral or hot-path change.

Overall: solid, well-scoped, well-tested refactor. The two items worth a second look before merge are #1 (Dispose-on-partial-init) and #2 (IL2026 removal + TreatWarningsAsErrors interaction) — both are edge cases around startup failure handling rather than the happy path this PR is centered on.