Skip to content

Phase B: Fix OptionSelector mutual exclusivity broken by dispatch mechanism#61

Closed
Copilot wants to merge 179 commits intov2_developfrom
copilot/excellent-tyrannosaurus
Closed

Phase B: Fix OptionSelector mutual exclusivity broken by dispatch mechanism#61
Copilot wants to merge 179 commits intov2_developfrom
copilot/excellent-tyrannosaurus

Conversation

Copy link
Copy Markdown

Copilot AI commented Feb 19, 2026

Proposed Changes/Todos

Phase B dispatch mechanism broke OptionSelector's mutual exclusivity - clicking a non-active item would activate it without deactivating others. Root cause: removed OnActivating override and CheckBox event subscriptions that managed shared selection state.

Changes

  • Restored OnActivating override - Applies value change when CheckBox bubbles or direct command invokes, calls UpdateChecked() to synchronize all CheckBox states
  • Restored CheckBox event subscription pattern - Subscribe to CheckBox.Activating, invoke command on OptionSelector, set args.Handled = true to prevent CheckBox from toggling itself (OptionSelector owns selection state)
  • Added OnHandlingHotKey override - Restores HotKey cycling behavior
  • Smart dispatch prevention - GetDispatchTarget returns null when command already bubbling from CheckBox subview (prevents double-processing)
  • Loop prevention - Event handler checks CommandRouting.DispatchingDown to avoid infinite recursion when OptionSelector dispatches to CheckBox

Key Insight

Views with ConsumeDispatch = true that own shared state must subscribe to subview events to intercept commands and centrally apply state changes. Prevents subviews from independently modifying shared state.

Test Results

  • Before: 13,965/14,002 passing (15 failures: 11 OptionSelector/SelectorBase, 4 Shortcut)
  • After: 13,974/14,002 passing (6 failures: 2 OptionSelector edge cases, 4 Shortcut edge cases)
  • Fixed 9 tests, 60% reduction in failures

Remaining 6 failures are edge cases involving direct command invocation and nested consume/relay view interactions.

