|
71 | 71 | - Cons: Slightly more code, relies on internal property (same assembly) |
72 | 72 |
|
73 | 73 | User's suggestion of checking window destroying state was spot-on! |
74 | | -**Selected Fix:** [PENDING] |
| 74 | + |
| 75 | +--- |
| 76 | + |
| 77 | +## ACTUAL IMPLEMENTED FIX |
| 78 | + |
| 79 | +**Selected Fix:** Architectural improvement - Remove duplication + strengthen Core layer |
| 80 | + |
| 81 | +**What was implemented:** |
| 82 | + |
| 83 | +1. **REMOVED** `TraitCollectionDidChange` override from `ShellSectionRootRenderer` (Controls layer) |
| 84 | + - Lines 144-151 deleted |
| 85 | + - This was duplicate code that didn't belong in Controls |
| 86 | + |
| 87 | +2. **ENHANCED** `TraitCollectionDidChange` in `PageViewController` (Core layer) |
| 88 | + - Added `window?.Handler == null` check (like attempt #8) |
| 89 | + - Added try-catch safety net (like attempt #4) |
| 90 | + - Uses `GetRequiredService` instead of `FindMauiContext` |
| 91 | + - Combined approach: Window.Handler check + try-catch for race conditions |
| 92 | + |
| 93 | +**Why this wasn't discovered by try-fix:** |
| 94 | + |
| 95 | +1. **Tunnel vision** - Only looked at ShellSectionRootRenderer (where error appeared) |
| 96 | +2. **Didn't search codebase** - Never found PageViewController also had TraitCollectionDidChange |
| 97 | +3. **Didn't recognize duplication** - Both Core and Controls had the override |
| 98 | +4. **Missed layer architecture** - Theme changes are CORE functionality, not Shell-specific |
| 99 | + |
| 100 | +**Key insight:** |
| 101 | + |
| 102 | +The bug existed because theme handling was **duplicated** across layers: |
| 103 | +- Core (PageViewController) - Fundamental, applies to ALL pages |
| 104 | +- Controls (ShellSectionRootRenderer) - Shell-specific override |
| 105 | + |
| 106 | +The proper fix was to **remove the Controls override** and **strengthen the Core implementation**, not patch the Controls one. |
| 107 | + |
| 108 | +**Files changed:** |
| 109 | +- `src/Controls/src/Core/Compatibility/Handlers/Shell/iOS/ShellSectionRootRenderer.cs` (-10 lines) |
| 110 | +- `src/Core/src/Platform/iOS/PageViewController.cs` (+29 lines) |
| 111 | +- PublicAPI.Unshipped.txt (iOS/MacCatalyst) - document removal |
| 112 | + |
| 113 | +**Test verification:** |
| 114 | +✅ TraitCollectionDidChangeAfterDisposeDoesNotCrash passes with new implementation |
75 | 115 |
|
76 | 116 | ## Test Command |
77 | 117 |
|
78 | 118 | ```bash |
79 | 119 | pwsh .github/scripts/BuildAndRunHostApp.ps1 -Platform catalyst -TestFilter "FullyQualifiedName~TraitCollectionDidChangeAfterDisposeDoesNotCrash" |
80 | 120 | ``` |
| 121 | + |
| 122 | +## Lessons Learned |
| 123 | + |
| 124 | +**What would have helped discover this fix:** |
| 125 | + |
| 126 | +1. **Codebase-wide search** - `grep -r "TraitCollectionDidChange" src/` would have found both locations |
| 127 | +2. **Layer analysis** - Ask "Does this belong in Core or Controls?" |
| 128 | +3. **Duplication detection** - Recognize when the same override exists in multiple layers |
| 129 | +4. **Remove vs patch** - Consider whether code should exist at all, not just how to fix it |
| 130 | + |
| 131 | +**Repository improvements needed:** |
| 132 | + |
| 133 | +1. Architecture documentation explaining Core vs Controls layer responsibility |
| 134 | +2. Try-fix skill enhancement to search for duplicate implementations |
| 135 | +3. Inline comments in key classes about layer responsibilities |
| 136 | +4. Linting rule to detect duplicate iOS/Android method overrides across layers |
0 commit comments