-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[Android] Fixed duplicate title icon when setting TitleIconImageSource Multiple times #31487
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request fixes a bug where duplicate title icons would appear in Android navigation bars when the TitleIconImageSource property was set multiple times. The issue was caused by the UpdateTitleIcon method always creating a new ToolbarTitleIconImageView without checking for existing ones.
Key changes:
- Modified the UpdateTitleIcon method to check for existing ToolbarTitleIconImageView before creating new ones
- Added proper reuse logic to prevent duplicate icons
- Included test cases to verify the fix
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Controls/src/Core/Platform/Android/Extensions/ToolbarExtensions.cs | Updated UpdateTitleIcon method to check for existing icon views and reuse them instead of always creating new ones |
| src/Controls/tests/TestCases.HostApp/Issues/Issue31445.cs | Added test page that reproduces the duplicate icon issue by setting TitleIconImageSource multiple times |
| src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue31445.cs | Added UI test that verifies the fix by checking that only one title icon appears after button interaction |
| VerticalOptions = LayoutOptions.Start, | ||
| AutomationId = "Issue31445Button" | ||
| }; | ||
| button.Clicked += (s, e) => { NavigationPage.SetTitleIconImageSource(this, "dotnet_bot.png"); }; |
Copilot
AI
Sep 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test only sets the same image source again. To better validate the fix, consider testing with different image sources or setting the same source multiple times in rapid succession to ensure the duplicate prevention works in various scenarios.
|
/azp run MAUI-UITests-public |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run MAUI-UITests-public |
|
Azure Pipelines successfully started running 1 pipeline(s). |
b7eb486 to
ffb45bb
Compare
|
/rebase |
ffb45bb to
01ee772
Compare
PureWeen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Review Complete
The fix works correctly for the reported issue - tests pass and the duplicate icon bug is resolved.
However, I explored an alternative approach that might offer better robustness and wanted to get your thoughts:
Alternative: Dedupe & Scan
Instead of checking only index 0, we could scan all toolbar children to find any ToolbarTitleIconImageView instances:
ToolbarTitleIconImageView? titleIcon = null;
for (int i = 0; i < nativeToolbar.ChildCount; i++)
{
var child = nativeToolbar.GetChildAt(i);
if (child is ToolbarTitleIconImageView icon)
{
if (titleIcon == null)
titleIcon = icon; // Keep the first one found
else
nativeToolbar.RemoveView(icon); // Remove any extras (self-healing)
}
}Why consider this?
- Robustness against view injection - If another library inserts a view at index 0, the current fix would miss the existing icon and create a duplicate
- Self-healing - Cleans up any pre-existing duplicates from previous bugs
Trade-off: O(N) loop instead of O(1), but toolbar child count is typically very small.
What do you think - is the added robustness worth the change, or should we stick with the simpler index 0 check?
Co-authored-by: PureWeen <[email protected]>
Improves agent/skill documentation for automated PR reviews and UI test
authoring.
### Agent Workflow Documentation (Updated)
**`.github/agents/pr.md`**
- Clarify state file commit rule: state file should always be included
when committing changes
**`.github/instructions/uitests.instructions.md`**
- Add rule prohibiting inline `#if` platform directives in test methods
- Require platform-specific logic to be in extension methods for better
readability:
```csharp
// ❌ Hard to read
[Test]
public void MyTest()
{
#if ANDROID
App.TapCoordinates(100, 200);
#else
App.Tap("MyElement");
#endif
}
// ✅ Clean and readable
[Test]
public void MyTest()
{
App.TapElementCrossPlatform("MyElement");
}
```
**`.github/skills/try-fix/SKILL.md`**
- Add "Model" column to Fix Candidates table format to track which AI
model generated each fix attempt
### Summary
These changes provide clearer guidelines for automated PR reviews and UI
test authoring.
<!-- START COPILOT CODING AGENT SUFFIX -->
<!-- START COPILOT ORIGINAL PROMPT -->
<details>
<summary>Original prompt</summary>
> Can you create a PR with the changes from the ".github" folder on
#33406 and
#31487
</details>
<!-- START COPILOT CODING AGENT TIPS -->
---
✨ Let Copilot coding agent [set things up for
you](https://github.com/dotnet/maui/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot)
— coding agent works faster and does higher quality work when set up for
your repo.
---------
Co-authored-by: copilot-swe-agent[bot] <[email protected]>
Co-authored-by: PureWeen <[email protected]>
Co-authored-by: Shane Neuville <[email protected]>
|
@PureWeen Yes it is a valid suggestion and I have committed the changes. |
🤖 AI Summary📊 Expand Full Review🔍 Pre-Flight — Context & Validation📝 Review Session — Commited the suggested changes ·
|
| # | Source | Approach | Test Result | Files Changed | Model | Notes |
|---|---|---|---|---|---|---|
| 1 | try-fix | Check for existing icon view at position 0, reuse if exists | ✅ PASS | ToolbarExtensions.cs (+7) |
Opus 4.5 | Works! |
| 2 | try-fix | Dedupe defensively by scanning all toolbar children | ✅ PASS | ToolbarExtensions.cs (+22/-5) |
GPT 5.2 | More robust - ADOPTED BY PR |
| 3 | try-fix | Use FindViewWithTag to identify the MAUI title icon |
✅ PASS | ToolbarExtensions.cs (+20/-6) |
Gemini 2.0 Flash | Most robust but adds Tag overhead |
| PR | PR #31487 | Scan all children, keep first, remove extras (self-healing) | ✅ PASS (Gate) | ToolbarExtensions.cs (+27/-6) |
Author | Now uses Alternative #2 approach |
Exhausted: Yes (3 passing alternatives found in prior session)
Selected Fix: PR's fix (updated to use the dedupe & scan approach from try-fix #2)
- Scans all children of toolbar to find existing
ToolbarTitleIconImageView - Keeps first one found, removes any extras (self-healing)
- Creates new only if none exists
- More robust than just checking index 0
📋 Report — Final Recommendation
📝 Review Session — Commited the suggested changes · 1f9137f
Status: ✅ COMPLETE
Final Recommendation: APPROVE
Root Cause Analysis
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.
The bug has existed since the original implementation in Oct 2021 (e2f3aaa222). The original code had a check for removing an existing icon when source is null/empty, but this check was only in the removal path, not in the creation path.
Key Findings
- PR adopted the more robust approach - Instead of just checking index 0, the PR now scans all children
- Self-healing behavior - If duplicates already exist, they get cleaned up
- Robust against external view insertions - Works even if another library inserts views
- Prior review completed - Full agent review embedded in PR (
pr-31487.md)
Fix Verified via Cherry-Pick
Applied fix locally (without branch switch) and verified:
- ✅
Controls.Core.csprojbuilds for Android - ✅
TestCases.HostAppbuilds for Android - ✅
TestCases.Shared.Testsbuilds
Verdict
✅ APPROVE - The PR correctly fixes the duplicate title icon issue on Android. The fix is well-tested, follows codebase patterns, and includes self-healing behavior for edge cases.
Notes
- PR needs rebase (PureWeen requested)
- Milestoned for .NET 10 SR5
…e Multiple times (#31487) <!-- Please let the below note in for people that find this PR --> > [!NOTE] > Are you waiting for the changes in this PR to be merged? > It would be very helpful if you could [test the resulting artifacts](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! <!-- !!!!!!! MAIN IS THE ONLY ACTIVE BRANCH. MAKE SURE THIS PR IS TARGETING MAIN. !!!!!!! --> ### RootCause: UpdateTitleIcon always added a new ToolbarTitleIconImageView without checking for an existing one, causing duplicate icons when TitleIconImageSource was set multiple times. ### Description of Change <!-- Enter description of the fix in this section --> Modified UpdateTitleIcon method to check for an existing ToolbarTitleIconImageView and only add a new one if necessary, ensuring that duplicate icons are not created when the title icon is set multiple times. ### Issues Fixed <!-- Please make sure that there is a bug logged for the issue being fixed. The bug should describe the problem and how to reproduce it. --> Fixes #31445 ### Tested the behavior in the following platforms - [x] Windows - [x] Android - [x] iOS - [x] Mac ### Screenshot | Before Issue Fix | After Issue Fix | |----------|----------| | <video src="https://github.com/user-attachments/assets/3aa9a70b-8db9-4e41-bd65-6790190c0fd1"> | <video src="https://github.com/user-attachments/assets/872687d1-de55-40df-8a7b-591be9c70b09"> | <!-- Are you targeting main? All PRs should target the main branch unless otherwise noted. --> --------- Co-authored-by: Shane Neuville <[email protected]>
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
RootCause:
UpdateTitleIcon always added a new ToolbarTitleIconImageView without checking for an existing one, causing duplicate icons when TitleIconImageSource was set multiple times.
Description of Change
Modified UpdateTitleIcon method to check for an existing ToolbarTitleIconImageView and only add a new one if necessary, ensuring that duplicate icons are not created when the title icon is set multiple times.
Issues Fixed
Fixes #31445
Tested the behavior in the following platforms
Screenshot
BeforeFix31445.mov
AfterFix31445.mov