Skip to content

Fixes #4473 - Re-engineerCommand system and Fix Menus#4620

Merged
tig merged 236 commits intogui-cs:v2_developfrom
tig:copilot/fix-command-propagation-issue-clean
Feb 25, 2026
Merged

Fixes #4473 - Re-engineerCommand system and Fix Menus#4620
tig merged 236 commits intogui-cs:v2_developfrom
tig:copilot/fix-command-propagation-issue-clean

Conversation

@tig
Copy link
Copy Markdown
Member

@tig tig commented Jan 23, 2026

Fixes

Overview

Re-engineers the Terminal.Gui v2 command system for correctness, clarity, and composability. See docfx/docs/command.md for the complete deep dive.

What Changed

Core infrastructure (View.Command.cs):

  • CommandRouting enum (Direct, BubblingUp, DispatchingDown, Bridged) replaces ad-hoc boolean flags
  • Immutable CommandContext (readonly record struct) with WithCommand/WithRouting
  • CommandOutcome enum with bool? conversion shims (migration deferred — see below)
  • GetDispatchTarget / ConsumeDispatch virtuals for declarative composite-view dispatch
  • CommandBridge for cross-boundary routing (weak refs, one-way)
  • Route tracing via Trace.CommandEnabled (zero overhead in Release builds)
  • _lastDispatchOccurred reset at handler entry to prevent stale-flag bugs

View-specific:

  • Shortcut: Relay dispatch (ConsumeDispatch=false) with deferred completion via CommandView_Activated
  • OptionSelector/FlagSelector: Consume dispatch (ConsumeDispatch=true), context-dependent targets
  • MenuBar: Consume dispatch + OnActivating toggle logic for activation/deactivation
  • MenuBarItem: CommandBridge.Connect for PopoverMenu, Bridged guard, IsInitialized guard
  • Menu: OnActivating override dispatches to focused MenuItem
  • Button: IAcceptTarget, Space/Enter → Command.Accept, does NOT raise Activating

Design

See plans/finalizing-command-system.md for:

  • Architecture overview and design invariants
  • Dispatch pattern summary (relay vs. consume)
  • Phase B deviations from original plan (7 documented)
  • All completed work with commit references

See plans/menubar-reengineer.md for MenuBar bug fixes and test migration.

Tests

Suite Passed Failed Skipped
UnitTestsParallelizable 14,045 0 23
UnitTests ~1,001 0 ~22

Old non-parallelizable MenuBarTests.cs deleted; ~30 modern parallelizable tests replace it.

Porting Guide: Migrating from v2_develop

This PR changes how Activating/Accepting events work. Code written against v2_develop will need updates.

The big win: No more e.Handled = true

Previously, every Accepting/Activating handler had to set e.Handled = true or the command system would misbehave (double-firing, unexpected bubbling, or the event being silently swallowed). This was a constant source of bugs. Now, Handled is only for cancellation — set it to true only when you want to prevent the action from proceeding.

// BEFORE (v2_develop) — Handled was mandatory boilerplate
button.Accepting += (s, e) =>
{
    DoSomething ();
    e.Handled = true; // REQUIRED or things break
};

// AFTER (this PR) — just subscribe, no Handled needed
button.Accepted += (_, _) =>
{
    DoSomething ();
};

Use Accepted/Activated instead of Accepting/Activating

Previously, Accepted didn't really work — the post-events were unreliable or missing. All real work had to go in the pre-events (Accepting/Activating). Now the two-phase model works correctly:

  • Accepting/Activating (pre-events) — Use only for cancellation or validation. Set e.Handled = true to prevent the action.
  • Accepted/Activated (post-events) — Use for reacting to completed actions. No Handled property — these are non-cancellable notifications.
// BEFORE (v2_develop) — work in pre-event, must set Handled
checkBox.Activating += (s, e) =>
{
    label.Text = $"Value: {((CheckBox)s!).Value}";
    e.Handled = true;
};

// AFTER (this PR) — work in post-event, state is already updated
checkBox.Activated += (_, _) =>
{
    label.Text = $"Value: {checkBox.Value}";
};

Quick migration checklist

