-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Fix memory leak in CollectionView and CarouselView by using instance-based ItemsLayout defaults #31918
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
Fix memory leak in CollectionView and CarouselView by using instance-based ItemsLayout defaults #31918
Changes from all commits
35fc593
38d3dac
c215e4e
7300933
46ecaab
62b5228
1fb05e8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,54 @@ | ||
| namespace Microsoft.Maui.DeviceTests | ||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Threading.Tasks; | ||
| using Microsoft.Maui.Controls; | ||
| using Microsoft.Maui.Controls.Handlers.Items2; | ||
| using Microsoft.Maui.Handlers; | ||
| using Xunit; | ||
|
|
||
| namespace Microsoft.Maui.DeviceTests | ||
| { | ||
| public partial class CarouselViewTests | ||
| { | ||
| [Fact(DisplayName = "CarouselView Does Not Leak With Default ItemsLayout")] | ||
| public async Task CarouselViewDoesNotLeakWithDefaultItemsLayout() | ||
| { | ||
| SetupBuilder(); | ||
|
|
||
| WeakReference weakCarouselView = null; | ||
| WeakReference weakHandler = null; | ||
|
|
||
| await InvokeOnMainThreadAsync(async () => | ||
| { | ||
| var carouselView = new CarouselView | ||
| { | ||
| ItemsSource = new List<string> { "Item 1", "Item 2", "Item 3" }, | ||
| ItemTemplate = new DataTemplate(() => new Label()) | ||
| // Note: Not setting ItemsLayout - using the default | ||
| }; | ||
|
|
||
| weakCarouselView = new WeakReference(carouselView); | ||
|
|
||
| var handler = await CreateHandlerAsync<CarouselViewHandler2>(carouselView); | ||
|
|
||
| // Verify handler is created | ||
| Assert.NotNull(handler); | ||
|
|
||
| // Store weak reference to the handler | ||
| weakHandler = new WeakReference(handler); | ||
|
|
||
| // Disconnect the handler | ||
| ((IElementHandler)handler).DisconnectHandler(); | ||
| }); | ||
|
|
||
| // Force garbage collection | ||
| await AssertionExtensions.WaitForGC(weakCarouselView, weakHandler); | ||
|
|
||
| // Verify the CarouselView was collected | ||
| Assert.False(weakCarouselView.IsAlive, "CarouselView should have been garbage collected"); | ||
|
|
||
| // Verify the handler was collected | ||
| Assert.False(weakHandler.IsAlive, "CarouselViewHandler2 should have been garbage collected"); | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -251,6 +251,47 @@ public void IndexPathValidTest() | |
| Assert.False(source.IsIndexPathValid(invalidSection)); | ||
| } | ||
|
|
||
| [Fact(DisplayName = "CollectionView Does Not Leak With Default ItemsLayout")] | ||
| public async Task CollectionViewDoesNotLeakWithDefaultItemsLayout() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While CollectionView has this GC test, CarouselView lacks a similar explicit test, could create a similar one for CarouselView?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well the way it handles layout is the same no ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added |
||
| { | ||
| SetupBuilder(); | ||
|
|
||
| WeakReference weakCollectionView = null; | ||
| WeakReference weakHandler = null; | ||
|
|
||
| await InvokeOnMainThreadAsync(async () => | ||
| { | ||
| var collectionView = new CollectionView | ||
| { | ||
| ItemsSource = new List<string> { "Item 1", "Item 2", "Item 3" }, | ||
| ItemTemplate = new DataTemplate(() => new Label()) | ||
| // Note: Not setting ItemsLayout - using the default | ||
| }; | ||
|
|
||
| weakCollectionView = new WeakReference(collectionView); | ||
|
|
||
| var handler = await CreateHandlerAsync<CollectionViewHandler2>(collectionView); | ||
|
|
||
| // Verify handler is created | ||
| Assert.NotNull(handler); | ||
|
|
||
| // Store weak reference to the handler | ||
| weakHandler = new WeakReference(handler); | ||
|
|
||
| // Disconnect the handler | ||
| ((IElementHandler)handler).DisconnectHandler(); | ||
| }); | ||
|
|
||
| // Force garbage collection | ||
| await AssertionExtensions.WaitForGC(weakCollectionView, weakHandler); | ||
|
|
||
| // Verify the CollectionView was collected | ||
| Assert.False(weakCollectionView.IsAlive, "CollectionView should have been garbage collected"); | ||
|
|
||
| // Verify the handler was collected | ||
| Assert.False(weakHandler.IsAlive, "CollectionViewHandler2 should have been garbage collected"); | ||
| } | ||
PureWeen marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| /// <summary> | ||
| /// Simulates what a developer might do with a Page/View | ||
| /// </summary> | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.