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.

Sign up for GitHub

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

Jump to bottom

[iOS] Fix setting the CurrentItem on CarouselView load #22279

Open

rmarinho wants to merge 19 commits into main from fix-18879

Member

rmarinho commented May 7, 2024 •

edited

Description of Change

Since the mappers run when the handler is started and in different order than forms, seems the initial position based on CurrentItem wasn't being set correctly since the mapper for the Position property runs first and overrides it.

We also fix an issue when rotating was not keeping the correct position, since the scroll event was triggered and we want to avoid updating the position till the orientation finishes. This changed the Scrolled event to be fired when the scroll animation finishes.

Added a way to know better when we the CollectionViewController is attached or detached from the view hierarchy. So now we can proper call the TearDown and Setup.

Fixes some warnings

Issues Fixed

Fixes #18879

Copilot summary

This pull request includes changes to the CarouselViewHandler.iOS.cs and CarouselViewController.cs files in the src/Controls/src/Core/Handlers/Items/ directory. The changes primarily involve refactoring the code, adding new methods, and improving the existing logic.

Here are the most significant changes:

Changes in CarouselViewHandler.iOS.cs:

Imported Foundation and UIKit namespaces.
Modified the ScrollToRequested method to include a new NSIndexPath instance and added a new scrollToItemAction method.
Updated the MapPosition method to handle situations where the initial position hasn't been set.
Added a new AnimationDuration property in the MapLoop method.

Changes in CarouselViewController.cs:

Imported the Microsoft.Maui.Devices namespace.
Refactored the CarouselViewController class to include new fields and methods, and modified the constructor.
Updated the GetCell method to include new conditions for DefaultCell and TemplatedCell. [1] [2]
Modified the ViewDidLayoutSubviews method to use _isCenteringItem instead of _updatingScrollOffset.
Refactored the UpdateItemsSource method to use InitialPositionSet instead of _initialPositionSet.
Updated the AttachingToWindow and DetachingFromWindow methods, and added new methods InitialPositionSet, TearDown, and Setup.
Modified the CarouselViewScrolled method to include new conditions.
Refactored the CollectionViewUpdating and CollectionViewUpdated methods to include new conditions.
Updated the UpdateLoop and UpdateFromCurrentItem methods to include new conditions. [1] [2]
Modified the UpdateFromPosition method to include new conditions and logic.
Updated the UpdateInitialPosition and UpdateVisualStates methods to include new conditions and logic. [1] [2]
Refactored the CarouselViewLoopManager class to include new conditions. [1] [2] [3]

rmarinho requested a review from a team as a code owner

May 7, 2024 18:46

rmarinho requested review from jsuarezruiz, tj-devel709 and PureWeen

May 7, 2024 18:46

PureWeen reviewed

View reviewed changes

src/Controls/src/Core/Handlers/Items/iOS/CarouselViewController.cs Outdated Show resolved Hide resolved

Eilon added the area-controls-collectionview label

rmarinho requested a review from PureWeen

May 8, 2024 22:50

jsuarezruiz previously approved these changes

View reviewed changes

jsuarezruiz reviewed

View reviewed changes

src/Controls/tests/UITests/Tests/CarouselViewUITests.cs Outdated

		@@ -23,9 +23,9 @@ protected override void FixtureSetup()
		[Category(UITestCategories.CarouselView)]
		public async Task CarouselViewSetPosition()

Contributor

jsuarezruiz May 9, 2024

This one is failing on iOS

jsuarezruiz self-requested a review

May 9, 2024 11:59

rmarinho dismissed jsuarezruiz’s stale review via

6a67bb7

May 21, 2024 10:39

rmarinho force-pushed the fix-18879 branch from e9571b8 to 6a67bb7 Compare

May 21, 2024 10:39

rmarinho commented

View reviewed changes

src/Controls/src/Core/Handlers/Items/iOS/CarouselViewController.cs

               		protected readonly CarouselView Carousel;
               		CarouselViewLoopManager _carouselViewLoopManager;
