Skip to content

Commit 15ecafe

Browse files
[release/9.0.1xx-sr12] [iOS] Fix memory leak in CollectionViewHandler2 ItemsLayout PropertyChanged subscription (#31922)
1 parent b31bd7f commit 15ecafe

File tree

8 files changed

+128
-21
lines changed

8 files changed

+128
-21
lines changed

src/Controls/src/Core/Items/CarouselLayoutTypeConverter.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,10 +19,10 @@ public override object ConvertFrom(ITypeDescriptorContext context, CultureInfo c
1919
var strValue = value?.ToString();
2020

2121
if (strValue == "HorizontalList")
22-
return LinearItemsLayout.CarouselDefault;
22+
return LinearItemsLayout.CreateCarouselHorizontalDefault();
2323

2424
if (strValue == "VerticalList")
25-
return LinearItemsLayout.CarouselVertical;
25+
return LinearItemsLayout.CreateCarouselVerticalDefault();
2626

2727
throw new InvalidOperationException($"Cannot convert \"{strValue}\" into {typeof(LinearItemsLayout)}");
2828
}

src/Controls/src/Core/Items/CarouselView.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -187,7 +187,7 @@ public object PositionChangedCommandParameter
187187
/// <summary>Bindable property for <see cref="ItemsLayout"/>.</summary>
188188
public static readonly BindableProperty ItemsLayoutProperty =
189189
BindableProperty.Create(nameof(ItemsLayout), typeof(LinearItemsLayout), typeof(ItemsView),
190-
LinearItemsLayout.CarouselDefault);
190+
null, defaultValueCreator: (b) => LinearItemsLayout.CreateCarouselHorizontalDefault());
191191

192192
/// <include file="../../../docs/Microsoft.Maui.Controls/CarouselView.xml" path="//Member[@MemberName='ItemsLayout']/Docs/*" />
193193
[System.ComponentModel.TypeConverter(typeof(CarouselLayoutTypeConverter))]

