Skip to content
This repository has been archived by the owner on May 1, 2024. It is now read-only.

[iOS, Android] Fix when we call ReloadData on CollectionView, Fix issue adding item to start of Collection on ANdroid #10450

Merged
merged 6 commits into from Apr 28, 2020

Conversation

rmarinho
Copy link
Member

@rmarinho rmarinho commented Apr 24, 2020

Description of Change

I found that #9911 introduced a new concept of Reload the CollectionView if it wasn't visible and an update to the items was in progress.
Seems this approach doesn't work well when we push a modal on top of the CollectionView and then on dispose we add/remove items from the CollectionView. This was actually preventing the item from being removed. So my first try was to remove the check for _collectionViewController.View.Window == null , this worked but was going around the real problem.

It seems that also the _batchUpdating.WaitAsync() on the Reload method isn't working as expected, it was getting stuck because there were no available threads on the _batchUpdating semaphore. The problem was after a BatchUpdates was executed (for example add a item) the semaphore wasn't releasing and making the threads available. So the next time for example if it wanted to Reload it was getting stuck.

Fix issue on Android that Scrolling to the current position when adding an item would scroll the last item.

This is actual a regression from previous versions.

Issues Resolved

API Changes

None

Platforms Affected

  • iOS

Behavioral/Visual Changes

None

Before/After Screenshots

Not applicable

Testing Procedure

Added UITest 10300
Click Add
Click Delete
Click Close,
You should see the item 2

PR Checklist

  • Targets the correct branch
  • Tests are passing (or failures are unrelated)

@rmarinho rmarinho requested a review from hartez April 24, 2020 23:26
@rmarinho rmarinho changed the title [iOS] Improve when we call ReloadData on CollectionVIew [iOS] Fix when we call ReloadData on CollectionView Apr 25, 2020
@samhouts samhouts added p/iOS 🍎 a/collectionview a/carouselview i/high Completely doesn't work, crashes, or is unusably slow, has no obvious workaround; occurs less often t/bug 🐛 labels Apr 25, 2020
@samhouts samhouts added this to In Review in v4.6.0 Apr 25, 2020
@rmarinho
Copy link
Member Author

Not sure if we should still would remove this check https://github.com/xamarin/Xamarin.Forms/blob/4.6.0/Xamarin.Forms.Platform.iOS/CollectionView/ObservableItemsSource.cs#L287

_collectionViewController.View.Window == null

In the issue i created Window is null because we were showing a Modal and going back to the page with collectionview

@samhouts samhouts added the blocker Issue blocks next stable release. Prioritize fixing and reviewing this issue. label Apr 27, 2020
Copy link
Member

@samhouts samhouts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test failed on Android API 19

@rmarinho rmarinho changed the title [iOS] Fix when we call ReloadData on CollectionView [iOS, Android] Fix when we call ReloadData on CollectionView, Fix issue adding item to start of Collection on ANdroid Apr 28, 2020
@samhouts samhouts merged commit c60fa54 into 4.6.0 Apr 28, 2020
v4.6.0 automation moved this from In Review to Done Apr 28, 2020
@samhouts samhouts deleted the fix_issue10300 branch April 28, 2020 22:22
@samhouts samhouts added this to Done in Sprint 169 Apr 30, 2020
@samhouts samhouts added this to Done in CarouselView May 5, 2020
@samhouts samhouts added this to the 4.6.0 milestone May 12, 2020
@samhouts samhouts removed this from Done in CarouselView Jun 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a/carouselview a/collectionview blocker Issue blocks next stable release. Prioritize fixing and reviewing this issue. i/high Completely doesn't work, crashes, or is unusably slow, has no obvious workaround; occurs less often p/Android p/iOS 🍎 t/bug 🐛
Projects
No open projects
Sprint 169
  
Done
v4.6.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants