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] Re-enable screenshots when app is in background #12590

Merged
merged 5 commits into from Oct 2, 2019

Conversation

@danielebogo
Copy link
Contributor

commented Oct 1, 2019

Fixes #12554

This PR fix the problem introduced fixing #12540 where the app wasn't able to keep the screenshot in background. Using @koke suggestion and the new UIColor API resolvedColor(with:) I'm able to generate an attributed string for the dark and light user interface and assign it.

To test:

• Run this branch on iOS 13 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
• On the App switch screen, change user interface style (light to dark or vice versa) and see if the app screenshot change
• Test on iOS 12 to check everything works fine

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.4 milestone Oct 1, 2019
@danielebogo danielebogo requested review from frosty and koke Oct 1, 2019
@danielebogo danielebogo self-assigned this Oct 1, 2019
@danielebogo danielebogo added this to In Review in Dark Mode Oct 1, 2019
@@ -765,22 +769,45 @@ open class ReaderDetailViewController: UIViewController, UIViewControllerRestora
guard let post = post else {
return
}

textView.isPrivate = post.isPrivate()
textView.content = post.contentForDisplay()

This comment has been minimized.

Copy link
@koke

koke Oct 1, 2019

Member

Unrelated to this PR, but I noticed it the other day, and this ☝️ is doing the same thing that we're already doing afterwards and generating the attributed string twice.

This comment has been minimized.

Copy link
@danielebogo

danielebogo Oct 1, 2019

Author Contributor

So textView.content = post.contentForDisplay() is not necessary because the attributed text is set after it right?

This comment has been minimized.

Copy link
@koke

koke Oct 1, 2019

Member

Yes, the content setter actually does:

attributedText = WPRichContentView.formattedAttributedStringForString(newValue)
textView.attributedText = isDark ? darkTextViewAttributedString : lightTextViewAttributedString
} else {
let attrStr = WPRichContentView.formattedAttributedStringForString(post.contentForDisplay())
textView.attributedText = attributedString(with: attrStr)

This comment has been minimized.

Copy link
@koke

koke Oct 1, 2019

Member

It makes me a bit uncomfortable that we're pre-creating the attributed string on iOS 13 but generating it on demand on earlier versions. It would be interesting to pre-create it in both scenarios, with just the number of variants changing, but I can't think of a specific suggestion that doesn't seem to overcomplicate things ¯\_(ツ)_/¯

Dark Mode automation moved this from In Review to Approved Oct 1, 2019
@koke
koke approved these changes Oct 1, 2019
Copy link
Member

left a comment

Changes look good and seems to work fine on iOS 13.1. I don't have any devices on iOS 12 right now, so maybe someone else can test that

@frosty
frosty approved these changes Oct 2, 2019
Copy link
Contributor

left a comment

Looks good to me, and still looks fine on iOS 12 👍

@danielebogo

This comment has been minimized.

Copy link
Contributor Author

commented Oct 2, 2019

Thanks @frosty @koke

@danielebogo danielebogo merged commit c164b58 into develop Oct 2, 2019
6 checks passed
6 checks passed
Hound No violations found. Woof!
Peril All green. Yay.
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
Dark Mode automation moved this from Approved to Done Oct 2, 2019
@danielebogo danielebogo deleted the fix/background-style-change branch Oct 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Dark Mode
  
Done
3 participants
You can’t perform that action at this time.