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 a crash during incremental update #398

Closed crazytonyli closed 1 year ago

crazytonyli commented 1 year ago

Issue

Fixes https://github.com/wordpress-mobile/WordPress-iOS/issues/19505.

I can easily reproduce this issue on current trunk branch of WordPress iOS repo, by using a site that has 36 photos in its media library and tapping the Media row in the My Site tab.

Changes

This PR reverts a commit in this PR whose changes were released in v1.8.5. I've verified this change fixed the crash. But I don't know why the reverted commit would cause a crash. Reloading inside the batch update block seems to be okay according to this API doc

Animates multiple insert, delete, reload, and move operations as a group.

Test instructions

  1. Prepare or choose a site that has 40 photos in its media library.
  2. Checkout WordPress-iOS trunk branch and pin the MediaPicker library dependency in the Podfile to this PR branch.
  3. Build and run the app.
  4. Go to the site prepared in step 1.
  5. Repeat the steps in https://github.com/wordpress-mobile/WordPress-iOS/issues/19505: go to "Media" screen and wait for a few seconds.
crazytonyli commented 1 year ago

@geriux @twstokes I've verified this PR fixed this media library crash: https://github.com/wordpress-mobile/WordPress-iOS/issues/19505, please feel free to test this PR if you'd like. A new version of MediaPicker library will be released once this PR is merged and I'll open a PR to WordPress-iOS repo. Thanks.

twstokes commented 1 year ago

@geriux @twstokes I've verified this PR fixed this media library crash: wordpress-mobile/WordPress-iOS#19505, please feel free to test this PR if you'd like.

Thanks for the quick fix @crazytonyli! I followed the testing steps and they worked great. 🎉 🙇

A new version of MediaPicker library will be released once this PR is merged and I'll open a PR to WordPress-iOS repo.

Just wanted to share that we'll often do two PRs in tandem from the start - one in the library and one in the app repo. This is nice for running CI on the app side early as well as making testing more convenient. Ultimately the app repo PR gets updated to point to the library's new release once it's merged. 👍

crazytonyli commented 1 year ago

@twstokes TIL, thanks! I've opened a PR in WordPress-iOS repo.

mokagio commented 1 year ago

Just wanted to share that we'll often do two PRs in tandem from the start - one in the library and one in the app repo. This is nice for running CI on the app side early as well as making testing more convenient.

I hope that in a not too distant future, we'll be able to iterate faster on the library repositories, so that we can do the bulk of the work there and not need to open PRs in tandem. This repo has an example app, but it doesn't fetch images from a site's media library.