Skip to content
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.

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

Fix incremental update #394

Merged
merged 5 commits into from
Sep 8, 2022
Merged

Fix incremental update #394

merged 5 commits into from
Sep 8, 2022

Conversation

crazytonyli
Copy link
Contributor

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.
Screen.Recording.2022-09-06.at.9.11.20.PM.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.

@crazytonyli crazytonyli requested a review from a team September 6, 2022 09:24
@crazytonyli crazytonyli self-assigned this Sep 6, 2022
@crazytonyli crazytonyli added the bug label Sep 6, 2022
Copy link
Contributor

@mokagio mokagio left a comment

Choose a reason for hiding this comment

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

I'm pretty rusty when it comes to UICollectionView, but I think this change makes sense.

From the docs, it doesn't seem like accessing indexes within the update block is recommended, so moving that code afterwards seems like the way to go.

Deletes are processed before inserts in batch operations. This means the indexes for the deletions are processed relative to the indexes of the collection view’s state before the batch operation, and the indexes for the insertions are processed relative to the indexes of the state after all the deletions in the batch operation.

CHANGELOG.md Outdated
Comment on lines 6 to 32
- `1.8.1` Release - [1.8.1](#1.8.1)
- `1.8.0` Release - [1.8](#1.8.0)
- `1.7.0` Release - [1.7](#1.7.0)
- `1.6.0` Release - [1.6](#1.6.0)
- `1.5.0` Release - [1.5](#1.5.0)
- `1.4.2` Release - [1.4.2](#1.4.2)
- `1.4` Release - [1.4](#1.4)
- `1.3.4` Release - [1.3.4](#1.3.4)
- `1.3` Release - [1.3](#1.3)
- `1.2` Release - [1.2](#1.2)
- `1.1` Release - [1.1](#1.1)
- `1.0` Release - [1.0](#1.0)
- `0.28` Release - [0.28](#28)
- `0.27` Release - [0.27](#27)
- `0.26` Release - [0.26](#26)
- `0.25` Release - [0.25](#25)
- `0.24` Release - [0.24](#24)
- `0.23` Release - [0.23](#23)
- `0.22` Release - [0.22](#22)
- `0.21` Release - [0.21](#21)
- `0.20` Release - [0.20](#20)
- `0.19` Release - [0.19](#19)
- `0.18` Releases - [0.18](#18)
- `0.17` Releases - [0.17](#17)
- `0.16` Releases - [0.16](#16)
- `0.15` Releases - [0.15](#15)
- `1.8.5` Release - [1.8.5](#185)
- `1.8.1` Release - [1.8.1](#181)
- `1.8.0` Release - [1.8](#180)
- `1.7.0` Release - [1.7](#170)
- `1.6.0` Release - [1.6](#160)
- `1.5.0` Release - [1.5](#150)
- `1.4.2` Release - [1.4.2](#142)
- `1.4` Release - [1.4](#14)
- `1.3.4` Release - [1.3.4](#134)
- `1.3` Release - [1.3](#13)
- `1.2` Release - [1.2](#12)
- `1.1` Release - [1.1](#11)
- `1.0` Release - [1.0](#10)
- `0.28` Release - [0.28](#028)
- `0.27` Release - [0.27](#027)
- `0.26` Release - [0.26](#026)
- `0.25` Release - [0.25](#025)
- `0.24` Release - [0.24](#024)
- `0.23` Release - [0.23](#023)
- `0.22` Release - [0.22](#022)
- `0.21` Release - [0.21](#021)
- `0.20` Release - [0.20](#020)
- `0.19` Release - [0.19](#019)
- `0.18` Releases - [0.18](#018)
- `0.17` Releases - [0.17](#017)
- `0.16` Releases - [0.16](#016)
- `0.15` Releases - [0.15](#015)
Copy link
Contributor

Choose a reason for hiding this comment

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

For what is worth, I think we could get rid of this table of contents.

Having it is not a pattern I've seen elsewhere in the repo. The internal links look suspiciously like PRs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleted in 61afeb2 😄

@mokagio
Copy link
Contributor

mokagio commented Sep 7, 2022

@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
Copy link
Contributor Author

@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
Copy link
Contributor

mokagio commented Sep 8, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants