Skip to content
This repository has been archived by the owner on May 1, 2024. It is now read-only.

Commit

Permalink
[iOS] Fixed crash updating ToolbarItem icon (already disposed) (#14749)
Browse files Browse the repository at this point in the history 
* Add repro sample

* Fixed the issue

* Fix build error

Co-authored-by: Gerald Versluis <[email protected]>
  • Loading branch information
@jsuarezruiz @jfversluis
jsuarezruiz and jfversluis authored Oct 19, 2021
1 parent d7f8e42 commit 1b0d8f2
Show file tree
Hide file tree
Showing 4 changed files with 106 additions and 9 deletions.
15 changes: 15 additions & 0 deletions Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue6387.xaml
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<?xml version="1.0" encoding="utf-8" ?>
<controls:TestContentPage
xmlns:controls="clr-namespace:Xamarin.Forms.Controls"
xmlns="http://xamarin.com/schemas/2014/forms"
xmlns:x="http://schemas.microsoft.com/winfx/2009/xaml"
x:Class="Xamarin.Forms.Controls.Issues.Issue6387"
Title="Issue 6387">
<ContentPage.Content>
<StackLayout>
<Label
Padding="12"
Text="Tap the ToolBarItem with an Icon. Without exceptions, the test has passed." />
</StackLayout>
</ContentPage.Content>
</controls:TestContentPage>
61 changes: 61 additions & 0 deletions Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue6387.xaml.cs
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
using System;
using System.Collections.Generic;
using System.Diagnostics;
using Xamarin.Forms.CustomAttributes;
using Xamarin.Forms.Internals;
using System.Reflection;

#if UITEST
using Xamarin.UITest;
using NUnit.Framework;
using Xamarin.Forms.Core.UITests;
#endif

namespace Xamarin.Forms.Controls.Issues
{
#if UITEST
[Category(UITestCategories.ToolbarItem)]
#endif
[Preserve(AllMembers = true)]
[Issue(IssueTracker.Github, 6387,
"[Bug] [iOS] Crash When the First Toolbar Item Has an Icon and a Second Item Gets Added",
PlatformAffected.iOS)]
public partial class Issue6387 : TestContentPage
{
#if APP
readonly ToolbarItem _item0;
readonly ToolbarItem _item1;
#endif
public Issue6387()
{
#if APP
InitializeComponent();

_item0 = new ToolbarItem("Item 0", null, ClearAndAddToolbarItems)
{
IconImageSource = ImageSource.FromResource("Xamarin.Forms.Controls.GalleryPages.crimson.jpg", typeof(Issue6387).GetTypeInfo().Assembly)
};
_item1 = new ToolbarItem("Item 1", null, ClearAndAddToolbarItems)
{
// It doesn't matter if this item has an image or not.
};

ClearAndAddToolbarItems();
#endif
}

protected override void Init()
{

}
#if APP
void ClearAndAddToolbarItems()
{
ToolbarItems.Clear();

ToolbarItems.Add(_item0);
ToolbarItems.Add(_item1);
}
#endif
}
}
6 changes: 5 additions & 1 deletion ...ssues/Xamarin.Forms.Controls.Issues.Shared/Xamarin.Forms.Controls.Issues.Shared.projitems
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1801,6 +1801,7 @@
<Compile Include="$(MSBuildThisFileDirectory)Issue13577.xaml.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Issue14505.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Issue14505-II.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Issue6387.xaml.cs" />
</ItemGroup>
<ItemGroup>
<EmbeddedResource Include="$(MSBuildThisFileDirectory)Bugzilla22229.xaml">
Expand Down Expand Up @@ -2290,6 +2291,9 @@
<EmbeddedResource Include="$(MSBuildThisFileDirectory)Issue13577.xaml">
<Generator>MSBuild:UpdateDesignTimeXaml</Generator>
</EmbeddedResource>
<EmbeddedResource Include="$(MSBuildThisFileDirectory)Issue6387.xaml">
<Generator>MSBuild:UpdateDesignTimeXaml</Generator>
</EmbeddedResource>
</ItemGroup>
<ItemGroup>
<EmbeddedResource Include="$(MSBuildThisFileDirectory)Bugzilla27417Xaml.xaml">
Expand Down Expand Up @@ -2810,7 +2814,7 @@
<ItemGroup>
<EmbeddedResource Include="$(MSBuildThisFileDirectory)Issue13037.xaml">
<SubType>Designer</SubType>
<Generator>MSBuild:Compile</Generator>
<Generator>MSBuild:UpdateDesignTimeXaml</Generator>
</EmbeddedResource>
<EmbeddedResource Include="$(MSBuildThisFileDirectory)Issue2447.xaml">
<SubType>Designer</SubType>
Expand Down
33 changes: 25 additions & 8 deletions Xamarin.Forms.Platform.iOS/Extensions/ToolbarItemExtensions.cs
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System.ComponentModel;
using System.Threading.Tasks;
using CoreGraphics;
using UIKit;
using PointF = CoreGraphics.CGPoint;
Expand All @@ -23,6 +24,7 @@ public static UIBarButtonItem ToUIBarButtonItem(this ToolbarItem item, bool forc

sealed class PrimaryToolbarItem : UIBarButtonItem
{
bool _disposed;
readonly bool _forceName;
readonly ToolbarItem _item;

Expand All @@ -32,7 +34,7 @@ public PrimaryToolbarItem(ToolbarItem item, bool forceName)
_item = item;

if (item.IconImageSource != null && !item.IconImageSource.IsEmpty && !forceName)
UpdateIconAndStyle();
Device.BeginInvokeOnMainThread(async () => { await UpdateIconAndStyleAsync(); });
else
UpdateTextAndStyle();
UpdateIsEnabled();
Expand All @@ -50,11 +52,15 @@ public PrimaryToolbarItem(ToolbarItem item, bool forceName)
protected override void Dispose(bool disposing)
{
if (disposing)
{
_item.PropertyChanged -= OnPropertyChanged;
_disposed = true;
}

base.Dispose(disposing);
}

void OnPropertyChanged(object sender, PropertyChangedEventArgs e)
async void OnPropertyChanged(object sender, PropertyChangedEventArgs e)
{
if (e.PropertyName == MenuItem.IsEnabledProperty.PropertyName)
UpdateIsEnabled();
Expand All @@ -68,7 +74,7 @@ void OnPropertyChanged(object sender, PropertyChangedEventArgs e)
if (!_forceName)
{
if (_item.IconImageSource != null && !_item.IconImageSource.IsEmpty)
UpdateIconAndStyle();
await UpdateIconAndStyleAsync();
else
UpdateTextAndStyle();
}
Expand All @@ -79,8 +85,11 @@ void OnPropertyChanged(object sender, PropertyChangedEventArgs e)
this.SetAccessibilityLabel(_item);
}

async void UpdateIconAndStyle()
async Task UpdateIconAndStyleAsync()
{
if (_disposed)
return;

Image = await _item.IconImageSource.GetNativeImageAsync();
Style = UIBarButtonItemStyle.Plain;
}
Expand All @@ -100,13 +109,14 @@ void UpdateTextAndStyle()

sealed class SecondaryToolbarItem : UIBarButtonItem
{
bool _disposed;
readonly ToolbarItem _item;

public SecondaryToolbarItem(ToolbarItem item) : base(new SecondaryToolbarItemContent())
{
_item = item;
UpdateText();
UpdateIcon();
Device.BeginInvokeOnMainThread(async () => { await UpdateIconAsync(); });
UpdateIsEnabled();

((SecondaryToolbarItemContent)CustomView).TouchUpInside += (sender, e) => ((IMenuItemController)_item).Activate();
Expand All @@ -122,16 +132,20 @@ public SecondaryToolbarItem(ToolbarItem item) : base(new SecondaryToolbarItemCon
protected override void Dispose(bool disposing)
{
if (disposing)
{
_item.PropertyChanged -= OnPropertyChanged;
_disposed = true;
}

base.Dispose(disposing);
}

void OnPropertyChanged(object sender, PropertyChangedEventArgs e)
async void OnPropertyChanged(object sender, PropertyChangedEventArgs e)
{
if (e.PropertyName == MenuItem.TextProperty.PropertyName)
UpdateText();
else if (e.PropertyName == MenuItem.IconImageSourceProperty.PropertyName)
UpdateIcon();
await UpdateIconAsync();
else if (e.PropertyName == MenuItem.IsEnabledProperty.PropertyName)
UpdateIsEnabled();
else if (e.PropertyName == AutomationProperties.HelpTextProperty.PropertyName)
Expand All @@ -140,8 +154,11 @@ void OnPropertyChanged(object sender, PropertyChangedEventArgs e)
this.SetAccessibilityLabel(_item);
}

async void UpdateIcon()
async Task UpdateIconAsync()
{
if (_disposed)
return;

UIImage image = null;
if (_item.IconImageSource != null && !_item.IconImageSource.IsEmpty)
{
Expand Down

0 comments on commit 1b0d8f2

Please sign in to comment.