Old pattern New pattern
view.Accepting += (s, e) => { DoWork(); e.Handled = true; }; view.Accepted += (_, _) => { DoWork(); };
view.Activating += (s, e) => { DoWork(); e.Handled = true; }; view.Activated += (_, _) => { DoWork(); };
view.Accepting += (s, e) => { if (invalid) e.Handled = true; }; view.Accepting += (_, e) => { if (invalid) e.Handled = true; }; (unchanged — cancellation still uses pre-event)
override OnAccepting(args) { ...; return true; } override OnAccepted(ctx) { ...; } (move work to post-event unless cancelling)
override OnActivating(args) { ...; return true; } override OnActivated(ctx) { ...; } (move work to post-event unless cancelling)

Button no longer raises Activating

Button now maps all interactions (Space, Enter, mouse) to Command.Accept. It does not raise Activating/Activated. If you were subscribing to button.Activating, switch to button.Accepted.

EventArgs changes

  • Accepting/Activating use CommandEventArgs (has Handled and Context)
  • Accepted uses CommandEventArgs (has Context, no Handled)
  • Activated uses EventArgs<ICommandContext?> (has Value, no Handled)
  • ICommandContext provides Command, Source, Binding, and Routing for advanced scenarios

Remaining Work

  • MenuBar bugs: Several MenuBar interaction bugs remain that are not yet captured by unit tests (e.g., arrow key navigation edge cases, mouse interaction scenarios, dynamic title/HotKey changes). See plans/menubar-reengineer.md → Remaining Work.
  • Phase D: 267 AddCommand call sites need bool?CommandOutcome migration. Purely mechanical; conversion shims exist.
  • CommandManager extraction: View.Command.cs (1,043 lines, 6 responsibilities) is an SRP candidate. Deferred — route tracing provides sufficient debugging visibility for now.

tig and others added 9 commits January 23, 2026 12:08
- Add POST-GENERATION-VALIDATION.md with comprehensive checklist
  for validating AI-generated code before commits
- Add formatting.md with detailed spacing, brace, and blank line rules
- Update REFRESH.md to reference the validation checklist
- Update CLAUDE.md with references to new validation docs

These additions address the most common formatting violations
that AI agents make when writing code for Terminal.Gui.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add clean-code-review.md task guide for reimplementing branches
  with narrative-quality commit histories
- Remove scenario-modernization.md (outdated task)

The clean code review task provides a workflow for AI agents to
reimplement messy branch histories into clean, reviewer-friendly
commit sequences suitable for PR review.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Addresses: gui-cs#4473 (Menu etc. with Selectors are broken)

- Add command-propagation-plan.md with phased implementation plan
- Remove command-propagation-analysis.md (superseded by plan)

The plan introduces PropagatedCommands property to enable opt-in
command propagation from SubViews to SuperViews, solving the issue
where commands from nested views (e.g., FlagSelector in MenuBar)
don't reach their containing views.

Phase breakdown:
1. Foundation - PropagatedCommands infrastructure
2. WeakReference - Lifecycle-safe CommandContext.Source
3. Shortcut propagation - Enable Command.Activate for Shortcuts
4. Test cleanup - Re-enable skipped tests
5. MenuBar propagation - Full menu hierarchy support (deferred)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Addresses: gui-cs#4473 (Menu etc. with Selectors are broken)

This commit implements Phases 1 and 2 of the command propagation plan:

Phase 1 - PropagatedCommands Infrastructure:
- Add PropagatedCommands property to View (default: [Command.Accept])
- Add PropagateCommand helper method for command bubbling
- Refactor RaiseAccepting to use PropagateCommand helper
- Reorganize View.Command.cs regions for clarity

Phase 2 - WeakReference for CommandContext.Source:
- Change ICommandContext.Source from View? to WeakReference<View>?
- Update CommandContext to use WeakReference for lifecycle safety
- Update InvokeCommand overloads to wrap 'this' in WeakReference
- Update all views using ctx.Source to use TryGetTarget pattern:
  - Dialog, DialogTResult
  - MenuBar, PopoverMenu
  - ComboBox, OptionSelector
  - ScrollSlider, Shortcut

The PropagatedCommands property enables opt-in command propagation:
when a SubView raises a command that isn't handled and the command
is in the SuperView's PropagatedCommands list, the command bubbles up.