-              		bool _initialPositionSet;
-              		bool _updatingScrollOffset;

Member Author

rmarinho May 22, 2024

I just renamed this one to be more clear

rmarinho commented

View reviewed changes

src/Controls/src/Core/Handlers/Items/iOS/CarouselViewController.cs

               				var correctedIndexPath = NSIndexPath.FromRowSection(cellAndCorrectedIndex.correctedIndex, 0);
               				if (cell is DefaultCell defaultCell)
+              				{

Member Author

rmarinho May 22, 2024

Fixing warnings and added {

PureWeen reviewed

View reviewed changes

src/Controls/src/Core/Handlers/Items/iOS/CarouselViewController.cs Outdated Show resolved Hide resolved

rmarinho requested a review from PureWeen

May 23, 2024 16:44

PureWeen reviewed

View reviewed changes

src/Controls/src/Core/Handlers/Items/iOS/ItemsViewController.cs Outdated

@@ @@ -249,6 +262,15 @@ void InvalidateMeasureIfContentSizeChanged() @@
               			_previousContentSize = contentSize.Value;
               		}
+              		internal virtual void AttachingToWindow()

Member

PureWeen May 23, 2024

can we make this private protected instead?

PureWeen reviewed

View reviewed changes

src/Controls/src/Core/Handlers/Items/iOS/CarouselViewController.cs Outdated

+              		internal override void DettachingFromWindow()
+              		{
+              			base.DettachingFromWindow();
+              			TearDown();

Member

PureWeen May 23, 2024

I don't think we can just outright call TearDown here

Or we need to move some of the cod that's in the ctor into AttachingToWindow

for example this is unsubscrived from

carousel.Scrolled -= CarouselViewScrolled;

Inside of TearDown, but subscribed to in the ctor.

Just because something is detaching from the window, doesn't mean it won't get re-attached.

For example, when a modal is pushed on top of the current screen. The VC below that modal will get unattached.

Member Author

rmarinho May 23, 2024

Yeah , you right, got eager that actually never gets called, let me move the subscribe to the correct place to.

PureWeen reviewed

View reviewed changes

src/Controls/src/Core/Handlers/Items/iOS/CarouselViewController.cs Outdated

+              			}
+              		}
+              		internal override void DettachingFromWindow()

Member

PureWeen May 23, 2024

Suggested change

      
            		internal override void DettachingFromWindow()
          
            		internal override void DetachingFromWindow()

PureWeen reviewed

View reviewed changes

src/Controls/src/Core/Handlers/Items/iOS/CarouselViewController.cs Outdated Show resolved Hide resolved

rmarinho commented

View reviewed changes

src/Controls/tests/TestCases/Elements/CarouselViewCoreGalleryPage.xaml.cs

@@ @@ -139,13 +139,15 @@ public CarouselItem(int index, string image = null) @@
               			Index = index;
               			if (string.IsNullOrEmpty(image))
-              				Image = "https://picsum.photos/700/300/";
+              				Image = "oasis.jpg";

Member Author

rmarinho May 27, 2024

Just fix so we don't use a web image

rmarinho commented

View reviewed changes

src/Controls/tests/TestCases/Elements/CarouselViewCoreGalleryPage.xaml

@@ @@ -82,27 +82,31 @@ @@
                               CurrentItem="{Binding Selected}">
                               <CarouselView.ItemTemplate>
                                   <DataTemplate>
-                                      <Frame
+                                      <Border

Member Author

rmarinho May 27, 2024

We like Border

rmarinho commented

View reviewed changes

src/Controls/src/Core/PublicAPI/net-maccatalyst/PublicAPI.Unshipped.txt Outdated Show resolved Hide resolved

rmarinho commented

View reviewed changes

src/Controls/src/Core/Handlers/Items/iOS/MauiCollectionView.cs

               using CoreGraphics;
               using UIKit;
               namespace Microsoft.Maui.Controls.Handlers.Items;
