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

[Dark Mode] Crash for unexpected state #12540

Merged
merged 3 commits into from Sep 25, 2019

Conversation

@danielebogo
Copy link
Contributor

commented Sep 24, 2019

Fixes #12519

This PR fixes a crash that happens when the app changes its app state to .background.
Because the app is notified that the trait is changed, it reloads the content to update the style. That generates an exception with the mutable attributed string used to generate the post content.

To test:

• Run this branch and go to the reader
• Open a post and check everything works normally
• Switch user interface style (light to dark or vice versa)
• Put the app in background and check it doesn't crash
• Switch user interface style (light to dark or vice versa) and bring back the app
• Put the app in background again and check it doesn't crash

Update release notes:

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.
@danielebogo danielebogo added this to the 13.3 ❄️ milestone Sep 24, 2019
@danielebogo danielebogo requested review from frosty and loremattei Sep 24, 2019
@danielebogo danielebogo self-assigned this Sep 24, 2019
@danielebogo danielebogo requested a review from jkmassel Sep 24, 2019
@jkmassel jkmassel requested review from jtreanor and removed request for jkmassel Sep 24, 2019
@jkmassel

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2019

I'll leave this for @jtreanor to 👍 as how is now the release manager for WPiOS , but it looks like this PR needs changes to Podfile.lock committed – you can fix this by running bundle exec pod install in the repo, then committing the changes! :)

@jtreanor

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2019

I'm happy to include this in 13.3 once someone with some Dark Mode context performs the review.

Copy link
Contributor

left a comment

The code change looks good to me.
I tested following the provided steps and I was able to reproduce the issue using the release/13.3 branch, but not using this one. 👍

:shipit:

@frosty
frosty approved these changes Sep 25, 2019
Copy link
Contributor

left a comment

Thanks for fixing this!

I was looking if we could move the fix into formattedAttributedStringForString somehow, but I can't see an easy way of doing it. I think we'd need to update that method to return an optional or throw an error if we're in the background. We already have another fix for the same issue in https://github.com/wordpress-mobile/WordPress-iOS/blob/develop/WordPress/Classes/ViewRelated/Reader/ReaderCommentsViewController.m#L909, so if we could consolidate them in one place that seems like it'd be a win.

If it's not easy, happy to roll with this fix instead!

@danielebogo danielebogo merged commit 9b19ac0 into release/13.3 Sep 25, 2019
6 checks passed
6 checks passed
Hound No violations found. Woof!
Peril All green. Well done.
Details
ci/circleci: Build Tests Your tests passed on CircleCI!
Details
ci/circleci: UI Tests (iPad Air 3rd generation) Your tests passed on CircleCI!
Details
ci/circleci: UI Tests (iPhone 11) Your tests passed on CircleCI!
Details
ci/circleci: Unit Tests Your tests passed on CircleCI!
Details
@danielebogo

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2019

Thanks @frosty @loremattei !

@koke

This comment has been minimized.

Copy link
Member

commented Sep 26, 2019

I missed this one, so I re-investigated this in #12554. The PR fixed the crash, but broke the app switcher when changing color schemes while the app is in the background: #12554 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.