src/Controls/src/Core/Items/ItemsLayoutTypeConverter.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,9 @@ public override object ConvertFrom(ITypeDescriptorContext context, CultureInfo c
2424
int identifierLength = 0;
2525

2626
if (strValue == "VerticalList")
27-
return LinearItemsLayout.Vertical;
27+
return LinearItemsLayout.CreateVerticalDefault();
2828
else if (strValue == "HorizontalList")
29-
return LinearItemsLayout.Horizontal;
29+
return LinearItemsLayout.CreateHorizontalDefault();
3030
else if (strValue.StartsWith("VerticalGrid", StringComparison.Ordinal))
3131
{
3232
orientation = ItemsLayoutOrientation.Vertical;

src/Controls/src/Core/Items/ItemsView.cs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,8 @@ public int RemainingItemsThreshold
113113

114114
internal static readonly BindableProperty InternalItemsLayoutProperty =
115115
BindableProperty.Create(nameof(ItemsLayout), typeof(IItemsLayout), typeof(ItemsView),
116-
LinearItemsLayout.Vertical, propertyChanged: OnInternalItemsLayoutPropertyChanged);
116+
null, propertyChanged: OnInternalItemsLayoutPropertyChanged,
117+
defaultValueCreator: (b) => LinearItemsLayout.CreateVerticalDefault());
117118

118119
static void OnInternalItemsLayoutPropertyChanged(BindableObject bindable, object oldValue, object newValue)
119120
{

src/Controls/src/Core/Items/LinearItemsLayout.cs

Lines changed: 24 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -12,22 +12,14 @@ public LinearItemsLayout([Parameter("Orientation")] ItemsLayoutOrientation orien
1212
}
1313

1414
/// <include file="../../../docs/Microsoft.Maui.Controls/LinearItemsLayout.xml" path="//Member[@MemberName='Vertical']/Docs/*" />
15-
public static readonly IItemsLayout Vertical = new LinearItemsLayout(ItemsLayoutOrientation.Vertical);
15+
public static readonly IItemsLayout Vertical = CreateVerticalDefault();
1616
/// <include file="../../../docs/Microsoft.Maui.Controls/LinearItemsLayout.xml" path="//Member[@MemberName='Horizontal']/Docs/*" />
17-
public static readonly IItemsLayout Horizontal = new LinearItemsLayout(ItemsLayoutOrientation.Horizontal);
17+
public static readonly IItemsLayout Horizontal = CreateHorizontalDefault();
1818

1919
/// <include file="../../../docs/Microsoft.Maui.Controls/LinearItemsLayout.xml" path="//Member[@MemberName='CarouselVertical']/Docs/*" />
20-
public static readonly IItemsLayout CarouselVertical = new LinearItemsLayout(ItemsLayoutOrientation.Vertical)
21-
{
22-
SnapPointsType = SnapPointsType.MandatorySingle,
23-
SnapPointsAlignment = SnapPointsAlignment.Center
24-
};
20+
public static readonly IItemsLayout CarouselVertical = CreateCarouselVerticalDefault();
2521

26-
internal static readonly LinearItemsLayout CarouselDefault = new LinearItemsLayout(ItemsLayoutOrientation.Horizontal)
27-
{
28-
SnapPointsType = SnapPointsType.MandatorySingle,
29-
SnapPointsAlignment = SnapPointsAlignment.Center
30-
};
22+
internal static readonly LinearItemsLayout CarouselDefault = CreateCarouselHorizontalDefault();
3123

3224
/// <summary>Bindable property for <see cref="ItemSpacing"/>.</summary>
3325
public static readonly BindableProperty ItemSpacingProperty =
@@ -40,5 +32,25 @@ public double ItemSpacing
4032
get => (double)GetValue(ItemSpacingProperty);
4133
set => SetValue(ItemSpacingProperty, value);
4234
}
35+
36+
internal static LinearItemsLayout CreateVerticalDefault()
37+
=> new LinearItemsLayout(ItemsLayoutOrientation.Vertical);
38+
39+
internal static LinearItemsLayout CreateHorizontalDefault()
40+
=> new LinearItemsLayout(ItemsLayoutOrientation.Horizontal);
41+
42+
internal static LinearItemsLayout CreateCarouselVerticalDefault()
43+
=> new LinearItemsLayout(ItemsLayoutOrientation.Vertical)
44+
{
45+
SnapPointsType = SnapPointsType.MandatorySingle,
46+
SnapPointsAlignment = SnapPointsAlignment.Center
47+
};
48+
49+
internal static LinearItemsLayout CreateCarouselHorizontalDefault()
50+
=> new LinearItemsLayout(ItemsLayoutOrientation.Horizontal)
51+
{
52+
SnapPointsType = SnapPointsType.MandatorySingle,
53+
SnapPointsAlignment = SnapPointsAlignment.Center
54+
};
4355
}
4456
}

src/Controls/tests/Core.UnitTests/ItemsLayoutTypeConverterTests.cs

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,15 +11,21 @@ public void HorizontalListShouldReturnLinearItemsLayout()
1111
{
1212
var converter = new ItemsLayoutTypeConverter();
1313
var result = converter.ConvertFromInvariantString("HorizontalList");
14-
Assert.Same(LinearItemsLayout.Horizontal, result);
14+
15+
Assert.IsType<LinearItemsLayout>(result);
16+
var linearLayout = (LinearItemsLayout)result;
17+
Assert.Equal(ItemsLayoutOrientation.Horizontal, linearLayout.Orientation);
1518
}
1619

1720
[Fact]
1821
public void VerticalListShouldReturnLinearItemsLayout()
1922
{
2023
var converter = new ItemsLayoutTypeConverter();
2124
var result = converter.ConvertFromInvariantString("VerticalList");
22-
Assert.Same(LinearItemsLayout.Vertical, result);
25+
26+
Assert.IsType<LinearItemsLayout>(result);
27+
var linearLayout = (LinearItemsLayout)result;
28+
Assert.Equal(ItemsLayoutOrientation.Vertical, linearLayout.Orientation);
2329
}
2430

2531
[Fact]
Lines changed: 48 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,54 @@
1-
namespace Microsoft.Maui.DeviceTests
1+
using System;
2+
using System.Collections.Generic;
3+
using System.Threading.Tasks;
4+
using Microsoft.Maui.Controls;
5+
using Microsoft.Maui.Controls.Handlers.Items2;
6+
using Microsoft.Maui.Handlers;
7+
using Xunit;
8+
9+
namespace Microsoft.Maui.DeviceTests
210
{
311
public partial class CarouselViewTests
412
{
13+
[Fact(DisplayName = "CarouselView Does Not Leak With Default ItemsLayout")]
14+
public async Task CarouselViewDoesNotLeakWithDefaultItemsLayout()
15+
{
16+
SetupBuilder();
17+
18+
WeakReference weakCarouselView = null;
19+
WeakReference weakHandler = null;
20+
21+
await InvokeOnMainThreadAsync(async () =>
22+
{
23+
var carouselView = new CarouselView
24+
{
25+
ItemsSource = new List<string> { "Item 1", "Item 2", "Item 3" },
26+
ItemTemplate = new DataTemplate(() => new Label())
27+
// Note: Not setting ItemsLayout - using the default
28+
};
29+
30+
weakCarouselView = new WeakReference(carouselView);
31+
32+
var handler = await CreateHandlerAsync<CarouselViewHandler2>(carouselView);
33+
34+
// Verify handler is created
35+
Assert.NotNull(handler);
36+
37+
// Store weak reference to the handler
38+
weakHandler = new WeakReference(handler);
39+
40+
// Disconnect the handler
41+
((IElementHandler)handler).DisconnectHandler();
42+
});
43+
44+
// Force garbage collection
45+
await AssertionExtensions.WaitForGC(weakCarouselView, weakHandler);
546

47+
// Verify the CarouselView was collected
48+
Assert.False(weakCarouselView.IsAlive, "CarouselView should have been garbage collected");
49+
50+
// Verify the handler was collected
51+
Assert.False(weakHandler.IsAlive, "CarouselViewHandler2 should have been garbage collected");
52+
}
653
}
754
}

src/Controls/tests/DeviceTests/Elements/CollectionView/CollectionViewTests.iOS.cs

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -251,6 +251,47 @@ public void IndexPathValidTest()
251251
Assert.False(source.IsIndexPathValid(invalidSection));
252252
}
253253

254+
[Fact(DisplayName = "CollectionView Does Not Leak With Default ItemsLayout")]
255+
public async Task CollectionViewDoesNotLeakWithDefaultItemsLayout()
256+
{
257+
SetupBuilder();
258+
259+
WeakReference weakCollectionView = null;
260+
WeakReference weakHandler = null;
261+
262+
await InvokeOnMainThreadAsync(async () =>
263+
{
264+
var collectionView = new CollectionView
265+
{
266+
ItemsSource = new List<string> { "Item 1", "Item 2", "Item 3" },
267+
ItemTemplate = new DataTemplate(() => new Label())
268+
// Note: Not setting ItemsLayout - using the default
269+
};
270+
271+
weakCollectionView = new WeakReference(collectionView);
272+
273+
var handler = await CreateHandlerAsync<CollectionViewHandler2>(collectionView);
274+
275+
// Verify handler is created
276+
Assert.NotNull(handler);
277+
278+
// Store weak reference to the handler
279+
weakHandler = new WeakReference(handler);
280+
281+
// Disconnect the handler
282+
((IElementHandler)handler).DisconnectHandler();
283+
});
284+
285+
// Force garbage collection
286+
await AssertionExtensions.WaitForGC(weakCollectionView, weakHandler);
287+
288+
// Verify the CollectionView was collected
289+
Assert.False(weakCollectionView.IsAlive, "CollectionView should have been garbage collected");
290+
291+
// Verify the handler was collected
292+
Assert.False(weakHandler.IsAlive, "CollectionViewHandler2 should have been garbage collected");
293+
}
294+
254295
/// <summary>
255296
/// Simulates what a developer might do with a Page/View
256297
/// </summary>

0 commit comments

Comments
 (0)