BREAKING CHANGE: ICommandContext.Source is now WeakReference<View>?
Code accessing ctx.Source must use TryGetTarget pattern:
  if (ctx?.Source?.TryGetTarget (out View? view) == true) { ... }

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add a standalone test application for verifying Command.Activate
propagation through the Shortcut → Window hierarchy.

The example demonstrates:
- CheckBox (CanFocus=false) in Shortcut → propagates to Window
- CheckBox (CanFocus=true) in Shortcut → propagates to Window
- Button in Shortcut → propagates to Window

This serves as a manual test bed for the PropagatedCommands feature
and helps verify that command context (Source) is properly preserved
when commands bubble up through the view hierarchy.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add tests to verify PropagatedCommands and WeakReference changes:

CommandPropagationTests.cs (NEW - 588 lines):
- Tests for command propagation through view hierarchies
- Tests for WeakReference lifecycle safety
- Tests for PropagatedCommands customization
- Tests for Command.Accept and Command.Activate propagation

ViewCommandTests.cs (updated):
- Add tests for PropagatedCommands default value
- Add tests for PropagateCommand helper behavior
- Add tests for IsDefault button propagation

CommandContextTests.cs (updated):
- Update tests for WeakReference-based Source property
- Add TryGetTarget pattern tests

InputBindingTests.cs (updated):
- Update tests for WeakReference compatibility

All tests follow Terminal.Gui patterns and are added to
UnitTestsParallelizable for parallel execution.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
command.md:
- Document PropagatedCommands property
- Explain command propagation behavior
- Add examples for customizing propagated commands

events.md:
- Add section on command events and propagation
- Document CommandEventArgs and ICommandContext
- Explain WeakReference pattern for Source property

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Update scenarios to use TryGetTarget pattern for accessing
CommandContext.Source after the WeakReference change:

Menus.cs:
- Update command handlers to use TryGetTarget

MouseTester.cs:
- Update mouse event handlers for new pattern

UICatalogRunnable.cs:
- Update command context handling

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Change absolute path to relative path for cross-machine compatibility.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 23, 2026

Codecov Report

❌ Patch coverage is 80.83709% with 467 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.51%. Comparing base (7678411) to head (f79d564).
⚠️ Report is 2 commits behind head on v2_develop.

Files with missing lines Patch % Lines
Terminal.Gui/Views/Menu/PopoverMenu.cs 67.08% 29 Missing and 24 partials ⚠️
Terminal.Gui/Views/Menu/MenuBar.cs 81.81% 29 Missing and 17 partials ⚠️
Terminal.Gui/Views/LinearRange/LinearRange.cs 69.72% 23 Missing and 10 partials ⚠️
Terminal.Gui/ViewBase/View.Command.cs 88.66% 6 Missing and 22 partials ⚠️
Terminal.Gui/Views/Menu/MenuBarItem.cs 84.50% 15 Missing and 7 partials ⚠️
Terminal.Gui/Views/Selectors/SelectorBase.cs 62.71% 7 Missing and 15 partials ⚠️
Terminal.Gui/App/Tracing/LoggingBackend.cs 61.22% 14 Missing and 5 partials ⚠️
Terminal.Gui/Views/Shortcut.cs 91.51% 9 Missing and 10 partials ⚠️
Terminal.Gui/Views/ProgressBar.cs 25.00% 14 Missing and 1 partial ⚠️
Terminal.Gui/Input/CommandBridge.cs 73.58% 8 Missing and 6 partials ⚠️
... and 57 more
Additional details and impacted files
@@              Coverage Diff               @@
##           v2_develop    #4620      +/-   ##
==============================================
+ Coverage       77.34%   77.51%   +0.17%     
==============================================
  Files             450      457       +7     
  Lines           45200    45623     +423     
  Branches         6593     6745     +152     
