Add comprehensive analysis of CWP order and helper class migration for solving #3714

[Copilot]

Overview

This PR provides a comprehensive analysis of the Cancellable Work Pattern (CWP) order issue raised in #3714, where external code cannot prevent views like Slider from handling mouse events because virtual methods are called before events.

Problem Statement

The current CWP implementation calls virtual methods (e.g., OnMouseEvent) before raising events (e.g., MouseEvent). This means:

// What users WANT but CANNOT do today:
var slider = new Slider();
slider.MouseEvent += (sender, args) => 
{
    if (shouldDisableSlider)
        args.Handled = true; // TOO LATE - Slider.OnMouseEvent already handled it
};

Views that override OnMouseEvent can mark events as handled before external subscribers ever see them, preventing external code from intercepting or canceling the interaction.

Analysis Scope

This PR delivers the requested analysis to understand:

• Every instance where CWP-style eventing is used
• What existing code depends on the current behavior
• The potential impact of changing the CWP guidance
• NEW: Feasibility of migrating direct CWP implementations to CWP helper classes

Key Findings

The analysis identified:

• 33+ CWP implementations across the codebase (mouse, keyboard, commands, navigation, drawing, property changes)
• 100+ virtual method overrides in views that depend on current order
• Only 2 files currently use CWP helpers (View.Drawing.Scheme.cs)
• 28 direct CWP implementations that could potentially be migrated
• Explicit tests validating virtual method called before event (OrientationTests)
• Code comments documenting current order as "best practice"
• HIGH RISK for global order reversal (4-6 weeks effort, major breaking change)

Documents Created

All analysis documents are in docs/analysis/:

1. README.md - Navigation guide and quick reference
2. ISSUE_SUMMARY.md - Formatted summary for GitHub issue discussion
3. cwp_analysis_report.md - Executive summary with statistics and impact assessment
4. cwp_detailed_code_analysis.md - Technical deep-dive with actual code examples
5. cwp_recommendations.md - Implementation specifications and checklist
6. cwp_helper_migration_analysis.md - NEW: Detailed helper class migration analysis

Total: 3,089 lines of comprehensive analysis

Recommended Solution for #3714

Option 2: Add "Before" Events ✅

Instead of reversing the order globally (which would be a major breaking change), add new events that fire before virtual methods:

public partial class View
{
    public event EventHandler<MouseEventArgs>? BeforeMouseEvent;
    
    public bool RaiseMouseEvent(MouseEventArgs args)
    {
        // Phase 1: Before event (NEW) - external pre-processing
        BeforeMouseEvent?.Invoke(this, args);
        if (args.Handled) return true;
        
        // Phase 2: Virtual method (EXISTING) - view processing
        if (OnMouseEvent(args) || args.Handled) return true;
        
        // Phase 3: After event (EXISTING) - external post-processing
        MouseEvent?.Invoke(this, args);
        return args.Handled;
    }
}

