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 16 commits into main from fix-18879

+392 −84

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 a 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.

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 contains changes to the CarouselViewHandler and CarouselViewController in the src/Controls/src/Core/Handlers/Items/ directory. The changes primarily focus on improving the handling of carousel view items, including updating the position and visual states of items, managing the carousel loop, and handling device rotation. The changes also include additional null checks and the introduction of new private properties and methods.

Key changes include:

CarouselViewHandler.iOS.cs: The MapPosition method has been updated to handle the initial position of the carousel view. The changes ensure that the UpdateFromPosition method is only called when the initial position has been set.
CarouselViewController.cs: Several changes have been made to this file, including:
- The addition of a new Microsoft.Maui.Devices namespace.
- The introduction of new private properties (_isCenteringItem, _initialOrientation, _isRotating) and the renaming of existing properties (_initialPositionSet to InitialPositionSet, _updatingScrollOffset to _isCenteringItem). [1] [2] [3] [4] [5] [6]
- The addition of new methods (AttachingToWindow, DetachingFromWindow, OnDisplayInfoChanged) and the updating of existing methods (UpdateItemsSource, UpdateVisibility, UpdateLoop, UpdateFromCurrentItem, UpdateFromPosition, UpdateInitialPosition). These changes improve the handling of carousel view items and device rotation. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15]
CarouselViewLoopManager: The CenterIfNeeded, Dispose, GetCorrectPositionForCenterItem, GetGoToIndex, CenterVerticallyIfNeeded and CenterHorizontalIfNeeded methods have been updated to include additional null checks. [1] [2] [3] [4] [5]

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

+              			{
+              				return;
+              			}
               			SetPosition(e.CenterItemIndex);
               			UpdateVisualStates();

Member

PureWeen May 23, 2024

Do we need to call UpdateVisualStates after the rotation has finished?

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

    
            @@ -134,6 +134,7 @@ static Microsoft.Maui.Controls.Region.operator !=(Microsoft.Maui.Controls.Region
          
              static Microsoft.Maui.Controls.Region.operator ==(Microsoft.Maui.Controls.Region left, Microsoft.Maui.Controls.Region right) -> bool

              static Microsoft.Maui.Controls.Shapes.Matrix.operator !=(Microsoft.Maui.Controls.Shapes.Matrix left, Microsoft.Maui.Controls.Shapes.Matrix right) -> bool

              static Microsoft.Maui.Controls.Shapes.Matrix.operator ==(Microsoft.Maui.Controls.Shapes.Matrix left, Microsoft.Maui.Controls.Shapes.Matrix right) -> bool

              virtual Microsoft.Maui.Controls.Handlers.Items.CarouselViewHandler.AnimationDuration.get -> double

Member Author

rmarinho May 27, 2024

Not sure we want to make this public or not,

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

		@@ -1,9 +1,11 @@
		using System;
		using System.Diagnostics.CodeAnalysis;

Member Author

rmarinho May 27, 2024

Drop this line

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

		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 ?

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