Skip to content

Commit 82bf111

Browse files
CopilotPureWeen
Copilot
and
PureWeen
committed
Add .github folder changes from PRs #33406 and #31487
Co-authored-by: PureWeen <[email protected]>
1 parent 70da9b6 commit 82bf111

5 files changed

Lines changed: 551 additions & 55 deletions

File tree

‎.github/agent-pr-session/pr-31487.md‎

Lines changed: 261 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,261 @@
1+
# PR Review: #31487 - [Android] Fixed duplicate title icon when setting TitleIconImageSource Multiple times
2+
3+
**Date:** 2026-01-08 | **Issue:** [#31445](https://github.com/dotnet/maui/issues/31445) | **PR:** [#31487](https://github.com/dotnet/maui/pull/31487)
4+
5+
## ✅ Final Recommendation: APPROVE
6+
7+
| Phase | Status |
8+
|-------|--------|
9+
| Pre-Flight | ✅ COMPLETE |
10+
| 🧪 Tests | ✅ COMPLETE |
11+
| 🚦 Gate | ✅ PASSED |
12+
| 🔧 Fix | ✅ COMPLETE |
13+
| 📋 Report | ✅ COMPLETE |
14+
15+
---
16+
17+
<details>
18+
<summary><strong>📋 Issue Summary</strong></summary>
19+
20+
On Android, calling `NavigationPage.SetTitleIconImageSource(page, "image.png")` more than once for the same page results in the icon being rendered multiple times in the navigation bar.
21+
22+
**Steps to Reproduce:**
23+
1. Launch app on Android
24+
2. Tap "Set TitleIconImageSource" once: icon appears
25+
3. Tap it again: a second identical icon appears
26+
27+
**Expected:** Single toolbar icon regardless of how many times SetTitleIconImageSource is called.
28+
29+
**Actual:** Each repeated call adds an additional duplicate icon.
30+
31+
**Platforms Affected:**
32+
- [ ] iOS
33+
- [x] Android
34+
- [ ] Windows
35+
- [ ] MacCatalyst
36+
37+
**Version:** 9.0.100 SR10
38+
39+
</details>
40+
41+
<details>
42+
<summary><strong>📁 Files Changed</strong></summary>
43+
44+
| File | Type | Changes |
45+
|------|------|---------|
46+
| `src/Controls/src/Core/Platform/Android/Extensions/ToolbarExtensions.cs` | Fix | +17/-6 |
47+
| `src/Controls/tests/TestCases.HostApp/Issues/Issue31445.cs` | Test | +38 |
48+
| `src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue31445.cs` | Test | +23 |
49+
| `snapshots/android/Issue31445DuplicateTitleIconDoesNotAppear.png` | Snapshot | binary |
50+
| `snapshots/mac/Issue31445DuplicateTitleIconDoesNotAppear.png` | Snapshot | binary |
51+
| `snapshots/windows/Issue31445DuplicateTitleIconDoesNotAppear.png` | Snapshot | binary |
52+
| `snapshots/ios/Issue31445DuplicateTitleIconDoesNotAppear.png` | Snapshot | binary |
53+
54+
</details>
55+
56+
<details>
57+
<summary><strong>💬 PR Discussion Summary</strong></summary>
58+
59+
**Key Comments:**
60+
- Issue verified by LogishaSelvarajSF4525 on MAUI 9.0.0 & 9.0.100
61+
- PR triggered UI tests by jsuarezruiz
62+
- PureWeen requested rebase
63+
64+
**Reviewer Feedback:**
65+
- Copilot review: Suggested testing with different image sources or rapid succession to validate fix better
66+
67+
**Disagreements to Investigate:**
68+
| File:Line | Reviewer Says | Author Says | Status |
69+
|-----------|---------------|-------------|--------|
70+
| Issue31445.cs:31 | Test with different images or rapid calls | N/A | ⚠️ INVESTIGATE |
71+
72+
**Author Uncertainty:**
73+
- None noted
74+
75+
</details>
76+
77+
<details>
78+
<summary><strong>🧪 Tests</strong></summary>
79+
80+
**Status**: ✅ COMPLETE
81+
82+
- [x] PR includes UI tests
83+
- [x] Tests reproduce the issue
84+
- [x] Tests follow naming convention (`Issue31445`)
85+
86+
**Test Files:**
87+
- HostApp: `src/Controls/tests/TestCases.HostApp/Issues/Issue31445.cs`
88+
- NUnit: `src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue31445.cs`
89+
90+
**Test Behavior:**
91+
- Uses snapshot verification (`VerifyScreenshot()`)
92+
- Navigates to test page, taps button to trigger duplicate icon scenario
93+
- Verified to compile successfully
94+
95+
</details>
96+
97+
<details>
98+
<summary><strong>🚦 Gate - Test Verification</strong></summary>
99+
100+
**Status**: ✅ PASSED
101+
102+
- [x] Tests FAIL without fix (bug reproduced - duplicate icons appeared)
103+
- [x] Tests PASS with fix (single icon as expected)
104+
105+
**Result:** PASSED ✅
106+
107+
**Verification Details:**
108+
- Platform: Android (emulator-5554)
109+
- Without fix: Test FAILED (screenshot mismatch - duplicate icons)
110+
- With fix: Test PASSED (single icon verified)
111+
112+
</details>
113+
114+
<details>
115+
<summary><strong>🔧 Fix Candidates</strong></summary>
116+
117+
**Status**: ✅ COMPLETE
118+
119+
| # | Source | Approach | Test Result | Files Changed | Model | Notes |
120+
|---|--------|----------|-------------|---------------|-------|-------|
121+
| 1 | try-fix | Check for existing icon view at position 0, reuse if exists, only create new if needed | ✅ PASS | `ToolbarExtensions.cs` (+7) | Opus 4.5 | Works! Independently arrived at same solution logic as PR |
122+
| 2 | try-fix | Dedupe defensively by scanning all toolbar children, keep first `ToolbarTitleIconImageView`, remove extras; then reuse/create | ✅ PASS | `ToolbarExtensions.cs` (+22/-5) | GPT 5.2 | More robust if child ordering changes or duplicates already exist |
123+
| 3 | try-fix | Use `FindViewWithTag` to uniquely identify/retrieve the MAUI title icon | ✅ PASS | `ToolbarExtensions.cs` (+20/-6) | Gemini 2.0 Flash | Explicit identification; avoids index assumptions and iteration; most robust against external view insertions |
124+
| PR | PR #31487 | Check for existing ToolbarTitleIconImageView before adding new one | ✅ PASS (Gate) | `ToolbarExtensions.cs` (+17/-6) | Author | Original PR - validated by Gate |
125+
126+
**Exhausted:** Yes (3 passing alternatives found)
127+
128+
**Selected Fix:** PR's fix - It’s simplest and sufficient.
129+
- #3 (Tag) is the most "correct" for robustness but adds Tag management overhead.
130+
- #2 (Dedupe) is good for cleanup.
131+
- PR/#1 (Index 0) are standard for this codebase's patterns.
132+
133+
**Comparison Notes:**
134+
- PR/try-fix #1 rely on `GetChildAt(0)` being the title icon view when present
135+
- try-fix #2 is more defensive: it collapses existing duplicates regardless of child index and then reuses/creates as needed
136+
- try-fix #3 uses explicit tagging: precise but introduces new state (Tag) to manage
137+
138+
</details>
139+
140+
---
141+
142+
**Next Step:** Propose Alternative Fix #2 (Dedupe & Scan) to Author for Discussion
143+
144+
---
145+
146+
## 💬 Draft Comment for Author
147+
148+
Hi @PureWeen,
149+
150+
Reviewing the fix in this PR, it works correctly for the reported issue and tests pass.
151+
152+
I explored a couple of alternative approaches and found one that might offer slightly better robustness against edge cases, which I wanted to run by you:
153+
154+
**Alternative: Dedupe & Scan**
155+
Instead of just checking index 0, we could scan all children of the toolbar to find any `ToolbarTitleIconImageView` instances.
156+
157+
```csharp
158+
// Scan all children to find existing title icons
159+
ToolbarTitleIconImageView? titleIcon = null;
160+
for (int i = 0; i < nativeToolbar.ChildCount; i++)
161+
{
162+
var child = nativeToolbar.GetChildAt(i);
163+
if (child is ToolbarTitleIconImageView icon)
164+
{
165+
if (titleIcon == null)
166+
titleIcon = icon; // Keep the first one found
167+
else
168+
nativeToolbar.RemoveView(icon); // Remove any extras (self-healing)
169+
}
170+
}
171+
```
172+
173+
**Why consider this?**
174+
1. **Robustness against Injection:** If another library inserts a view at index 0 (e.g., search bar), the current PR fix (checking only index 0) would fail to see the existing icon and create a duplicate.
175+
2. **Self-Healing:** If the toolbar is already in a bad state (multiple icons from previous bugs), this approach cleans them up.
176+
177+
**Trade-off:**
178+
It involves a loop, so O(N) instead of O(1), but for a toolbar with very few items, this is negligible.
179+
180+
Do you think the added robustness is worth the change, or should we stick to the simpler Index 0 check (current PR) which matches the existing removal logic?
181+
182+
---
183+
184+
## 📋 Final Report
185+
186+
### Summary
187+
188+
PR #31487 correctly fixes the duplicate title icon issue on Android. The fix checks for an existing `ToolbarTitleIconImageView` at position 0 before creating a new one, preventing duplicate icons when `SetTitleIconImageSource` is called multiple times.
189+
190+
### Root Cause
191+
192+
The original `UpdateTitleIcon` method always created a new `ToolbarTitleIconImageView` and added it to position 0, without checking if one already existed. This caused duplicate icons when the method was called repeatedly.
193+
194+
### Validation
195+
196+
| Check | Result |
197+
|-------|--------|
198+
| Tests reproduce bug | ✅ Test fails without fix (duplicate icons) |
199+
| Tests pass with fix | ✅ Test passes with fix (single icon) |
200+
| Independent fix analysis | ✅ try-fix arrived at same solution |
201+
| Code quality | ✅ Clean, minimal change |
202+
203+
### Regression Analysis
204+
205+
<details>
206+
<summary><strong>📜 Git History Analysis</strong></summary>
207+
208+
**Original Implementation:** `e2f3aaa222` (Oct 2021) by Shane Neuville
209+
- Part of "[Android] ToolbarHandler and fixes for various page nesting scenarios (#2781)"
210+
- The bug has existed since the original implementation - it was never designed to handle repeated calls
211+
212+
**Key Finding:** The original code had a check for removing an existing icon when source is null/empty:
213+
```csharp
214+
if (nativeToolbar.GetChildAt(0) is ToolbarTitleIconImageView existingImageView)
215+
nativeToolbar.RemoveView(existingImageView);
216+
```
217+
But this check was **only in the removal path**, not in the creation path. The fix extends this pattern to also check before adding.
218+
219+
**Related Toolbar Issues in This File:**
220+
| Commit | Issue | Description |
221+
|--------|-------|-------------|
222+
| `a93e88c3de` | #7823 | Fix toolbar item icon not removed when navigating |
223+
| `c04b7d79cc` | #19673 | Fixed android toolbar icon change |
224+
| `158ed8b4f1` | #28767 | Removing outdated menu items after activity switch |
225+
226+
**Pattern:** Multiple fixes in this file address issues where Android toolbar state isn't properly cleaned up or reused. This PR follows the same pattern.
227+
228+
</details>
229+
230+
<details>
231+
<summary><strong>🔄 Platform Comparison</strong></summary>
232+
233+
| Platform | TitleIcon Implementation | Duplicate Prevention |
234+
|----------|-------------------------|---------------------|
235+
| **Android** | Creates `ToolbarTitleIconImageView`, adds to position 0 | ❌ Was missing (now fixed by PR) |
236+
| **Windows** | Sets `TitleIconImageSource` property directly | ✅ Property-based, no duplicates possible |
237+
| **iOS** | Uses `NavigationRenderer` with property binding | ✅ Property-based approach |
238+
239+
**Why Android was vulnerable:** Android uses a view-based approach (adding/removing child views) while other platforms use property-based approaches. View management requires explicit duplicate checks.
240+
241+
</details>
242+
243+
<details>
244+
<summary><strong>⚠️ Risk Assessment</strong></summary>
245+
246+
**Regression Risk: LOW**
247+
248+
1. **Minimal change** - Only modifies the creation logic, doesn't change removal
249+
2. **Consistent pattern** - Uses same `GetChildAt(0)` check that already existed for removal
250+
3. **Well-tested** - UI test verifies the specific scenario
251+
4. **No side effects** - Reusing existing view is safe; `SetImageDrawable` handles updates
252+
253+
**Potential Edge Cases (from Copilot review suggestion):**
254+
- Setting different image sources rapidly → Should work fine, image is updated on existing view
255+
- Setting same source multiple times → Explicitly tested, works correctly
256+
257+
</details>
258+
259+
### Recommendation
260+
261+
**✅ APPROVE** - The PR's approach is correct and validated by independent analysis. The fix is minimal, focused, and addresses the root cause.

0 commit comments

Comments
 (0)