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

Merged
merged 4 commits into from
Oct 27, 2022
Merged

Conversation

crazytonyli
Copy link
Contributor

@crazytonyli crazytonyli commented Oct 26, 2022

Issue

Fixes wordpress-mobile/WordPress-iOS#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 Media Section/Picker crashes the app WordPress-iOS#19505: go to "Media" screen and wait for a few seconds.

  • I have considered if this change warrants release notes and have added them to the appropriate section in the CHANGELOG.md if necessary.

@crazytonyli crazytonyli requested review from mokagio and a team October 26, 2022 01:06
@crazytonyli
Copy link
Contributor Author

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

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

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

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.

Odd. I remember reviewing your PR at the time, looking at the docs, and the change made sense to me.

Great idea leaving a note about the issue the setup aims to address. It will help if/when we'll get back here to improve the code.

CHANGELOG.md Outdated Show resolved Hide resolved
@mokagio mokagio enabled auto-merge October 27, 2022 01:01
@mokagio
Copy link
Contributor

mokagio commented Oct 27, 2022

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.

@mokagio mokagio merged commit d1e5dba into trunk Oct 27, 2022
@mokagio mokagio deleted the fix-incremental-update branch October 27, 2022 01:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Media Section/Picker crashes the app
3 participants