search

wordpress-mobile / MediaPicker-iOS

WPMediaPicker is an iOS controller that allows capture and picking of media assets.
GNU General Public License v2.0
110 stars 37 forks source link

Fix incremental update #394

Closed crazytonyli closed 2 years ago

crazytonyli commented 2 years ago

Issue

When the media library is updated incrementally with changed items, the collection view shows incorrect thumbnails for some items.

This issue can be easily reproduced from WordPress-iOS repo trunk branch. To reproduce this issue,

  1. Change ContextManager in WordPress-iOS repo to using ContainerContextFactory.
  2. Launch WordPress app from Xcode.
  3. [Desktop browser] Go to a WordPress.com site.
  4. [Desktop browser] Add or delete photos in the site's media library.
  5. [WordPress app] Go to the site's "Media" screen in the launched WordPress-iOS app.
  6. Repeat step 4 and 5, check out the collection view in the media library screen.

https://user-images.githubusercontent.com/1101828/188596443-ff1efbf6-104b-4ec1-b949-5933dbfc13c6.mov

Change

During batch updates, the changed items were reloaded manually by calling the configuring cell method, instead of using the collection view. I believe this approach causes the issue since index path of the item in collection view doesn't necessary match the index path of the media library data, not during batch updates.

This PR moves the reloading changed items code to the batch update's completion block, which seems like a more reliable place to reload items.

mokagio commented 2 years ago

@crazytonyli what do you think about updating this in the clients?

It seems to me like an edge case bug, so not worth targeting the frozen branch?

crazytonyli commented 2 years ago

@mokagio Looking at the doc, it seems it's more appropriate to reload the items with other updates all together. I've updated this PR accordingly, which now only changed how changed item are reloaded and seems like a minimal change.

The app isn't affected by this issue actually since the app is using LegacyContextFactory, not ContainerContextFactory, and this issue only occurs when ContainerContextFactory is used.

mokagio commented 2 years ago

@mokagio Looking at the doc, it seems it's more appropriate to reload the items with other updates all together. I've updated this PR accordingly, which now only changed how changed item are reloaded and seems like a minimal change.

Well there you go. You always learn something new, even from the docs you (me in this case!) linked yourself.

The app isn't affected by this issue actually since the app is using LegacyContextFactory, not ContainerContextFactory, and this issue only occurs when ContainerContextFactory is used.

Cool. No rush in upgrading then.

Re-iterating my approval. Merge and ship a new version at will.