Fixes #5636 - Scope MainLoopSyncContext to running sessions (await-before-RunAsync deadlock) - #5641
Conversation
…k await-before-RunAsync. Since #5588, Init installed the app's MainLoopSyncContext as the calling thread's ambient SynchronizationContext. Any await between Init and Run/RunAsync then captured a context whose Post queues onto the main loop - which is not running yet - so the continuation (the code about to start the loop) was stranded and the app hung at startup. ConfigureAwait(false) avoided the capture, matching the workaround shipped in tui-cs/clet. - Init creates the context but no longer installs it as ambient; Begin still installs it for the duration of a session, so awaits inside event handlers keep marshaling to the main loop (the #5579 contract). - Run saves the caller's ambient context and restores it on exit, so an await after Run cannot capture a context that stopped pumping. - ResetState clears the ambient context only when it is this app's own; a foreign (caller-owned) context is left untouched. - MainLoopSyncContext.Post/Send fall back to the thread pool / inline execution once the app is no longer Initialized - callbacks posted after Shutdown/Dispose execute instead of being stranded (previously Post threw NotInitializedException via Invoke). Test-first: RunAsync_AfterAwaitFollowingInit_DoesNotDeadlock reproduced the hang (10s timeout) before the fix and passes in ~2s after. New SyncContextLifecycleTests cover the Init/Run/Dispose ambient-context contract and the post-dispose Post fallback; existing tests that encoded the #5588 ambient-at-Init behavior were updated to the new contract. Suites: 17,564 parallelizable, 32 non-parallelizable, 437 integration - all pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NGq38GbuZPAEa5wZHkmPiZ
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9286cf8c1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR fixes #5636 by scoping MainLoopSyncContext to active application sessions and adding lifecycle regression tests.
Changes:
- Stops installing the context during
Initand restores ambient contexts afterRun. - Adds fallback dispatch behavior after shutdown or disposal.
- Adds and updates synchronization-context and deadlock tests.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Review result |
|---|---|
Tests/UnitTestsParallelizable/Application/SyncContextLifecycleTests.cs |
Adds lifecycle regression tests. No final review comments. |
Tests/UnitTestsParallelizable/Application/RunAsyncTests.cs |
Adds the startup deadlock regression test. No final review comments. |
Tests/UnitTestsParallelizable/Application/ApplicationImplTests.cs |
Updates context-related assertions. No final review comments. |
Tests/UnitTests.NonParallelizable/Application/SynchronizationContextTests.cs |
Updates synchronization-context contract tests. No final review comments. |
Terminal.Gui/App/MainLoop/MainLoopSyncContext.cs |
Critical (3 votes): Posts after Run/End can still queue to a stopped loop while Initialized remains true; gate dispatch on active session state while retaining pre-run queueing behavior. |
Terminal.Gui/App/ApplicationImpl.Run.cs |
Critical (2 votes): Begin installs the app context but End does not restore the replaced context, including for nested sessions, allowing subsequent awaits to deadlock. |
Terminal.Gui/App/ApplicationImpl.Lifecycle.cs |
Critical (1 vote): Direct Begin/End sessions can leave the stopped app context ambient; save and restore the caller context across these sessions, including nested sessions. |
Suppressed comments (2)
Terminal.Gui/App/ApplicationImpl.Lifecycle.cs:317
- This cleanup clears the ambient context to
nullwhen it sees the app's context, but the caller's context underneath it is not retained for directBeginusage. For example, with a foreign context installed,Beginreplaces it, andDisposewhile that session is active reaches this branch and loses the foreign context instead of restoring it. Preserve the prior context when Begin installs the app context and restore it during End/ResetState.
if (SynchronizationContext is { } ownContext && System.Threading.SynchronizationContext.Current == ownContext)
{
System.Threading.SynchronizationContext.SetSynchronizationContext (null);
}
Terminal.Gui/App/MainLoop/MainLoopSyncContext.cs:50
- The same liveness check leaves
Sendblocking forever after a session ends:Initializedis still true, so a non-main-thread caller enters_app.Invokeand waits for a callback that no loop can drain.Sendneeds to use the same inactive-session fallback asPost(while preserving synchronous dispatch during an active run).
// After Shutdown/Dispose no main loop can ever pump the queue; execute inline.
if (!_app.Initialized || _app.MainThreadId == Thread.CurrentThread.ManagedThreadId)
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…back after sessions end. Codex/Copilot review of #5641 flagged two gaps in the session scoping: - The documented Begin/End building-block sequence installed the app context but never restored the caller's. Begin now saves the caller's ambient context on the SessionToken and End restores it (guarded so a foreign thread's context is never clobbered); for nested sessions the outermost End restores the caller's context. - Post/Send gated only on Initialized, so a continuation captured during a session that resumed after End was queued onto a loop that may never pump again. They now gate on CanPumpPostedWork: pump while a session is running, or before the first Run completes (preserving the documented post-Init/pre-Run queueing); otherwise fall back to the thread pool / inline execution. Test-first: Begin_End_RestoresAmbientSyncContext_IncludingNestedSessions and Post_AfterSessionEnded_StillExecutesCallback both failed before the change and pass after. Suites: 17,566 parallelizable, 32 non-parallelizable, 437 integration - all pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NGq38GbuZPAEa5wZHkmPiZ
…ter MEC migration (#5416) * chore: start CM removal follow-up Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * chore: start CM removal follow-up Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Add spec for legacy ConfigurationManager removal Companion to specs/replace-cm-with-mec.md. Defines the scope, design, and phased execution of fully removing CM after the MEC migration in #5411. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Update removal spec to reflect Phase A1/B/C-extract landed in #5411 Removes from scope: ConfigurationManager.Applied subscriber rewiring (done via ThemeChanges in #5411), JSON converter SerializerContext sweep (done via TuiSerializerContext in #5411). Adds Phase A2 (Mec managers own runtime theme/scheme data) as the gating prerequisite for ScopeJsonConverter deletion. Sharpens D-02 decision (Option alpha: nested-only + TuiConfigMigrator; Option beta: custom legacy MEC source). Corrects PrintJsonErrors framing — behavior-preserving replacement is possible via JsonConfigurationSource.OnLoadException. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Add CM removal prep artifact: AOT baseline, examples/test inventories - AOT baseline (post-#5411 @ 83ded73): NativeAot.exe = 22.77 MB, Terminal.Gui.dll = 1.77 MB, ConfigPropertyHostTypes roots 31 types. - Examples inventory: ~115 files but ~105 are the same one-line CM.Enable() call; identifies the 10 non-trivial scenarios that need per-site review (ConfigurationEditor, Runner, UICatalogRunnable, etc.). - Test inventory: ~13 files to delete, ~11 to keep, ~2 benchmarks to port. Flags glyph + apply-over-defaults behaviors that need MEC-side ports rather than straight deletion. - Records the reproduce command + threshold for the eventual size delta. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Resolve D-02 to alpha-lite (detect+warn, no library-side migration) Per source session decision: - Spec section 5.4 rewritten: pure-nested MEC read path + ~20 LOC peek-and-warn in TuiConfigurationBuilder.AddTuiJsonFile. Legacy shapes are NOT parsed; affected settings fall through to defaults. - Phase D scope shrinks: no TuiConfigMigrator, no LegacyTuiConfigurationSource, no round-trip tests. Two tests only (flat-key warn, array-themes warn). - Standalone Tools/MigrateConfig/ console app (not shipped in Terminal.Gui.dll) as migration aid; deletable any time. - Records explicitly-rejected alternatives (keep-both, silent-translate, throw) so future reviewers don't re-litigate. - Updated section 7 JSON-breaking-change wording and section 8 risk row to reflect the warn-and-default contract. - Prep artifact section 5 updated to reflect the now-unblocked Phase D step list. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Add Tools/MigrateConfig console app for legacy config.json migration Standalone .NET 10 console utility, ~190 LOC including comments. Not included in Terminal.slnx and not part of any shipping artifact - exists solely so users on the legacy flat-key config.json shape have a one-shot upgrade path when the library stops parsing it. Transforms: - Top-level dotted property names split into nested objects. - 'Themes' array-of-single-key-objects collapses to dict. - 'Schemes' inside a theme follows the same collapse. Verified end-to-end on Terminal.Gui/Resources/config.json: 52 KB flat input -> 49 KB nested output, structurally correct. Includes Tools/README.md establishing the folder convention (not Examples, not in Terminal.slnx, deletable any release) and Tools/MigrateConfig/README.md with usage + lifecycle notes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Add ThemeDefinition POCO bind target for nested Themes config (foundation only) Step 1 of N in the Phase A2/D commit chain for #5416. Lands the bind target for the post-D-rewrite nested `Themes` section in config.json. No production code consumes ThemeDefinition yet; the consumer (rewired MecThemeManager reading via IOptionsMonitor<ThemeSettings>) lands in a subsequent commit. Binding correctness validated by tests against in-memory MEC providers. Shape (named explicitly so reviewers can object to specific subsections): public class ThemeDefinition { public Dictionary<string, Scheme>? Schemes; // 18 nullable per-component override POCOs, matching every ThemeScope- // flavored BindSection<T> call in TuiConfigurationBuilder.ApplyToStaticFacades: public ButtonSettings? Button; public CheckBoxSettings? CheckBox; public CharMapSettings? CharMap; public DialogSettings? Dialog; public FrameViewSettings? FrameView; public HexViewSettings? HexView; public LinearRangeSettings? LinearRange; public MenuBarSettings? MenuBar; public MenuSettings? Menu; public MessageBoxSettings? MessageBox; public NerdFontsSettings? NerdFonts; public PopoverMenuSettings? PopoverMenu; public SelectorBaseSettings? SelectorBase; public StatusBarSettings? StatusBar; public TextFieldSettings? TextField; public TextViewSettings? TextView; public WindowSettings? Window; public GlyphSettings? Glyphs; // JSON section name is "Glyphs", not "Glyph" } Design choice: null = no theme-level override. Considered alternatives, rejected: (a) "Missing entry in dictionary" - forces a stringly-typed lookup; loses the IDE/compiler awareness that a strongly-typed nullable property gives. (b) "Explicit empty object" - makes "I appear in JSON but override nothing" indistinguishable from "I appear in JSON to assert my-own-values-as-overrides". Nullability avoids that ambiguity at the cost of zero ergonomics. Whether a non-null subsection (i) wholesale-replaces the root *Settings or (ii) property-level merges with it is a manager-rewire concern and is NOT encoded in the POCO. Both consumption strategies are compatible with this shape. Surfaced as an open design question for the PR thread. Source-gen registrations added for ThemeDefinition and Dictionary<string, ThemeDefinition>. These are additive, AOT-safe, and meaningful even before any consumer exists (they unblock the next commit without further SourceGenerationContext edits). Tests (Tests/UnitTestsParallelizable/Configuration/ThemeDefinitionBindingTests.cs): - Bind_FullAndPartialThemes_PartialHasNullsInOmittedSubsections - binds a nested sample with one fully-populated theme and one partial-override theme, asserts the partial theme has nulls in every unmentioned subsection. - Bind_SchemesDictionaryInsideTheme_PopulatesSchemes - binds Schemes as a Dictionary<string, Scheme> inside a ThemeDefinition. Verifies the MEC reflection binder handles Scheme directly via its parameterless ctor + init-only Normal property (PASSES - no SchemeDefinition DTO wrapper needed). - Bind_EmptyThemesSection_ProducesEmptyDictionary - degenerate case. All tests use AddJsonStream against in-memory JSON, not Resources/config.json, so they remain valid while the embedded library config keeps its legacy flat shape through Commits A and B. Explicit deferrals (subsequent commits): - MecThemeManager/MecSchemeManager rewire to consume ThemeDefinition via IOptionsMonitor<ThemeSettings>. - SwitchTheme overlay/replace algorithm. - Adding `Themes: Dictionary<string, ThemeDefinition>` to ThemeSettings. - TuiConfigurationBuilder.ApplyToStaticFacades themes binding. - Removal of legacy `ThemeManager.Theme = ...` and `.GetThemeNames()` delegation. - Resources/config.json rewrite to nested. - Peek-and-warn for legacy shapes. - ScopeJsonConverter deletion. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Spike: document MEC binder accessibility for two-pass overlay (A2 prep) Before mass-converting 18 *Settings POCOs to records with { get; internal set; } per the A2 design contract, validated the binder's behavior on the proposed accessibility shapes via a focused spike. Three Facts in MecBinderAccessibilitySpike, all passing, documenting observed behavior against this MEC version: 1. TwoPassBind_InternalSet_SilentlyIgnoredByBinder `{ get; internal set; }` is NOT written by Bind(existingInstance) under default BinderOptions. Both passes complete with no exception and the POCO stays at constructor defaults. Default BindingFlags = Public|Instance exclude internal accessors. 2. TwoPassBind_InitOnly_OverlaysCorrectly `{ get; init; }` IS written by Bind(existingInstance). Root-pass populates property A; subsequent overlay pass writes property B without disturbing A. Two-pass merge semantics are preserved end-to-end. 3. TwoPassBind_InternalSet_WorksWithBindNonPublicProperties Opting into o.BindNonPublicProperties = true at each Bind() call site rescues internal set. Trade-off: extra trim hint, non-default code path, two-line invocation at every bind site. Implication for the A2 manager-rewire commit: do NOT use internal set as the sender's design contract proposed. Two viable replacements: (A) Use { get; init; } on all 18 *Settings records. Works with MEC's default binder. Preserves immutability to public consumers. No accessibility escape hatch. Recommended. (B) Keep { get; internal set; } and opt into BindNonPublicProperties at every MecThemeManager bind call. Works, but adds a code-path divergence from the idiomatic MEC pattern and requires manual maintenance to stay enabled. Recommendation: (A). The sender's stated objection to init ("doesn't work via all reflection binder paths") is falsified by Fact 2 in this MEC version. If the sender has a specific reflection path in mind where init breaks (e.g. AOT source-gen binder for top-level Configure<T>(), as distinct from Bind(instance)), that should be named so we can validate with another spike. No production code changed in this commit. Spike file documents the wall so the sender can re-litigate the design choice before the 430+ call-site rollout. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * A2.1: convert 17 ThemeScope POCOs to immutable records with atomic Current swap Lands the heart of A2: replaces the mutable `*Settings.Defaults` pattern with immutable `sealed record` POCOs and a `Volatile`-swapped `Current` property, binding each ThemeScope POCO through MEC's two-pass overlay (root section + `Themes:<active>:<section>`). Pattern (per POCO): public sealed record FooSettings { public T Prop { get; init; } = ...; public static FooSettings Default { get; } = new (); public static FooSettings Current { get => Volatile.Read (ref _current); internal set => Volatile.Write (ref _current, value); } private static FooSettings _current = Default; } Why `init` (not `internal set`): the spike at Tests/UnitTestsParallelizable/Configuration/MecBinderAccessibilitySpike.cs (commit 84dbd6a) proves that `Bind(existingInstance)` silently ignores `internal set` under default BindingFlags=Public|Instance, while `init` accessors are written normally. `init` keeps the binder happy without an opt-in `BindNonPublicProperties=true` at every call site. Why a record (not a class) + `with`-swap setter on the view facade: records give us a free `with` expression for atomic replacement. The static View facades (e.g. `Button.DefaultShadow`) now read `Current.X` on get and do `Current = Current with { X = value }` on set. That keeps the legacy CM `ConfigProperty.Apply` reflection-write path functional during the transition (it calls `PropertyInfo.SetValue (null, value)` against the static facade, which still works), while MEC writes `Current` wholesale. Two-pass overlay (`TuiConfigurationBuilder.BindThemeScope<T>`): T next = new (); config.GetSection (sectionName).Bind (next); // root config.GetSection ($"Themes:{activeTheme}:{sectionName}").Bind (next); // overlay apply (next); // atomic Volatile.Write to Current MEC's `Bind(existing)` only writes properties present in JSON, so unmentioned overlay properties survive — property-level merge for free, matching legacy CM `Scope.Apply` semantics. No `Merge<T>` helper, no DeepCloner equivalent. Converted (17 POCOs): ButtonSettings, CheckBoxSettings, CharMapSettings, DialogSettings, FrameViewSettings, HexViewSettings, LinearRangeSettings, MenuBarSettings, MenuSettings, MessageBoxSettings, NerdFontsSettings, PopoverMenuSettings, SelectorBaseSettings, StatusBarSettings, TextFieldSettings, TextViewSettings, WindowSettings. View facades updated to `with`-swap (16 files, ~28 setter pairs): Button.cs, CheckBox.cs, CharMap.cs, Dialog.cs, FrameView.cs, HexView.cs, LinearRangeDefaults.cs, Menu.cs, MenuBar.cs, PopoverMenu.cs, MessageBox.cs, SelectorBase.cs, StatusBar.cs, TextField.cs, TextView.cs, Window.cs, Text/NerdFonts.cs. Deferred to A2.2: `Glyphs` facade redesign. `GlyphSettings` keeps its mutable `Defaults` pattern in this commit; `TuiConfigurationBuilder` still uses `BindSection<GlyphSettings>` for it. 288 call sites under `Glyphs.X` get the dedicated commit there. Deferred to A2.3: `NerdFonts` static facade redesign (the POCO is converted here; the facade still has a setter that does `with`-swap). Deferred to A2.4: removal of uncalled public static setters on View types. Out of scope (kept mutable, SettingsScope not ThemeScope): ApplicationSettings, DriverSettings, FileDialogSettings, FileDialogStyleSettings, KeySettings, ThemeSettings, TraceSettings. Tests: - New `ThemeOverlayMergeTests` (3 Facts) validates two-pass merge: overlay-overrides-root, no-overlay-uses-root, atomic-swap-does-not-mutate. - Updated `MecSettingsTests.StaticFacade_CanBeOverridden` to use `Current`. - Full suites green: 17,275 / 0 / 17 parallelizable; 72 / 0 / 2 non-parallelizable. Cross-assembly `init`: MEC's source-gen binder emits direct assignments for `Bind(existing)`, which would fail cross-assembly against `init`-only properties. Since `MecThemeManager` and `TuiConfigurationBuilder` live in `Terminal.Gui.dll` alongside the POCOs, intra-assembly calls compile fine. End-users hypothetically calling `cfg.Bind(ButtonSettings.Current)` from their app would hit a compile error — but no real consumer does this; they read `Current.X`. Reflection binder fallback works for that case anyway. Refs #4943 (CM-to-MEC replacement spec, A2). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * A2.2: Glyphs facade redesign + GlyphSettings record conversion Converts GlyphSettings to the immutable-record + atomic-swap Current pattern (matching A2.1's 17 ThemeScope POCOs) and collapses the Glyphs static facade from a CM-reflection target into a read-only projection over GlyphSettings.Current. Changes ======= Terminal.Gui/Configuration/Settings/GlyphSettings.cs - public class -> public sealed record. - ~143 Rune properties: { get; set; } -> { get; init; }. - Replaces static `Defaults` block with: Default (compile-time truth, never reassigned) Current (Volatile-read/write, internal set) _current (private backing, init'd to Default). - Bind target shape now matches all 17 A2.1 POCOs. Terminal.Gui/Drawing/Glyphs.cs - All 144 properties rewritten from [ConfigurationProperty] public static Rune NAME { get => Defaults.NAME; set => Defaults.NAME = value; } to bare expression-bodied readers: public static Rune NAME => GlyphSettings.Current.NAME; - Setters and [ConfigurationProperty] attributes fully removed. - File-level comment about "generates default config" dropped; the SaveDefaults reflection mechanism that produced that text is dead now that Glyphs has no [ConfigurationProperty] surface. The "Resources/config.json is source of truth at runtime" half stays accurate (theme overlay can still override compile-time defaults). - Call sites in Terminal.Gui/, Tests/, Examples/ unchanged: every Glyphs.NAME reader keeps working; only the host changed. Terminal.Gui/Configuration/Settings/TuiConfigurationBuilder.cs - Glyphs binding switched from BindSection<GlyphSettings> to BindThemeScope<GlyphSettings> (root -> overlay -> atomic publish), matching the other 17 theme-overlay POCOs. - activeTheme snapshot now has a TODO(A2) marker for the future ThemeSettings record conversion (review flag #4). Terminal.Gui/Configuration/ConfigPropertyHostTypes.cs - Drops `typeof (Glyphs)` from the `_types` list and removes the matching `[DynamicDependency (PRESERVED_MEMBERS, typeof (Glyphs))]`. Glyphs is no longer a CM reflection host. Terminal.Gui/Configuration/SourceGenerationContext.cs - Drops `[JsonSerializable (typeof (Glyphs))]`. Glyphs has no JSON bind state of its own; `GlyphSettings` is the bind target and is already covered elsewhere. specs/remove-legacy-cm.md - ┬º4.2 row for `Settings/*Settings.cs` annotated with the SettingsScope vs ThemeScope pattern divergence rationale (review flag #3). Tests/UnitTestsParallelizable/Configuration/SourcesManagerTests.cs - Three Skip markers added with the rationale "A2.2: Glyphs lost [ConfigurationProperty]; Resources/config.json Glyphs.X flat keys are now CM-unknown. Test removed with CM in step D." affecting: Load_WithValidResource_UpdatesSettingsScope Load_Runtime_Overrides Load_AddsResourceSourceToCollection These tests load `Terminal.Gui.Resources.config.json` through CM's SourcesManager.Load, which deserializes flat `Glyphs.X` keys via ScopeJsonConverter looking up ConfigProperty hosts. With Glyphs no longer registered, that path throws a JsonException -> Load returns false. The legacy CM contract these tests assert (flat-key resolve against reflection-discovered hosts) is exactly what A2.2 removes for Glyphs and what step D removes wholesale. Skipping is correct; the tests die with CM. Non-Default theme Glyph dormancy ================================ The flat `Glyphs.X` keys inside non-Default theme subsections of Resources/config.json (TurboPascal 5, Anders, Dark, Light, etc.) are dormant from this commit through step D. Active theme at startup is "Default", whose theme block in config.json is intentionally empty - so default startup is structurally unchanged. Glyph values for the Default theme come from GlyphSettings's `init` defaults, which by spec are the canonical Default values. Theme switching (MecThemeManager.SwitchTheme("TurboPascal 5") etc.) does NOT apply Glyph overrides during this window: - CM path: [ConfigurationProperty] removed from Glyphs in A2.2; ScopeJsonConverter no-ops the keys. - MEC path: BindThemeScope<GlyphSettings> reads section Themes:<name>:Glyphs, but the flat key form `"Glyphs.LeftBracket"` inside the theme block is treated by MEC as a literal top-level-of-theme key, not a Glyphs:LeftBracket nested path. Step D's config.json rewrite to nested form reactivates Glyph overrides for all theme-overlay POCOs uniformly. This dormancy is the same window that applies to every other ThemeScope POCO's flat keys in non-Default themes; Glyphs is not special. Test results ============ Tests/UnitTestsParallelizable: total 17292 / 17272 passed / 0 failed / 20 skipped (baseline post-A2.1 was 17275/0/17; delta is exactly the 3 new SourcesManagerTests skips). Tests/UnitTests.NonParallelizable: total 74 / 72 passed / 0 failed / 2 skipped (unchanged from baseline). Design context ============== This is commit A2.2 of the A2 series (POCO ownership migration on PR #5416, stacked on copilot/replace-cm-with-mec). A2.1 (commit 2f7c13a) landed the 17 ThemeScope POCOs and BindThemeScope<T>. A2.2 brings the 18th (GlyphSettings) and finishes the Glyphs facade. A2.3 will repeat the facade redesign for NerdFonts. A2.4 will remove dead public static setters on Button.DefaultShadow etc. Cross-session review (PR #5411 owner) signed off on: - { get; init; } on the 18 records (concedes prior internal-set recommendation; init works on intra-assembly Bind(existing)) - Glyphs file-comment cleanup (drop the SaveDefaults half) - Three Skip markers on SourcesManagerTests as the resolution path for the CM-vs-Glyphs schema mismatch Deferred (per A2 contract) ========================== - `with`-swap setters on the view facades have a non-atomic read-modify- write window vs. MEC's Volatile.Write; zero practical impact (single- threaded reflection apply path). Bridge code removes in A2.4 / CM deletion, eliminating the only lost-write race on Current. - BindThemeScope<T>'s [UnconditionalSuppressMessage] still cites ConfigPropertyHostTypes. When CM deletes in step D, the citation will be re-justified against TuiSerializerContext's JsonSerializable entries (or wired through explicit [DynamicDependency] per POCO). - ThemeSettings itself stays mutable in this commit (record conversion is a future micro-commit; non-blocking for A2.2 review). - Resources/config.json still ships Glyphs.X as flat keys. That JSON shape is part of step B/D (nested-section rewrite), not A2.2. Refs: A2.1 = 2f7c13a, stacked on copilot/replace-cm-with-mec. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * A2.3: NerdFonts facade redesign Mops up the small static facade for NerdFonts.Enable, matching the A2.2 pattern used for the Glyphs facade. NerdFontsSettings was already converted to a sealed record + Default/Current in A2.1; A2.3 just removes the CM-reflection scaffolding on the consumer-facing static facade. Changes ======= Terminal.Gui/Text/NerdFonts.cs - NerdFonts.Enable rewritten from [ConfigurationProperty (Scope = typeof (ThemeScope))] public static bool Enable { get => NerdFontsSettings.Current.Enable; set => NerdFontsSettings.Current = NerdFontsSettings.Current with { Enable = value }; } to a bare expression-bodied reader: public static bool Enable => NerdFontsSettings.Current.Enable; - [ConfigurationProperty] attribute removed. - `with`-swap setter removed; NerdFontsSettings.Current is now exclusively written by MecThemeManager via BindThemeScope<T>. - Caller surface unchanged: every NerdFonts.Enable reader keeps working; only the host changed. Terminal.Gui/Configuration/ConfigPropertyHostTypes.cs - Drops `typeof (NerdFonts)` from the `_types` list and removes the matching [DynamicDependency (PRESERVED_MEMBERS, typeof (NerdFonts))]. NerdFonts is no longer a CM reflection host. No Resources/config.json change needed ====================================== Grep against `Resources/config.json` for `NerdFonts` returns zero matches; the file has never carried NerdFonts.X overrides. The A2.2 non-Default-theme dormancy footnote therefore does not apply here. NerdFonts.Enable resolves to the C# init default (NerdFontsSettings.Default.Enable = false) at startup and remains so unless a consumer assigns NerdFontsSettings.Current via MEC binding, which today happens only via TuiConfigurationBuilder's BindThemeScope<NerdFontsSettings> against a (currently absent) MEC section. Test results ============ Tests/UnitTestsParallelizable: total 17292 / 17272 passed / 0 failed / 20 skipped (unchanged from A2.2; no new skips, no regressions). Tests/UnitTests.NonParallelizable: total 74 / 72 passed / 0 failed / 2 skipped (unchanged). Design context ============== This is commit A2.3 of the A2 series (POCO ownership migration on PR #5416, stacked on copilot/replace-cm-with-mec). A2.1 (2f7c13a) landed the 17 ThemeScope POCOs and BindThemeScope<T>. A2.2 (441ef60 post-amend) landed the 18th (GlyphSettings) + Glyphs facade. A2.3 mops up NerdFonts. A2.4 will remove dead public static setters on Button.DefaultShadow etc. Cross-session review (PR #5411 owner) signed off on: - One-line reader pattern for NerdFonts.Enable; drop the `with`-swap setter that bridged CM's reflection write path. - ConfigPropertyHostTypes row removal mirrors A2.2's Glyphs treatment. Deferred (per A2 contract) ========================== - A2.4: removal of dead public static setters on view facades (Button.DefaultShadow, etc.) -- next commit. - Step D: CM deletion, config.json schema rewrite, BindThemeScope<T> suppression re-justification. Refs: A2.1 = 2f7c13a, A2.2 = 441ef60, stacked on copilot/replace-cm-with-mec. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * A2.4: Remove dead public static setters on view facades Removes [ConfigurationProperty (Scope = typeof (ThemeScope))] from all view-class static facade properties (Button.DefaultShadow, Dialog.DefaultBorderStyle, etc.) and collapses their bridge get/set bodies to bare expression-bodied readers, matching the A2.2 Glyphs and A2.3 NerdFonts patterns. These properties existed solely as CM-reflection bind targets — the `with`-swap setter (`Current = Current with { X = value }`) was the only writer, called by ConfigProperty.Apply via PropertyInfo.SetValue against the embedded Resources/config.json flat keys. Audit confirmed zero external callers of the setters; tests that read the getters (e.g. ButtonTests asserting `button.ShadowStyle == Button.DefaultShadow`) continue to work — only the host plumbing changed. Net effect at runtime ===================== For every affected property: - Default theme: value comes from `<X>Settings.Default.<Prop>`'s C# init default. Unchanged. - Non-Default themes (Dark, Light, TurboPascal 5, Anders, etc.): flat-key overrides like `"Button.DefaultShadow": "Opaque"` in Resources/config.json are dormant from this commit through step D. Same scope as the A2.2 Glyphs dormancy: CM path no longer matches these keys (the [ConfigurationProperty] hosts are gone) and MEC path can't read them (the JSON is still flat-keyed, not nested). Step D rewrites Resources/config.json to nested form, which reactivates non-Default theme view-facade overrides for all theme-overlay POCOs uniformly via the existing BindThemeScope<ButtonSettings> / <DialogSettings> / etc. calls in TuiConfigurationBuilder.ApplyToStaticFacades. Files changed ============= Terminal.Gui/Views/Button.cs (2 props) Terminal.Gui/Views/CheckBox.cs (1 prop) Terminal.Gui/Views/CharMap/CharMap.cs (1 prop) Terminal.Gui/Views/Dialog.cs (4 props) Terminal.Gui/Views/FrameView.cs (1 prop) Terminal.Gui/Views/HexView.cs (1 prop) Terminal.Gui/Views/LinearRange/LinearRangeDefaults.cs (1 prop) Terminal.Gui/Views/Menu/Menu.cs (1 prop) Terminal.Gui/Views/Menu/MenuBar.cs (1 prop) Terminal.Gui/Views/MessageBox.cs (2 props) Terminal.Gui/Views/Selectors/SelectorBase.cs (1 prop) Terminal.Gui/Views/StatusBar.cs (1 prop) Terminal.Gui/Views/TextInput/TextField/TextField.cs (1 prop) Terminal.Gui/Views/TextInput/TextView/TextView.cs (1 prop) Terminal.Gui/Views/Window.cs (2 props) All ThemeScope-scoped view-facade props converted; total 21 properties across 15 files. Pattern per property: Before: /// <summary>...</summary> [ConfigurationProperty (Scope = typeof (ThemeScope))] public static T Name { get => XSettings.Current.Name; set => XSettings.Current = XSettings.Current with { Name = value }; } After: /// <summary>...</summary> public static T Name => XSettings.Current.Name; Out of scope ============ SettingsScope-scoped [ConfigurationProperty] on view classes (FileDialog.MaxSearchResults, FileDialogStyle.DefaultUseColors, MenuBar.DefaultKey, PopoverMenu.DefaultKey, View.DefaultMouseBindings, View.ViewMouseBindings, BorderView.DefaultMouseBindings) are NOT touched. SettingsScope follows the mutable-Defaults pattern (per A2.1's divergence note in specs/remove-legacy-cm.md §4.2) and remains CM-managed until step D. Terminal.Gui/Configuration/ConfigPropertyHostTypes.cs - Drops 14 entries (typeof + matching [DynamicDependency]) for types whose only [ConfigurationProperty] attrs were ThemeScope and are therefore now empty hosts: Button, CharMap, CheckBox, Dialog, FrameView, HexView, LinearRangeDefaults, Menu, MessageBox, SelectorBase, StatusBar, TextField, TextView, Window. - Keeps entries that still hold SettingsScope [ConfigurationProperty]: FileDialog, FileDialogStyle, MenuBar, PopoverMenu, View, BorderView, plus the unchanged Application / Color / Driver / Key / Trace / ConfigurationManager / SchemeManager / ThemeManager facades. Tests/UnitTestsParallelizable/Configuration/ScopeJsonConverterTests.cs - Drops the one InlineData row that exercised CM's ScopeJsonConverter with `"Dialog.DefaultButtonAlignment": "End"` (Dialog.DefaultButtonAlignment is one of the 21 properties this commit removes [ConfigurationProperty] from; the converter now rejects the key as Unknown). Comment notes the rationale and the expected removal alongside CM in step D. Sibling InlineData rows that don't reference dropped facade props continue to test the converter. Test results ============ Tests/UnitTestsParallelizable: total 17291 / 17271 passed / 0 failed / 20 skipped (one fewer test row vs. A2.3 baseline because the ScopeJsonConverter InlineData row was removed by design; no failures, no new skips, ConfigPropertyHostTypes drift-detector still green because it tracks reflected hosts and we removed matching list entries in lockstep). Tests/UnitTests.NonParallelizable: total 74 / 72 passed / 0 failed / 2 skipped (unchanged). Design context ============== This is commit A2.4 of the A2 series (POCO ownership migration on PR #5416, stacked on copilot/replace-cm-with-mec). A2.1 (2f7c13a) landed the 17 ThemeScope POCOs and BindThemeScope<T>. A2.2 (441ef60) landed GlyphSettings + Glyphs facade. A2.3 (f85e930) mopped up NerdFonts. A2.4 finishes the series by removing the dead view-facade setter scaffolding. A2 status: complete. The cleared-out view-facade properties leave ButtonSettings, CheckBoxSettings, DialogSettings, FrameViewSettings, etc. as the sole owners of theme-overlayed state; MecThemeManager mutates `<X>Settings.Current` exclusively via BindThemeScope<T> intra-assembly; consumer reads go through the bare expression-bodied getters on the view facades or directly through `<X>Settings.Current`. Cross-session review (PR #5411 owner) signed off on: - Removal of public static setters on view facades; "zero external callers" audit accepted. - Inheriting the A2.2 dormancy framing — Dark/Light/etc. theme overrides for view facades are dormant from A2.4 through step D's config.json rewrite. Same window as Glyphs. Deferred to step D / later commits ================================== - Resources/config.json rewrite to nested form (B/D). - CM deletion (D). - BindThemeScope<T> [UnconditionalSuppressMessage] re-justification once ConfigPropertyHostTypes goes (D). - ThemeSettings record + Current conversion (future micro-commit). - Mec* prefix drops on the manager types (post-D). Refs: A2.1 = 2f7c13a, A2.2 = 441ef60, A2.3 = f85e930, stacked on copilot/replace-cm-with-mec. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address #5416 CR feedback test-first and rewrite configuration docs. Preserve checkbox glyph defaults; SwitchTheme applies overlays without resetting to the config theme; bind Rune scalars; collapse empty Themes arrays; replace ConfigurationManager docs with TuiConfigurationBuilder. * Fix follow-up review: ThemeChanged after overlay, public Current, legacy-shape warning, case-insensitive SwitchTheme. * Complete Phase D/E: nested config.json, delete ConfigurationManager, rewire ThemeManager/SchemeManager to MEC. * Complete remaining #5416 phases F-I: dotted RuntimeConfig overlay, rename Mec* tests, NativeAOT size. Nested library Driver/Key/PopoverMenu sections skipped BindFlatDottedKeys, so RuntimeConfig dotted keys lost. Always overlay dotted keys after nested Bind. Phase F keeps MecThemeManager/MecSchemeManager because static ThemeManager/SchemeManager facades collide. Test files renamed. NativeAotSmoke win-arm64: 23.50 MB to 23.08 MB (-1.78%). * Fixes #5631 - Update tui-config-schema.json to nested MEC shape. The hosted schema is this in-repo file (docs publish). It still described flat keys, array Themes, and ConfigurationManager.ThrowOnJsonErrors. Nested sections now match Settings POCOs; Themes/Schemes are dictionaries. * Collapse dual-stack configuration: nested-only bind, one theme switch path. Binder no longer parses dotted keys or array-Themes (warn + MigrateConfig). ThemeCatalog owns names/Find. TrySwitchTheme publishes overlays then ThemeChanged. SchemeManager implements ISchemeManager; MecSchemeManager and unused ThemeDefinition are gone. Rune bind uses RuneJsonConverter. * Align shipped docs with nested MEC configuration. Replace leftover ConfigurationProperty / dotted-key / ConfigLocations wording in docfx, lexicon, CONTRIBUTING, primers, and UICatalog help. Key bindings and ViewKeyBindings are documented as code-only. Specs are marked historical. * Fix ultrareview findings test-first: resilient config load, CWD paths, scheme merge. Fixes the 10 confirmed code-review findings on this branch, each with tests written first (25 new tests across 5 new files plus additions to 2): - Config sources can no longer crash the app at assembly load: inline and embedded JSON is validated before adding, user files get a load-exception handler, Build() falls back to library defaults, and the module initializer catches. TuiJsonErrors now has real callers, so Print() at shutdown works. - ./.tui/ config files resolve against the current directory (injectable via new currentDirectory parameters), not the app's install directory. - SchemeManager.ApplyFromConfiguration preserves runtime-added schemes and deep-merges root Schemes with the theme overlay (root-then-overlay contract); invalid scheme JSON is reported via TuiJsonErrors instead of swallowed. - ApplyToStaticFacades raises ThemeManager.ThemeChanged when the config's Theme key changes the active theme. - Legacy-shaped sources (files, TUI_CONFIG, RuntimeConfig, app-embedded) are actually skipped, making the existing warning truthful; unreadable files no longer throw; ThemeCatalog no longer filters numeric theme names. - UICatalog's config-file watcher reloads and re-applies (new TuiConfigurationBuilder.Reload) instead of only logging. - MenuBar.DefaultKey app-set values survive theme switches via an override. - MigrateConfig deep-merges mixed dotted and nested keys instead of dropping settings order-dependently. - Glyph strings parse glyph-first, so "6" is '6' while legacy numeric codepoints (e.g. 9733) still work. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NGq38GbuZPAEa5wZHkmPiZ * Restore nested JSON overlays for DefaultKeyBindings. Key bindings were ConfigurationProperty in 2.4 and were silently dropped after the MEC migration because they are not Settings POCO properties. ApplyToStaticFacades now overlays Application.DefaultKeyBindings, View.DefaultKeyBindings, and View.ViewKeyBindings from nested JSON. ConfigurationSectionJson rebuilds MEC index children as JSON arrays so PlatformKeyBinding.All deserializes. Unmentioned commands keep hard-coded defaults. * Simplify key-binding JSON bind after review. Remove the global JsonStringEnumConverter from TuiSerializerContext; Command-name dictionary keys already deserialize with the default enum converter. Move STJ deserialize onto ConfigurationSectionJson and overlay ViewKeyBindings through the same command-dict merge. Treat MEC children as a JSON array only when keys are exactly 0..n-1. * Fix hot-reload review findings: per-source resilience, overlay revert, atomic arrays. Addresses the 10 code-review findings on the reload/re-apply path: - AddTuiInlineJson rejects non-object roots and duplicate-key JSON at add time (single parse), so one bad TUI_CONFIG/RuntimeConfig source is skipped per-source instead of throwing inside builder.Build() and discarding every source. User files now flow through the same validated inline path, replacing the file-load exception handler. - TuiConfigurationBuilder builds a raw-JSON merged view of all sources (JsonMerge: objects merge, arrays replace wholesale). Key bindings and schemes bind from it, so a higher-priority source's shorter array atomically replaces a lower-priority source's instead of inheriting MEC's per-index tail merge. - KeyBindingConfiguration records what each config overlay replaced and reverts it before re-applying, so a binding removed from config regains its default on reload ("unmentioned commands keep hard-coded defaults" now holds across Reload). App-code mutations are preserved. - ApplyToStaticFacades always raises ThemeManager.ThemeChanged - a hot reload that edits the current theme's content now repaints instead of updating facades invisibly. - PopoverMenu.DefaultKey gets the same app-override treatment as MenuBar.DefaultKey (the previous fix missed its only sibling). - SchemeManager tracks runtime AddScheme entries explicitly, so updates to existing schemes survive re-apply like added schemes do; scheme binding and deep-merge now use the shared ConfigurationSectionJson/JsonMerge helpers instead of hand-rolled clone/merge/deserialize copies. - TuiJsonErrors clears at the start of each build, so a watcher re-applying a persistently malformed source reports it once, not unboundedly. - UICatalog's config watcher debounces (250ms), wraps the reload in try/catch, and disposes its timer on stop - a bad config value can no longer kill the process from the watcher thread. - Config tests use a builder that skips user files and TUI_CONFIG (internal includeUserSources ctor), so machine-local configuration cannot change test outcomes. Tests: 227 Configuration tests (10 new) plus full suites pass: 17,560 parallelizable, 32 non-parallelizable, 437 integration. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NGq38GbuZPAEa5wZHkmPiZ * Fixes #5636 - Scope MainLoopSyncContext to running sessions (await-before-RunAsync deadlock) (#5641) * Fixes #5636 - Scope MainLoopSyncContext to running sessions to unblock await-before-RunAsync. Since #5588, Init installed the app's MainLoopSyncContext as the calling thread's ambient SynchronizationContext. Any await between Init and Run/RunAsync then captured a context whose Post queues onto the main loop - which is not running yet - so the continuation (the code about to start the loop) was stranded and the app hung at startup. ConfigureAwait(false) avoided the capture, matching the workaround shipped in tui-cs/clet. - Init creates the context but no longer installs it as ambient; Begin still installs it for the duration of a session, so awaits inside event handlers keep marshaling to the main loop (the #5579 contract). - Run saves the caller's ambient context and restores it on exit, so an await after Run cannot capture a context that stopped pumping. - ResetState clears the ambient context only when it is this app's own; a foreign (caller-owned) context is left untouched. - MainLoopSyncContext.Post/Send fall back to the thread pool / inline execution once the app is no longer Initialized - callbacks posted after Shutdown/Dispose execute instead of being stranded (previously Post threw NotInitializedException via Invoke). Test-first: RunAsync_AfterAwaitFollowingInit_DoesNotDeadlock reproduced the hang (10s timeout) before the fix and passes in ~2s after. New SyncContextLifecycleTests cover the Init/Run/Dispose ambient-context contract and the post-dispose Post fallback; existing tests that encoded the #5588 ambient-at-Init behavior were updated to the new contract. Suites: 17,564 parallelizable, 32 non-parallelizable, 437 integration - all pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NGq38GbuZPAEa5wZHkmPiZ * Address CR: restore ambient context for Begin/End sessions; pool fallback after sessions end. Codex/Copilot review of #5641 flagged two gaps in the session scoping: - The documented Begin/End building-block sequence installed the app context but never restored the caller's. Begin now saves the caller's ambient context on the SessionToken and End restores it (guarded so a foreign thread's context is never clobbered); for nested sessions the outermost End restores the caller's context. - Post/Send gated only on Initialized, so a continuation captured during a session that resumed after End was queued onto a loop that may never pump again. They now gate on CanPumpPostedWork: pump while a session is running, or before the first Run completes (preserving the documented post-Init/pre-Run queueing); otherwise fall back to the thread pool / inline execution. Test-first: Begin_End_RestoresAmbientSyncContext_IncludingNestedSessions and Post_AfterSessionEnded_StillExecutesCallback both failed before the change and pass after. Suites: 17,566 parallelizable, 32 non-parallelizable, 437 integration - all pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NGq38GbuZPAEa5wZHkmPiZ --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * Harden Post-after-session thread-pool wait for Ubuntu parallel CI. The Ubuntu Parallel Unit Tests job failed on Post_AfterSessionEnded_StillExecutesCallback with a 2s ManualResetEventSlim timeout. After a session ends, Post correctly falls back to the thread pool; under parallel xunit load that callback can take longer than 2s to run. Increase the wait, assert CanPumpPostedWork is false, and dispose the app if the assertion fails. Co-authored-by: Tig <tig@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: tig <585482+tig@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Fixes #5636. Stacked on #5416 (
tig/remove-cm-followup).Root cause
Since #5588,
Initinstalled the app'sMainLoopSyncContextas the calling thread's ambientSynchronizationContext. AnyawaitbetweenInitandRun/RunAsynccaptured that context; itsPostqueues work onto the main loop — which is not running yet — so the continuation (the code about to start the loop) was stranded and the app hung at startup while the input thread spun.ConfigureAwait (false)avoids the capture, which is exactly the workaround tui-cs/clet shipped (clet#202).Fix
Initcreates the context but no longer installs it as ambient.Beginstill installs it for the duration of a session, soawaits inside event handlers keep marshaling to the main loop (the SynchronizationContext is not correctly implemented in v2 #5579 contract is preserved —Open_Calls_ContinueWith_On_UIThreadand bothInit_Posts_*tests still pass).Runsaves the caller's ambient context and restores it in afinally, so anawaitafterRunreturns can't capture a stopped-pumping context.ResetStateclears the ambient context only when it's this app's own; a caller-owned context is left untouched.MainLoopSyncContext.Post/Sendfall back to thread pool / inline execution once the app is no longerInitialized— callbacks posted afterShutdown/Disposeexecute instead of being stranded (previouslyPostthrewNotInitializedExceptionviaInvoke).Test-first
RunAsync_AfterAwaitFollowingInit_DoesNotDeadlockreproduces the exact #5636 scenario (Init→await Task.Yield ()→RunAsync) on a dedicated thread: it failed with a 10s timeout before the fix and passes in ~2s after. NewSyncContextLifecycleTestspin the Init/Run/Dispose ambient-context contract and the post-disposePostfallback. Tests that encoded the #5588 ambient-at-Init behavior (SynchronizationContextTests,Dispose_Resets_SyncContext, theInit_Posts_*post-run assertions) were updated to the new contract.Verification
Once this merges into the 5416 branch, tui-cs/clet can drop its
ConfigureAwait (false)workaround (clet#202).🤖 Generated with Claude Code
https://claude.ai/code/session_01NGq38GbuZPAEa5wZHkmPiZ