==============================================
+ Hits            34959    35364     +405     
+ Misses           8352     8301      -51     
- Partials         1889     1958      +69     
Files with missing lines Coverage Δ
Terminal.Gui/App/ApplicationImpl.cs 88.37% <ø> (ø)
Terminal.Gui/App/IApplication.cs 0.00% <ø> (ø)
Terminal.Gui/App/PopoverBaseImpl.cs 82.22% <100.00%> (+0.82%) ⬆️
Terminal.Gui/App/Runnable/SessionToken.cs 100.00% <100.00%> (ø)
Terminal.Gui/App/Tracing/ListBackend.cs 100.00% <100.00%> (ø)
Terminal.Gui/Configuration/Scope.cs 82.22% <ø> (ø)
Terminal.Gui/Configuration/SourcesManager.cs 96.40% <ø> (ø)
Terminal.Gui/Configuration/ThemeManager.cs 58.64% <ø> (ø)
Terminal.Gui/Drawing/Glyphs.cs 100.00% <100.00%> (ø)
Terminal.Gui/Input/CommandBindingsBase.cs 95.52% <100.00%> (ø)
... and 81 more

... and 7 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bca191c...f79d564. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@tig tig changed the title Fix command propagation for nested views (Shortcut hierarchy) Fixes #4473 - Fix command propagation for nested views (Shortcut hierarchy) Jan 23, 2026
@tig tig changed the title Fixes #4473 - Fix command propagation for nested views (Shortcut hierarchy) Fixes #4473 - Fix command propagation for nested views Jan 23, 2026
tig added 2 commits January 23, 2026 15:59
Clarify and strengthen event forwarding between Shortcut and its CommandView, ensuring Activating and Accepting events propagate correctly without recursion. MouseHighlightStates now defaults to MouseState.In for better UI feedback. Refactor property implementations and clean up code. Add comprehensive unit tests verifying CheckBox state changes, event propagation, and default behaviors. Enhance ShortcutTestWindow to log detailed event info and demonstrate propagation scenarios.
@tig tig marked this pull request as draft January 26, 2026 19:44
tig added 11 commits January 26, 2026 12:46
- Updated Key equality operators to handle nulls safely and avoid NullReferenceExceptions.
- ComboBox Accept command now uses non-nullable View in TryGetTarget and passes a WeakReference<View> to CommandContext for consistency.
- Shortcut.Key property now always returns Key.Empty if unset, and setter no longer throws on null.
- Simplified GetWidthDimAuto by removing unnecessary null-forgiving operators.
- Fixed CommandViewOnActivating logic to prevent recursion and ensure correct event propagation.
- Cleaned up redundant key initialization and clarified KeyView.Text assignment.
- Overall, these changes improve code safety, event handling, and clarity.
Refactored the Shortcut class to replace explicit backing fields
(_alignmentModes and _minimumKeyTextSize) with auto-implemented
properties for AlignmentModes and MinimumKeyTextSize. Set default
values directly in the property declarations and used the C# field
keyword in setters. This streamlines property definitions and
simplifies the codebase.
…/Terminal.Gui into copilot/fix-command-propagation-issue-clean
…/Terminal.Gui into copilot/fix-command-propagation-issue-clean
Improve command/event handling for menus and shortcuts

Refactor Shortcut, Menu, MenuBar, and PopoverMenu to enhance command propagation, event handling, and resource management. Set PropagatedCommands by default, improve design-time support, and clarify event subscriptions. Add helper methods for event origin detection, enforce proper menu hierarchy, and update documentation and logging for better maintainability and debugging.
@tig tig marked this pull request as ready for review February 23, 2026 22:28
@tig
Copy link
Copy Markdown
Member Author

tig commented Feb 23, 2026

@tznind this is pretty ready. Would love your eyes on it please.

@tig tig requested review from Copilot and tznind February 23, 2026 22:29
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 154 out of 223 changed files in this pull request and generated 11 comments.

Files not reviewed (1)
  • Terminal.Gui/Resources/Strings.Designer.cs: Language not supported

Comment thread Terminal.Gui/App/Keyboard/KeyboardImpl.cs
Comment thread Terminal.Gui/Views/ScrollBar/ScrollBar.cs Outdated
Comment thread Terminal.Gui/Input/Keyboard/Key.cs Outdated
Comment thread Terminal.Gui/ViewBase/WeakReferenceExtensions.cs Outdated
Comment thread Terminal.Gui/Input/CommandBridge.cs Outdated
Comment thread Terminal.Gui/Input/Keyboard/KeyBinding.cs Outdated
Comment thread Terminal.Gui/Views/CheckBox.cs
Comment thread Terminal.Gui/Views/FileDialogs/FileDialog.cs Outdated
Comment thread Examples/UICatalog/Scenarios/EditorsAndHelpers/NavigationEditor.cs Outdated
Comment thread Tests/UnitTests/Views/ButtonTests.cs
tig and others added 7 commits February 23, 2026 14:57
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Refactored ArrangementEditor and NavigationEditor: swapped their responsibilities and names for clarity, updated constructors, fields, and event handlers. ArrangementEditor now edits arrangement-related properties; NavigationEditor edits TabStop.