Usage Example (Solves #3714)

var slider = new Slider();

// NOW THIS WORKS!
slider.BeforeMouseEvent += (sender, args) =>
{
    if (shouldDisableSlider)
    {
        args.Handled = true; // Prevents Slider.OnMouseEvent from being called
    }
};

Why This Approach?

• ✅ Solves MouseEvent/MouseClick can't be externally controlled - was Slider does not seem to respect click handlers #3714 completely - external code can prevent view handling
• ✅ Non-breaking - all existing code continues to work unchanged
• ✅ Low risk - additive change only, no modifications to existing behavior
• ✅ Clear intent - "Before" explicitly means "external pre-processing"
• ✅ Selective - apply to problematic events (mouse, keyboard), not all 33
• ✅ Future-proof - provides migration path for v3.0
• ✅ 2-3 weeks effort vs. 4-6 weeks for global reversal

Helper Class Migration Analysis

The analysis of migrating direct CWP implementations to use CWPHelper classes found:

Migration Feasibility

• Perfect fit: 1 instance (CheckBox.OnCheckedStateChanging) - Strongly recommended
• Could work with wrappers: 7 instances (Command + Keyboard events) - Marginal benefit
• Should NOT migrate: 20 instances - Custom logic, embedded patterns, or breaking changes required

Key Gotchas Identified

1. Argument type mismatches: CommandEventArgs ≠ ResultEventArgs, CancelEventArgs ≠ ValueChangingEventArgs
2. Embedded patterns: Many implementations are embedded in larger methods with complex logic
3. Pre/post conditions: Enabled checks, command invocation, state management don't fit helper model
4. Direct is often clearer: For most cases, direct implementation is more readable and maintainable

Helper Migration Recommendation

Migrate only CheckBox.OnCheckedStateChanging (1-2 days effort, LOW risk). This demonstrates effective helper usage without forcing code into abstractions where direct implementation is clearer. Full migration would be 6-8 weeks for limited benefit.

Alternative Options Considered

Option	Effort	Risk	Verdict
1. Reverse Order Globally	4-6 weeks	VERY HIGH	❌ Not Recommended
2. Add Before Events	2-3 weeks	LOW	✅ RECOMMENDED
3. Add MouseInputEnabled Property	1 week	VERY LOW	⚠️ Band-aid only
4. Reverse Mouse Events Only	2 weeks	MEDIUM	⚠️ Inconsistent

Implementation Timeline

If proceeding with recommended Option 2:

• Week 1-2: Implement BeforeMouseEvent, BeforeMouseClick, BeforeMouseWheel
• Week 3: Update documentation (events.md, cancellable-work-pattern.md)
• Week 4: Testing and refinement

For helper migration:

• 1-2 days: Migrate CheckBox.OnCheckedStateChanging as demonstration

Changes in This PR

• ✅ No production code modified (analysis only)
• ✅ Six comprehensive analysis documents created
• ✅ All documents committed to docs/analysis/
• ✅ Build verification passed (0 errors)

Next Steps

1. Review analysis documents (start with docs/analysis/ISSUE_SUMMARY.md)
2. Review helper migration analysis (docs/analysis/cwp_helper_migration_analysis.md)
3. Discuss and confirm recommended approach
4. Create follow-up PR(s) to implement chosen solution

Related Issues

Closes #3714 (analysis phase)

---

For Reviewers: Start with docs/analysis/ISSUE_SUMMARY.md for a discussion-ready summary, then review docs/analysis/cwp_helper_migration_analysis.md for the helper class migration analysis, and other documents as needed for technical details.

Original prompt

---

This section details on the original issue you should resolve

<issue_title>Cancellable Work Pattern: Revisit order of calling virtual function first, then invoking event</issue_title>
<issue_description>While investigating #3714, I think I finally understand something @BDisp has been saying for a while...

The guidance for CWP, and the implementation within the library and examples today, says:

• Call the virtual function (e.g. OnMouseEvent) first.
• If not cancelled, invoke the event (e.g. MouseEvent)

When I settled on this design, I felt it was important that

a) This behavior was consistent
b) It didn't really matter much which came first.

On b) I decided that it was slightly better to do the v-function first so that code that inherited (via override) had priority over external code (which MUST use events) or inheriting code that chose to use the event. My reasoning was inheritance implies tighter coupling and thus devs would expect to have more control. In addition, the order in which events are raised is non-deterministic per the dotnet spec, so calling v-functions first would be more deterministic.

#3417 raises a real issue with this, particuarly for MouseEvent: It is not possible for code external to a View like Slider that overrides OnMouseEvent to prevent mouse events from going to the View. The same thing applies to MouseClick.

There are several ways to fix #3714:

Implement a "disable mouse events" property in View

E.g View.IgnoreMouse

Implement more granular mouse events