Pull Request checklist:

  • I've named my PR in the form of "Fixes #issue. Terse description."
  • My code follows the style guidelines of Terminal.Gui
  • My code follows the Terminal.Gui library design guidelines
  • I ran dotnet test before commit
  • I have made corresponding changes to the API documentation (using /// style comments)
  • My changes generate no new warnings
  • I have checked my code and corrected any poor grammar or misspellings
  • I conducted basic QA to assure all features are working
Original prompt

Implement Phases A-E of the plan

**Chronological Review:** 1. User asked agent to act like an Amazon PE and review a design document in `./plans` 2. Agent found and read `plans/command-system-redesign-requirements.md` - a comprehensive Terminal.Gui v2 Command System redesign document 3. Agent fetched Amazon PE tenets page (minimal content returned) 4. Agent dispatched an explore task to understand the current codebase implementation across 8 key files 5. Agent directly viewed `View.Command.cs`, `CommandContext.cs`, `ICommandContext.cs`, `ICommandBinding.cs` to validate claims 6. Agent viewed specific sections of `View.Command.cs` (DefaultAcceptHandler, RaiseAccepting, DefaultActivateHandler, RaiseActivating, TryBubbleUp, BubbleDown) 7. Agent grepped Shortcut.cs and FlagSelector.cs to validate deletion claims 8. Agent delivered a comprehensive PE-level review with 5 critical issues, 4 gaps, and minor nits 9. User responded with gap resolutions and said "/delegate Implement Phases A-E of the plan" 10. User provided PR link: https://github.com/gui-cs/pull/4620 11. Agent read the PR details (massive PR: 161 commits, 183 files, +15876/-9760 lines) 12. Agent checked git branch (copilot/fix-command-propagation-issue-clean) and status 13. User reported `/delegate` error: "Could not determine GitHub repository from git remotes" 14. Agent diagnosed malformed git remotes (SSH aliases `tig:` instead of standard URLs) 15. Agent fixed remotes to use HTTPS URLs, told user to retry `/delegate`

Intent Mapping:

  • Initial: "Act like an amazon PE... Read the doc in ./plans" → Review the command system redesign plan
  • Response to review: Gap resolutions (Gap 1: treat as null, Gap 2: don't worry, Gap 3: support both ILogger and Conditional, Gap 4: preserve Padding special-casing)
  • Action: "/delegate Implement Phases A-E of the plan" → User wants to delegate implementation to Copilot coding agents
  • Troubleshooting: Fix git remote issue preventing /delegate

Technical Inventory:

  • Terminal.Gui v2 - Cross-platform terminal UI toolkit for .NET
  • Command system redesign with 8 changes across 5 phases (A-E)
  • PR Fixes #4473 - Re-engineerCommand system and Fix Menus gui-cs/Terminal.Gui#4620 on branch copilot/fix-command-propagation-issue-clean
  • Key types: CommandOutcome, CommandRouting, CommandContext, CommandBridge, GetDispatchTarget/ConsumeDispatch
  • Current branch already has significant work done (161 commits, 183 files changed)

Code Archaeology:

  • View.Command.cs (~819 lines): Core command handling with bool? tri-state, RaiseActivating/RaiseAccepting/TryBubbleUp/BubbleDown
  • CommandContext.cs: Mutable record struct with Command/Source/Binding/IsBubblingUp/IsBubblingDown
  • ICommandContext.cs: Interface with mutable Command set accessor
  • ICommandBinding.cs: Source is View? (strong ref) — inconsistent with context's WeakReference<View>?
  • Shortcut.cs (~950 lines): Complex HandleActivate, deferred activation flags, CommandView_Activated wiring
  • FlagSelector.cs: _suppressHotKeyActivate temporal coupling, OnActivating with 3 mixed concerns
  • OptionSelector.cs: OnActivating override consuming bubble
  • PopoverMenu.cs: Manual event subscription across boundaries
  • MenuBarItem.cs: Event wiring in property setters, commented-out RaiseActivated

Progress Assessment:

  • PE review: COMPLETE — delivered with 5 critical issues, 4 gaps, minor nits
  • Gap resolution from user: COMPLETE — all 4 gaps addressed
  • PR exists with substantial work already done (status in PR shows most streams DONE)
  • /delegate attempt: BLOCKED by git remote issue → FIXED
  • User needs to retry /delegate

Context Validation:
The user wants to delegate implementation of Phases A-E to Copilot coding agents. The git remote issue was blocking this and has been fixed.

Recent Commands Analysis:

  1. github-mcp-server-pull_request_read - Read PR Fixes #4473 - Re-engineerCommand system and Fix Menus gui-cs/Terminal.Gui#4620 details (open, 161 commits, 183 files, mergeable, on branch copilot/fix-command-propagation-issue-clean, base: v2_develop)
  2. powershell - git branch --show-current && git status --short → branch is copilot/fix-command-propagation-issue-clean, one deleted file in status
  3. powershell - git remote -v → Found malformed remotes using SSH alias tig:
  4. powershell - git remote set-url origin/upstream → Fixed to HTTPS URLs
1. Conversation Overview: - Primary Objectives: User asked agent to "Act like an amazon PE" and review `./plans/command-system-redesign-requirements.md` — a comprehensive Terminal.Gui v2 Command System redesign plan. After the review, user provided gap resolutions and requested: "/delegate Implement Phases A-E of the plan" targeting PR gui-cs#4620. - Session Context: Agent performed a thorough PE-level architecture review of an 8-change, 5-phase command system redesign. The review identified 5 critical issues, 4 gaps, and minor nits. User resolved all gaps and attempted to delegate implementation to Copilot coding agents, but hit a git remote configuration issue whi...

Created from Copilot CLI via the copilot delegate command.


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

tig and others added 30 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>
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.
- 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.
…/Terminal.Gui into copilot/fix-command-propagation-issue-clean
Renames PropagatedCommands to CommandsToBubbleUp for clarity and consistency across all view classes. Command bubbling (for Accept/Activate) is now opt-in via this property; by default, no commands bubble up. Event handling is clarified: Accepted is only raised if Accepting is not handled, and bubbling only occurs for commands listed in CommandsToBubbleUp. Updates all affected classes, refactors and expands tests to cover new bubbling logic, and revises documentation and planning guides. This is a breaking change for code using the old PropagatedCommands property.
Pressing Enter now triggers Accepted event in single-line mode.
Accept command is no longer added but returns true when invoked.
Tests updated for new behavior. Minor list initialization cleanup.
Copilot AI and others added 2 commits February 19, 2026 01:31
…mmutable CommandContext, WeakReference bindings)

Co-authored-by: tig <585482+tig@users.noreply.github.com>
Co-authored-by: tig <585482+tig@users.noreply.github.com>
Copilot AI changed the title [WIP] Implement phases A-E of the plan Phase A: Command System Foundation Types - Immutable Context, Routing Enum, WeakReference Bindings Feb 19, 2026
Copilot AI requested a review from tig February 19, 2026 01:47
Co-authored-by: tig <585482+tig@users.noreply.github.com>
Copilot AI changed the title Phase A: Command System Foundation Types - Immutable Context, Routing Enum, WeakReference Bindings Phase A: Command System Foundation - Immutable Context, Routing Enum, WeakReference Bindings Feb 19, 2026
Copilot AI and others added 2 commits February 19, 2026 02:50
…elector, FlagSelector

Co-authored-by: tig <585482+tig@users.noreply.github.com>
…cut tests failing)

Co-authored-by: tig <585482+tig@users.noreply.github.com>
Copilot AI changed the title Phase A: Command System Foundation - Immutable Context, Routing Enum, WeakReference Bindings Phase A-B: Command System Redesign - Foundation Types and Dispatch Mechanism Feb 19, 2026
Copilot AI and others added 2 commits February 19, 2026 13:15
…ll failing)

Co-authored-by: tig <585482+tig@users.noreply.github.com>
…passing

Co-authored-by: tig <585482+tig@users.noreply.github.com>
Copilot AI changed the title Phase A-B: Command System Redesign - Foundation Types and Dispatch Mechanism Implement Phase A-B: Command System Foundation & Dispatch Mechanism Feb 19, 2026
…ubscription pattern

Co-authored-by: tig <585482+tig@users.noreply.github.com>
Copilot AI changed the title Implement Phase A-B: Command System Foundation & Dispatch Mechanism Phase B: Fix OptionSelector mutual exclusivity broken by dispatch mechanism Feb 19, 2026
@tig tig closed this Feb 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants