diff --git a/.github/architecture/core-vs-controls.md b/.github/architecture/core-vs-controls.md new file mode 100644 index 000000000000..240c3d3e8b26 --- /dev/null +++ b/.github/architecture/core-vs-controls.md @@ -0,0 +1,195 @@ +# Core vs Controls Layer Architecture + +Quick reference for understanding the separation between Core and Controls layers in .NET MAUI. + +## Layer Overview + +``` +┌─────────────────────────────────────────────────────────────┐ +│ Controls Layer (src/Controls/) │ +│ - Control-specific implementations │ +│ - Button, Label, Entry, etc. │ +│ - Shell, TabbedPage, NavigationPage │ +│ - Higher-level abstractions │ +└─────────────────────────────────────────────────────────────┘ + │ + │ Uses + ▼ +┌─────────────────────────────────────────────────────────────┐ +│ Core Layer (src/Core/) │ +│ - Fundamental page/view lifecycle │ +│ - Handler infrastructure │ +│ - Platform-agnostic abstractions │ +│ - Foundation for all controls │ +└─────────────────────────────────────────────────────────────┘ +``` + +## Decision Guide: Where Does My Code Belong? + +### Core Layer (`src/Core/`) + +Use Core layer when: +- ✅ Functionality applies to **ALL pages/views** (not specific to one control) +- ✅ Fundamental platform behavior (lifecycle, rendering, layout) +- ✅ Handler infrastructure and base classes +- ✅ Platform-specific integration (iOS UIKit, Android View system) + +**Examples**: +- `PageViewController` - Base view controller for ALL pages (iOS) +- `ViewRenderer` - Base renderer infrastructure +- `PlatformWindow` - Window management for all platforms +- `HandlerResolver` - Handler lookup and instantiation +- Theme change detection (applies to entire app) +- Safe area inset handling (applies to all pages) + +**Location**: `src/Core/src/Platform/{iOS|Android|Windows}/` + +### Controls Layer (`src/Controls/`) + +Use Controls layer when: +- ✅ Functionality specific to a **single control type** (Button, Label, Entry, etc.) +- ✅ Control-specific handlers and renderers +- ✅ Higher-level navigation abstractions (Shell, NavigationPage) +- ✅ Control customization and behavior + +**Examples**: +- `ButtonHandler` - Button-specific click handling +- `EntryHandler` - Text input specific behavior +- `ShellRenderer` - Shell-specific navigation +- `CollectionViewHandler` - Collection view layout and scrolling +- Control property mapping (Text → UILabel.Text) + +**Location**: `src/Controls/src/Core/Platform/{iOS|Android|Windows}/` + +### Compatibility Layer (`src/Controls/src/Core/Compatibility/`) + +Use Compatibility layer when: +- ✅ Supporting legacy Xamarin.Forms renderers +- ✅ Migration path from old renderer system +- ✅ Shell-specific legacy implementations + +**Note**: This layer is being phased out. New code should go in Core or Controls. + +## Common Mistakes + +### ❌ Mistake 1: Implementing Core Functionality in Controls + +**Bad**: Adding theme change detection to `ShellSectionRootRenderer` +```csharp +// src/Controls/.../ShellSectionRootRenderer.cs +public override void TraitCollectionDidChange(...) +{ + // ❌ Theme changes affect ALL pages, not just Shell + application.ThemeChanged(); +} +``` + +**Good**: Implement in Core layer where ALL pages benefit +```csharp +// src/Core/.../PageViewController.cs +public override void TraitCollectionDidChange(...) +{ + // ✅ All pages automatically get theme detection + application.ThemeChanged(); +} +``` + +### ❌ Mistake 2: Duplicating Functionality Across Layers + +**Problem**: Same override exists in both Core and Controls +``` +Core/PageViewController.TraitCollectionDidChange() ← Affects all pages +Controls/ShellSectionRootRenderer.TraitCollectionDidChange() ← Only Shell pages +``` + +**Result**: +- Theme change fires twice for Shell pages +- Controls override can hide Core bugs +- Race conditions during disposal + +**Solution**: Remove the Controls override, keep only Core implementation + +### ❌ Mistake 3: Putting Control-Specific Logic in Core + +**Bad**: Button click logic in `ViewRenderer` (Core) +```csharp +// src/Core/.../ViewRenderer.cs +public override void OnTouchUpInside(...) +{ + // ❌ Not all views are clickable, this is Button-specific + RaiseClicked(); +} +``` + +**Good**: Button logic in `ButtonHandler` (Controls) +```csharp +// src/Controls/.../ButtonHandler.cs +protected override void OnTouchUpInside(...) +{ + // ✅ Button-specific click handling + RaiseClicked(); +} +``` + +## iOS Lifecycle Methods - Layer Decision Matrix + +| Method | Core or Controls? | Reasoning | +|--------|-------------------|-----------| +| `TraitCollectionDidChange` | **Core** | Theme changes affect ALL pages | +| `SafeAreaInsetsDidChange` | **Core** | Safe areas affect ALL pages | +| `ViewDidLoad` | **Core** | Base lifecycle for ALL view controllers | +| `ViewWillAppear` | **Both** | Core for base, Controls for specific behavior | +| Button `TouchUpInside` | **Controls** | Button-specific interaction | +| Entry `EditingChanged` | **Controls** | Entry-specific input handling | + +## Quick Check: "Does This Belong Here?" + +Ask yourself: +1. **Does this apply to ALL pages/views?** + - YES → Probably Core + - NO → Probably Controls + +2. **Is this fundamental platform behavior?** + - YES → Core + - NO → Controls + +3. **Would a non-Shell, non-TabbedPage app need this?** + - YES → Core + - NO → Controls + +4. **Does this customize ONE specific control?** + - YES → Controls + - NO → Core + +## How to Fix Layer Violations + +### If You Find Core Functionality in Controls Layer: + +1. **Check if Core already implements it**: + ```bash + grep -r "MethodName" src/Core/src/Platform/iOS/ + ``` + +2. **If Core has it**: Remove the Controls override (it's duplicate) + +3. **If Core doesn't have it**: Move implementation to Core + +### If You Find Control-Specific Logic in Core Layer: + +1. **Extract to Controls layer handler** +2. **Keep Core layer generic** +3. **Use virtual methods in Core that Controls can override** + +## File Location Reference + +| Layer | iOS Path | Android Path | Windows Path | +|-------|----------|--------------|--------------| +| **Core** | `src/Core/src/Platform/iOS/` | `src/Core/src/Platform/Android/` | `src/Core/src/Platform/Windows/` | +| **Controls** | `src/Controls/src/Core/Platform/iOS/` | `src/Controls/src/Core/Platform/Android/` | `src/Controls/src/Core/Platform/Windows/` | +| **Compatibility** | `src/Controls/src/Core/Compatibility/Handlers/.../iOS/` | `.../Android/` | `.../Windows/` | + +## Related Documentation + +- `.github/instructions/ios-debugging.instructions.md` - iOS debugging patterns +- `docs/design/HandlerResolution.md` - Handler architecture details +- `.github/copilot-instructions.md` - General repository guidance diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 604d89989798..af18d2eae382 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -57,6 +57,19 @@ This guidance assumes: - **MacCatalyst** specific code is inside folders named `MacCatalyst` - **Windows** specific code is inside folders named `Windows` +#### Understanding Core vs Controls Layers + +.NET MAUI code is organized into layers with distinct responsibilities: + +- **Core layer** (`src/Core/`) - Fundamental page/view lifecycle for ALL platforms +- **Controls layer** (`src/Controls/`) - Control-specific implementations +- **Compatibility layer** (`src/Controls/.../Compatibility/`) - Legacy renderer system + +**Key principle**: Core functionality (theme changes, safe areas, etc.) should be implemented in the Core layer where it applies to ALL pages, not duplicated in Controls. + +📖 **See** `.github/architecture/core-vs-controls.md` for layer decision guidance. +📖 **See** `.github/instructions/ios-debugging.instructions.md` for iOS debugging patterns including duplication detection. + ### Platform-Specific File Extensions Platform-specific files use naming conventions to control compilation: diff --git a/.github/instructions/ios-debugging.instructions.md b/.github/instructions/ios-debugging.instructions.md new file mode 100644 index 000000000000..20dc70723c37 --- /dev/null +++ b/.github/instructions/ios-debugging.instructions.md @@ -0,0 +1,195 @@ +# iOS/MacCatalyst Debugging and Problem-Solving Guidelines + +This instruction file provides guidance for debugging iOS and MacCatalyst issues in .NET MAUI. + +## When This Applies + +This guidance applies when: +- Debugging crashes on iOS or MacCatalyst platforms +- Investigating issues in iOS-specific code paths +- Working with iOS lifecycle methods (ViewDidLoad, TraitCollectionDidChange, etc.) +- Fixing platform-specific bugs in handlers or renderers + +## Critical Debugging Principles + +### 1. Search for Patterns, Not Just Named Files + +**❌ BAD APPROACH**: Trust the stack trace filename and only look there + +**✅ GOOD APPROACH**: Search for the pattern across the entire codebase + +When a crash occurs in a specific method (e.g., `TraitCollectionDidChange`, `ViewDidLoad`, `ViewWillAppear`): + +1. **Search for ALL occurrences** of that method across the codebase: + ```bash + # Search for method overrides + grep -r "override.*TraitCollectionDidChange" src/ + grep -r "override.*ViewDidLoad" src/ + + # Search for interface implementations + grep -r "void TraitCollectionDidChange" src/ + ``` + +2. **Check multiple layers** where the method might be implemented: + - `src/Core/src/Platform/iOS/` - Core framework implementations + - `src/Controls/src/Core/Platform/iOS/` - Controls layer + - `src/Controls/src/Core/Compatibility/` - Legacy compatibility handlers + - `src/Essentials/src/` - Essentials platform implementations + +3. **Look for duplication** - If the same method exists in multiple files, the bug might be in a different file than where it crashed + +### 2. Understand iOS Layer Architecture + +iOS code in .NET MAUI is organized in layers: + +| Layer | Location | Responsibility | Examples | +|-------|----------|----------------|----------| +| **Core** | `src/Core/src/Platform/iOS/` | Fundamental page/view lifecycle | PageViewController, ViewRenderer | +| **Controls** | `src/Controls/src/Core/Platform/iOS/` | Control-specific handlers | ButtonHandler, LabelHandler | +| **Compatibility** | `src/Controls/src/Core/Compatibility/` | Legacy renderer system | ShellRenderer, TabbedRenderer | + +**Key principle**: Functionality should be implemented at the LOWEST appropriate layer. + +- **Theme changes** → Core layer (affects ALL pages) +- **Button clicks** → Controls layer (specific to Button) +- **Shell navigation** → Compatibility layer (Shell-specific) + +### 3. Check for Duplicate Implementations + +**Common pattern that causes bugs**: +1. Feature was originally implemented in Controls or Compatibility layer +2. Later, feature was moved to Core layer for broader applicability +3. Original implementation was NOT removed, creating duplication +4. Duplication causes race conditions, double-firing, or crashes + +**How to detect**: +```bash +# Example: Check if TraitCollectionDidChange exists in multiple layers +find src/ -name "*.cs" -exec grep -l "TraitCollectionDidChange" {} \; | grep -E "(Core|Controls|Compatibility)" + +# Check for duplicate implementations +grep -r "public override void TraitCollectionDidChange" src/ | wc -l +# If count > expected number of unique classes, investigate duplication +``` + +### 4. Consider "Remove vs Patch" for Fixes + +When you find a bug in a method override: + +**Ask these questions BEFORE patching**: +1. Is this override still needed, or was it superseded by another implementation? +2. Is this functionality already provided by a parent class or different layer? +3. Was this code added for a specific PR that might have been replaced by a later PR? + +**Check git history**: +```bash +# Find when the method was added +git log -p --all -S "TraitCollectionDidChange" -- "path/to/file.cs" + +# Check if a later PR might have replaced this functionality +gh pr list --search "TraitCollectionDidChange" --state merged +``` + +**Decision tree**: +``` +Is functionality provided elsewhere (different layer/class)? + YES → Consider REMOVING this override entirely + NO → Patch the existing implementation +``` + +### 5. Race Conditions and Disposal + +iOS lifecycle methods can be called AFTER disposal begins: + +| Scenario | Timing Issue | +|----------|--------------| +| App exit | TraitCollectionDidChange called after ServiceProvider disposed | +| View dismissed | ViewDidDisappear called after handler disconnected | +| Rotation | TraitCollectionDidChange during orientation change mid-disposal | + +**Safe patterns**: +```csharp +// Pattern 1: Check window handler (best for disposal detection) +var window = handler.MauiContext?.GetPlatformWindow()?.GetWindow(); +if (window?.Handler == null) +{ + // Window is being destroyed, skip + return; +} + +// Pattern 2: Try-catch as safety net +try +{ + var service = handler.GetRequiredService(); + service.DoWork(); +} +catch (ObjectDisposedException) +{ + // Services disposed, skip gracefully +} + +// Pattern 3: Combine both for maximum safety +if (window?.Handler != null) +{ + try + { + // Safe to proceed + } + catch (ObjectDisposedException) { } +} +``` + +### 6. Common iOS Crash Patterns + +| Pattern | Indicator | Solution | +|---------|-----------|----------| +| **ObjectDisposedException** | Accessing services after disposal | Check window.Handler before service access | +| **NullReferenceException in lifecycle** | Accessing properties after disconnect | Null-check all handler/context properties | +| **Double-firing events** | Same event triggered twice | Check for duplicate implementations in layers | +| **Race condition on exit** | Intermittent crash during app close | Add disposal checks to lifecycle methods | + +## Workflow for iOS Bugs + +When debugging an iOS issue: + +1. **Read the stack trace** - Note the method and class where it crashed +2. **Search for ALL occurrences** of that method across the codebase (don't assume it's only in one place) +3. **Check layer architecture** - Is this in the right layer? (Core vs Controls vs Compatibility) +4. **Look for duplicates** - Does this method exist in multiple classes? +5. **Check git history** - When was this added? Was it superseded? +6. **Consider "remove vs patch"** - Should this code exist at all? +7. **If patching, add disposal checks** - Protect against race conditions + +## Examples from Real Bugs + +### Example 1: Issue #33352 - TraitCollectionDidChange Crash + +**Stack trace pointed to**: `ShellSectionRootRenderer.TraitCollectionDidChange` + +**Agent approach**: 11 attempts patching ShellSectionRootRenderer + +**Actual fix**: +- Searched for "TraitCollectionDidChange" across codebase +- Found DUPLICATE in PageViewController (Core layer) +- Recognized Core layer already handles theme changes +- REMOVED ShellSectionRootRenderer override (duplicate code) +- ENHANCED PageViewController with better disposal checks + +**Key lesson**: Search for pattern, don't trust stack trace location alone + +## Best Practices Checklist + +When fixing iOS bugs, verify: +- [ ] Searched for method pattern across entire codebase (`grep -r "MethodName" src/`) +- [ ] Checked Core, Controls, and Compatibility layers +- [ ] Looked for duplicate implementations +- [ ] Checked git history to understand when code was added +- [ ] Considered whether code should be removed vs patched +- [ ] Added disposal checks if method can be called during teardown +- [ ] Verified fix doesn't break other platform implementations + +## Related Documentation + +- `.github/copilot-instructions.md` - General MAUI development guidelines +- `docs/design/HandlerResolution.md` - Handler architecture and resolution +- `.github/architecture/core-vs-controls.md` - Layer responsibility quick reference diff --git a/.github/skills/try-fix/SKILL.md b/.github/skills/try-fix/SKILL.md index eeaf4c9b1ad8..afa534b4acc1 100644 --- a/.github/skills/try-fix/SKILL.md +++ b/.github/skills/try-fix/SKILL.md @@ -125,10 +125,41 @@ Read the target files to understand the code: cat src/path/to/TargetFile.cs ``` +**CRITICAL: Search for Duplicate Implementations** + +Before proceeding with a fix, search for duplicate implementations of the problematic method: + +```bash +# If fixing a method override (e.g., TraitCollectionDidChange) +# Search for ALL occurrences across the codebase +grep -r "override.*MethodName" src/ | grep -v ".Designer.cs" + +# Example: Find all TraitCollectionDidChange implementations +grep -r "TraitCollectionDidChange" src/ | grep -v test + +# Check multiple layers +find src/Core src/Controls -name "*.cs" -exec grep -l "MethodName" {} \; +``` + +**Key insight from Issue #33352**: The bug was NOT in the file where it crashed. It was duplication between: +- `src/Core/src/Platform/iOS/PageViewController.cs` (Core - affects ALL pages) +- `src/Controls/.../ShellSectionRootRenderer.cs` (Controls - Shell-specific) + +The fix was to REMOVE the duplicate in Controls, not patch it. + +**Decision tree**: +``` +Found same method in multiple files? + YES → Check if one is obsolete/duplicate + → Consider REMOVING instead of patching + NO → Proceed with patching the single implementation +``` + **Key questions:** - What is the root cause of this bug? -- Where should the fix go? -- What's the minimal change needed? +- Where should the fix go? (Check for duplicates first!) +- Is this functionality already provided in a different layer? +- What's the minimal change needed? (Remove vs patch?) ### Step 4: Design ONE Fix diff --git a/src/Controls/src/Core/Compatibility/Handlers/Shell/iOS/ShellSectionRootRenderer.cs b/src/Controls/src/Core/Compatibility/Handlers/Shell/iOS/ShellSectionRootRenderer.cs index 6d880ec642a6..d4d0e6c6bb40 100644 --- a/src/Controls/src/Core/Compatibility/Handlers/Shell/iOS/ShellSectionRootRenderer.cs +++ b/src/Controls/src/Core/Compatibility/Handlers/Shell/iOS/ShellSectionRootRenderer.cs @@ -12,6 +12,12 @@ namespace Microsoft.Maui.Controls.Platform.Compatibility { + // CONTROLS LAYER: Shell-specific renderer (Compatibility layer) + // - Handles Shell-specific layout and navigation + // - DO NOT duplicate Core layer functionality here (e.g., theme changes, safe areas) + // - Core functionality is already provided by PageViewController which applies to ALL pages + // - Historical note: TraitCollectionDidChange was removed in PR #33353 (Issue #33352) + // because it duplicated PageViewController functionality and caused race condition crashes public class ShellSectionRootRenderer : UIViewController, IShellSectionRootRenderer, IDisconnectable { #region IShellSectionRootRenderer diff --git a/src/Core/src/Platform/iOS/PageViewController.cs b/src/Core/src/Platform/iOS/PageViewController.cs index 15746c88ed55..e9070087382c 100644 --- a/src/Core/src/Platform/iOS/PageViewController.cs +++ b/src/Core/src/Platform/iOS/PageViewController.cs @@ -3,6 +3,10 @@ namespace Microsoft.Maui.Platform { + // CORE LAYER: Base view controller for ALL pages in .NET MAUI (iOS/MacCatalyst) + // - Provides fundamental lifecycle for ALL pages (Shell, ContentPage, NavigationPage, etc.) + // - Handles platform integration: theme changes, safe areas, status bar + // - DO NOT duplicate functionality from here in Controls layer (see .github/architecture/core-vs-controls.md) public class PageViewController : ContainerViewController { public PageViewController(IView page, IMauiContext mauiContext) @@ -56,6 +60,11 @@ public override UIStatusBarAnimation PreferredStatusBarUpdateAnimation } } + // CORE LAYER: Theme change detection for ALL pages + // This method is called by iOS when the system theme changes (light/dark mode). + // Applies to ALL pages automatically - do not duplicate in Controls layer. + // Historical note: Issue #33352 was caused by duplicate implementation in ShellSectionRootRenderer + // which was removed in PR #33353 (the duplicate caused race conditions during disposal). public override void TraitCollectionDidChange(UITraitCollection? previousTraitCollection) { if (CurrentView?.Handler is ElementHandler handler)