This repository has been archived by the owner on May 1, 2024. It is now read-only.
[iOS] Fix when removing current last Item on CarouselView #12837
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hartez
suggested changes
Nov 17, 2020
Xamarin.Forms.Platform.iOS/CollectionView/CarouselViewController.cs
Outdated
Show resolved
Hide resolved
jsuarezruiz
suggested changes
Nov 19, 2020
There was a problem hiding this comment.
Trying to remove the latest item:
=================================================================
Managed Stacktrace:
=================================================================
at <unknown> <0xffffffff>
at ObjCRuntime.Messaging:void_objc_msgSend_IntPtr_IntPtr <0x0010d>
at UIKit.UICollectionView:PerformBatchUpdates <0x00432>
at Xamarin.Forms.Platform.iOS.ObservableItemsSource:BatchUpdate <0x002d4>
at <Remove>d__35:MoveNext <0x00b5a>
at System.Runtime.CompilerServices.AsyncTaskMethodBuilder:Start <0x001be>
at Xamarin.Forms.Platform.iOS.ObservableItemsSource:Remove <0x00262>
at <CollectionChanged>d__31:MoveNext <0x0050a>
at System.Runtime.CompilerServices.AsyncTaskMethodBuilder:Start <0x001be>
at Xamarin.Forms.Platform.iOS.ObservableItemsSource:CollectionChanged <0x00262>
at <CollectionChanged>d__30:MoveNext <0x006b2>
at System.Runtime.CompilerServices.AsyncVoidMethodBuilder:Start <0x001be>
at Xamarin.Forms.Platform.iOS.ObservableItemsSource:CollectionChanged <0x002e2>
at System.Collections.ObjectModel.ObservableCollection`1:OnCollectionChanged <0x000fa>
at System.Collections.ObjectModel.ObservableCollection`1:OnCollectionChanged <0x00103>
at System.Collections.ObjectModel.ObservableCollection`1:RemoveItem <0x0018a>
at System.Collections.ObjectModel.Collection`1:Remove <0x001e8>
at Xamarin.Forms.Controls.Issues.ViewModelIssue12574:ExecuteRemoveItemsCommand <0x0012a>
at Xamarin.Forms.Controls.Issues.ViewModelIssue12574:<.ctor>b__12_1 <0x00072>
at <>c__DisplayClass4_0:<.ctor>b__0 <0x00074>
at Xamarin.Forms.Command:Execute <0x0008c>
at Xamarin.Forms.ButtonElement:ElementClicked <0x001a7>
at Xamarin.Forms.Button:SendClicked <0x0007a>
at Xamarin.Forms.Platform.iOS.ButtonElementManager:OnButtonTouchUpInside <0x000ec>
at Xamarin.Forms.Platform.iOS.ButtonRenderer:OnButtonTouchUpInside <0x000ba>
at UIKit.UIControlEventProxy:Activated <0x000b1>
at System.Object:runtime_invoke_void__this__ <0x001a5>
at <unknown> <0xffffffff>
at UIKit.UIApplication:UIApplicationMain <0x00254>
at UIKit.UIApplication:Main <0x000b2>
at UIKit.UIApplication:Main <0x00132>
at UIKit.UIApplication:Main <0x002aa>
at Xamarin.Forms.ControlGallery.iOS.Application:Main <0x0009a>
at <Module>:runtime_invoke_void_object <0x001a8>
=================================================================
|
@jsuarezruiz can you give me the steps to reproduce ? I tried load the issue, and click remove until no items are available |
…CollectionView reloads it's items
hartez
force-pushed
the
fix-12574test
branch
from
58b3926
to
15b0276
Compare
…ler; Reset position/current item on ItemsSource update Don't update position until drag is released
jsuarezruiz
approved these changes
Dec 3, 2020
| { | ||
| public static class IndexPathHelpers | ||
| { | ||
| public static NSIndexPath[] GenerateIndexPathRange(int section, int startIndex, int count) |
There was a problem hiding this comment.
Could we add some specific tests to this helper?
| public static NSIndexPath[] GenerateLoopedIndexPathRange(int section, int sectionCount, int iterations, int startIndex, int count) | ||
| { | ||
| var result = new NSIndexPath[iterations * count]; | ||
| var step = sectionCount / iterations; |
There was a problem hiding this comment.
interactions could be zero here?
| @@ -527,7 +527,14 @@ public override bool ShouldInvalidateLayoutForBoundsChange(CGRect newBounds) | |||
| return base.ShouldInvalidateLayoutForBoundsChange(newBounds); | |||
| } | |||
|
|
|||
| UpdateConstraints(CollectionView.AdjustedContentInset.InsetRect(newBounds).Size); | |||
| if (Forms.IsiOS11OrNewer) | |||
| } | ||
| } | ||
|
|
||
| async Task CollectionChanged(NotifyCollectionChangedEventArgs args) | ||
| void CollectionChanged(NotifyCollectionChangedEventArgs args) |
There was a problem hiding this comment.
I think there are some issues related to "SIGABRT: Index was out of range. Must be non-negative and less than the size of the collection. Parameter name: index", I wonder if this change can impact on those issues. We could do a quick review of them after merging this PR.
2 tasks
pictos
pushed a commit
to pictos/Xamarin.Forms
that referenced
this pull request
Dec 30, 2020
) * [iOS] Fix when removing current last Item on CarouselView * [Uitests] Try fix test for 12574 * [iOS] Fix scroll when removing item on CarouselView * [iOS] Use CollectionView.PerformBatchUpdates to scroll to item after CollectionView reloads it's items * [iOS] Set Reload next to Scroll to better understand the logic between the 2 * [iOS] Fix for reload items and wait for iOS12 and iOS11 * Provide pre and post update events for CarouselView to do its bookkeeping * Remove animation after item removal * [Controls] Fix sample 12574 * [UITests] Enable test * Remove performBatchUpdates calls to prevent data disparity; * Remove async stuff from ObservableGroupedSource * Fix reload race condition with deletion/insertion of groups * Reset current item and position when ItemsSource resets * Update ObservableSource count before invoking Carousel's changed handler; Reset position/current item on ItemsSource update Don't update position until drag is released * Clean up unnecessary ReloadRequired stuff * Handle UICollectionView internal accounting edge cases Co-authored-by: E.Z. Hart <hartez@gmail.com>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of Change
When adding test for #12574 I saw that when we are removing the current last item sometimes the scroll to the previous position wasn't happening, this was related to ReloadingVisibleItems that is need when using the Loop feature, so we can update the UI when cell is removed. Adding a small delay will make the collection reload and then set the CollectionView to the correct position
Issues Resolved
API Changes
None
Platforms Affected
Behavioral/Visual Changes
Remove the last item should then show the previous item
Before/After Screenshots
Not applicable
Testing Procedure
Issue12574 should pass on iOS
PR Checklist