Added a Null glyph ('␀') to Glyphs with configuration support. Standardized [ConfigurationProperty] attribute formatting in Glyphs.cs.

Fixed Key.cs != operator to handle nulls correctly. Improved ToString() for KeyBinding and MouseBinding to show Data values. WeakReferenceExtensions now uses the Null glyph for dead references.

Fixed trace logging typo in CommandBridge. Corrected ValueChangedUntyped argument order in ScrollBar. Removed redundant OnAccepted in FileDialog and debug log in CheckBox.
…/Terminal.Gui into copilot/fix-command-propagation-issue-clean
The private static string field _null was removed from the Glyphs class in the Terminal.Gui.Drawing namespace as it was unused. No other changes were made.
Accept no longer triggers Action or TargetView commands directly.
Now, Accept is translated to Activate only for key bindings (e.g., Enter).
Moved OnAccepting override to Shortcut; MenuItem's is commented out.
Updated tests to verify Accept does not execute Action or commands.
PopoverMenu Enter key handling removed; MenuBar/Menus updated.
Clarifies distinction between Accept (confirmation) and Activate (action).
- Use null-conditional operators (`?.`) for safer access to `MenuBar` and related properties/methods in `Menus.cs`.
- Fix help text typo and remove redundant `CommandsToBubbleUp` assignment in `UICatalogRunnable.cs`.
- Enhance logging in `View.Hierarchy.cs` by using `ToIdentifyingString()` and clarify `SuperView` checks.
- Comment out `MouseBindings.Clear()` in `SelectorBase.cs` to preserve mouse bindings.
- Refactor `Shortcut.cs`:
  - Use a switch expression for concise `HelpView` margin logic.
  - Rewrite `ShowHide()` to avoid redundant add/remove operations and maintain correct subview order.
  - Remove obsolete comments for clarity.
- These changes improve code safety, readability, and maintainability, and address minor UI and logging issues.
Enhanced OptionSelector focus for keyboard navigation. Added Ctrl+hotkeys to trace toggles in UICatalogRunnable and clarified help text. Expanded OptionSelector cycling logic for better keyboard interaction. Uncommented MouseBindings.Clear() in SelectorBase to reset mouse bindings. Commented out TransparentMouse viewport setting in Shortcut views to adjust mouse event handling.
@tig
Copy link
Copy Markdown
Member Author

tig commented Feb 24, 2026

I will merge this in the morning....

tig added 3 commits February 24, 2026 06:36
Introduced Lifecycle trace category for Application/Driver events. Updated Trace class to support five categories (Lifecycle, Command, Mouse, Keyboard, Navigation), each independently configurable. Added Trace.LifecycleEnabled property and calls throughout ApplicationImpl and MainLoopCoordinator for detailed lifecycle tracing.

Renamed TraceEntry.ViewId to Id for generality. Simplified LoggingBackend formatting and added support for new categories. Updated config.json to enable lifecycle tracing by default. UI catalog trace menu now includes lifecycle toggle; other labels simplified.

Refactored methods for clarity and brevity. Updated documentation and tests to use new Id field. Minor bug fixes and improved event handling.
@tig tig changed the title Fixes #4473 - Fix Command system to support bubbling etc... Fixes #4473 - Re-engineerCommand system and Fix Menus Feb 25, 2026
Resolved 5 conflicts:
- ApplicationImpl.Driver.cs: kept both using statements
- ApplicationImpl.Lifecycle.cs: took v2_develop guard clause fix
- MainLoopCoordinator.cs: took v2_develop guard clause refactor
- ColorPicker.16.cs: kept HEAD's OnActivated override (new CWP pattern)
- View.md: took v2_develop's updated scrolling docs from PR gui-cs#4748

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

4 participants