For each existing event (e.g. MouseEvent and MouseClick) redefine things such that there are explicit "before" and "after" events, each following the current CWP model. E.g. MouseEvent` would become (horrible naming, but makes the point):

• BeforeMouseEvent (cancellable)
• AfterMouseEvent (not-cancellable)

AfterMouseEvent would replace the current MouseEvent (and all code that overrides it would be changed to not assume cancellation semantics).

The default impl of OnMouseEvent in View would only proceed if BeforeMouseEvent was not cancelled.

Change the CWP guidance to reverse the order such that the event is raised first, then the v-function

Here, events like MouseEvent would need to be rewritten to reverse the order. At this point I don't have a full understanding of what existing code (if any) DEPENDS on the current behavior. To this end, I'm going to use the AIs to do an analysis of the entire code base, creating a report of EVERY instance where CWP-style eventing is used and what the potential impact of changing the CWP guidance would be.

Which is why I've created this PR and am assigning it to copilot...
</issue_description>

Comments on the Issue (you are @copilot in this section)

Fixes #4262

---

💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.

[tig]

@copilot The CWPHelper classes abstract this. They currently are used in only a few places.

Estimate the work required to replace all instances where CWP is implemented directly with the helper classes. Ensure you provide a detailed analysis of each instance and call out any gotchas that might make an instance tricky.

[Copilot]

@copilot The CWPHelper classes abstract this. They currently are used in only a few places.

Estimate the work required to replace all instances where CWP is implemented directly with the helper cl...

I've completed a comprehensive analysis of migrating all direct CWP implementations to use the helper classes. The detailed analysis is in docs/analysis/cwp_helper_migration_analysis.md.

Summary:

• Current state: Only 2 files use helpers (View.Drawing.Scheme.cs)
• Direct implementations: 28 methods across 11 files
• Migration feasibility: Most instances have gotchas that make migration complex or counterproductive

Key Findings:

1. Only 1 instance is a perfect fit: CheckBox.OnCheckedStateChanging

   • ✅ Clean property change pattern
   • ✅ Effort: 1-2 days
   • ✅ Strongly recommended

2. 7 instances could work but with issues: Command events (4) + Keyboard events (3)

   • ⚠️ Argument type mismatches require wrapper logic
   • ⚠️ Marginal improvement over direct implementation
   • ⚠️ Effort: 2-3 weeks
   • ⚠️ Consider only for consistency

3. 20 instances should NOT migrate:

   • ❌ Mouse events: Custom pre/post processing (MouseHeldDown, InvokeCommandsBoundToMouse)
   • ❌ Navigation: Embedded in complex methods (AdvanceFocus)
   • ❌ Drawing: Custom pipeline with dual-overload pattern
   • ❌ Property changes: Would require breaking API changes (OnVisibleChanging)

Key Gotchas:

• Argument mismatches: CommandEventArgs ≠ ResultEventArgs, CancelEventArgs ≠ ValueChangingEventArgs
• Embedded patterns: Many CWP patterns are embedded in larger methods with complex surrounding logic
• Pre/post conditions: Enabled checks, command invocation, state management don't fit helper model
• Dual virtual methods: Drawing events have two overloads that can't fit helper pattern
• Direct is clearer: For many cases, direct implementation is more readable and maintainable

Recommendation: Migrate only CheckBox.OnCheckedStateChanging. This demonstrates helper usage effectively while avoiding complex migrations that don't add value. Full migration would be 6-8 weeks for limited benefit.

Commit: 6edae78

[tig]

@copilot what if all the work was assigned to you? Would it still take that long?

[Copilot]

@tig Unfortunately I hit an unexpected error while processing your comment. I've automatically reported this to GitHub.

You can ask me to try again later by mentioning me in a new comment.

If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: 1e2d0a2a-8623-4e0a-ad9a-a18b68ad0098

Sorry for the inconvenience!

[tig]

@copilot what if all the work was assigned to you? Would it still take that long?

[Copilot]

@tig Unfortunately I hit an unexpected error while processing your comment. I've automatically reported this to GitHub.

You can ask me to try again later by mentioning me in a new comment.

If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: dfa31e7f-9e5b-4067-85cb-321e66113a15

Sorry for the inconvenience!

[tig]

@copilot what if all the work was assigned to you? Would it still take that long?

[Copilot]

@tig Unfortunately I hit an unexpected error while processing your comment. I've automatically reported this to GitHub.

You can ask me to try again later by mentioning me in a new comment.

If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: 13106024-7c1c-4cdc-83f5-32bd3f809982

Sorry for the inconvenience!

[BDisp]

At least he should say what error was hit to try understanding the implications.

[tig]

This is pretty cool though! These are my first real uses of copilot agent like this. I can totally see how this can radically improve productivity. The analysis it did was compressive and spot-on.

[tig]

@copilot try agian

[Copilot]

@tig Unfortunately I hit an unexpected error while processing your comment. I've automatically reported this to GitHub.

You can ask me to try again later by mentioning me in a new comment.

If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: 9efa70f5-be93-4787-9b8b-e3325c12506d

Sorry for the inconvenience!