-              internal class MauiCollectionView : UICollectionView
+              internal class MauiCollectionView : UICollectionView, IUIViewLifeCycleEvents

Member Author

rmarinho May 27, 2024

This allows the Controller to know how it can subscribe and cleanup better.

rmarinho commented

View reviewed changes

src/Controls/src/Core/Handlers/Items/iOS/MauiCollectionView.cs Show resolved Hide resolved

rmarinho and others added 5 commits

May 27, 2024 16:16


          [tests] Enable Uitests for setting CurrentItem on iOS

aa94893


          [iOS] Fix setting the initial CurrentItem

7c721e8


          cleanup naming

8aad9ef


          better template no need to get image from web

d2647f4


          Added another UI Test

86c5dfc

rmarinho added 11 commits

May 27, 2024 16:16


          Try add delay to check position.

d9baf44


          Try other way

a5c5b2b


          Test again

d833ec7


          [iOS] Don t update carousel position while doing rotation

4d5f854


          Fix build

11a426b


          [iOS] Use IUIViewLifeCycleEvents to better subscribe/unsubscribe

aa2697d


          [iOS] Make Sure to call Setup on attach the CarouselView

38468d8


          Fix build

e007452


          Try new thing

70f94d9


          Fix scroll animation

eb9d282


          some cleanup

4c45df2

rmarinho force-pushed the fix-18879 branch from 9386738 to 4c45df2 Compare

May 27, 2024 15:17

rmarinho commented

View reviewed changes

src/Controls/src/Core/Handlers/Items/CarouselViewHandler.iOS.cs Outdated

		this.GetDesiredSizeFromHandler(widthConstraint, heightConstraint);

		protected virtual double AnimationDuration => 0.5;

Member Author

rmarinho May 27, 2024

Should this be private for now? open on net9 ?

rmarinho added 3 commits

May 27, 2024 16:41


          Dont call base

186fe61


          [iOS] make sure to update IsScrolling

e9b499e


          [iOS] No need of new api

bb7e435

Member Author

rmarinho commented May 27, 2024

Seems is breaking this test FlyoutHeaderScroll

Scrolled Header: position 44 is no enough to cover height (-250). Epsilon: 0.3

https://dev.azure.com/xamarin/public/_build/results?buildId=116396&view=ms.vss-test-web.build-test-results-tab&runId=2821703&resultId=100285&paneView=debug

jsuarezruiz reviewed

View reviewed changes

src/Controls/tests/TestCases.Shared.Tests/Tests/CarouselViewUITests.cs

@@ @@ -17,48 +17,146 @@ public CarouselViewUITests(TestDevice device) @@
               		protected override void FixtureSetup()
               		{
               			base.FixtureSetup();
-              			App.NavigateToGallery(CarouselViewGallery);
+              			//App.NavigateToGallery(CarouselViewGallery);

Contributor

jsuarezruiz May 31, 2024

This was maybe commented testing and has it escaped?

src/Controls/src/Core/Handlers/Items/iOS/ItemsViewController.cs

+              			base.LoadView();
+              			var collectionView = new MauiCollectionView(CGRect.Empty, ItemsViewLayout);
+              			CollectionView = collectionView;
+              			(collectionView as IUIViewLifeCycleEvents).MovedToWindow += CollectionViewMovedToWindow;

Contributor

jsuarezruiz May 31, 2024

Unsubscribe when doing dispose

src/Controls/src/Core/Handlers/Items/iOS/MauiCollectionView.cs

               		if (!KeyboardAutoManagerScroll.IsKeyboardAutoScrollHandling)
               			base.ScrollRectToVisible(rect, animated);
               	}
+              	[UnconditionalSuppressMessage("Memory", "MEM0002", Justification = IUIViewLifeCycleEvents.UnconditionalSuppressMessage)]

Contributor

jsuarezruiz May 31, 2024

Also the CarouselView is included in the HandlerDoesNotLeak. Looks fine pasing the test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment