DragonCherry / AssetsPickerViewController

Powerfully Customizable - Multiple Photo & Video Picker Controller
MIT License
388 stars 136 forks source link

Added guard for sort and fetch albumsArray #68

Closed mikemike396 closed 4 years ago

mikemike396 commented 4 years ago

This fixes the crash that happens if you scroll fast then click done while the UICollectionView is still scrolling. The crash also seems to happen the first time you are opening an album sometimes.

This is the original crash we were getting, I also attached more up to date stack trace. This bug is a bit tricky to reproduce.

Steps to reproduce:

  1. Fresh install (Albums should be full of images to allow scrolling up and down)
  2. Launch picker select 1-2 images
  3. Scroll up and down that list very fast
  4. While the list is still scrolling click the done button

** We have also noticed this crash when swapping between albums fast and clicking the done button.

Original Crash: https://github.com/DragonCherry/AssetsPickerViewController/issues/40

image

mikemike396 commented 4 years ago

@DragonCherry Thanks!

giannigdev commented 2 years ago

Hi @mikemike396 , we are experiencing the following crash at this line in the code (without your solution) and we are trying to understand if your solution covers all the edge cases:

Line 69 var updatedIndexSet = updatedIndexSets[section]

Can you explain why did you put the guard after the second nested for loop rather than before the line 69 where the section is accessed for the first time from the updatedIndexSets property assuming that section was causing the same crash to you?

Is it because that guard logic was dependent from the logic produced by the nested for loop and so you had to put the guard after the end of that for loop or did you had a crash produced by a different line ?

mikemike396 commented 2 years ago

Our crash was happening right after the guard. We didn't notice any crashes in the outer loop.

var oldSortedAlbums = sortedAlbumsArray[section]
let newSortedAlbums = sortedAlbums(fromAlbums: fetchedAlbumsArray[section])
giannigdev commented 2 years ago

@mikemike396 Ok so we have a different crash. Anyway if the property interested by the crash is the section one it should be more secure to include also in your case the first time the section is being accessed (line 69) especially when we consider as you said that the crash it seems also happening sometime when opening for the first time the album so you may have not included all the edge cases in your solution even if the exceptions occurs much later in the scope of the code, just saying; thanks for the feedback ;)

mikemike396 commented 2 years ago

Makes sense! Feel free to open a PR to add the additional case. Thanks for the detailed response!

giannigdev commented 2 years ago

@mikemike396 We need to figure out first if the extra guard case is needed or if it could also work for the crash you have reported so it could be a single guard statement at the beginning of the outer for loop. Otherwise if the logic of the nested for loop is required to validate the checks of the guard you have added after it (i don't know if you remember that, more than 2 years have passed xD) then maybe a 2 guard statements solution is required :)

mikemike396 commented 2 years ago

I honestly don't remember off hand! We moved to another library awhile back.