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

[NT-400] Pledge View attributedString crash fix #900

Merged
merged 4 commits into from
Oct 21, 2019

Conversation

ifbarrera
Copy link
Contributor

@ifbarrera ifbarrera commented Oct 17, 2019

πŸ“² What

Fixes a crash on PledgeViewController when the devices is rotated or the dynamic type size is changed.

πŸ€” Why

Because crashes are bad.

πŸ›  How

This crash was due to some strange async-like behavior when parsing an html string using:

NSMutableAttributedString(
    data: Data(string.utf8),
    options: [
      .documentType: NSAttributedString.DocumentType.html,
      .characterEncoding: String.Encoding.utf8.rawValue
    ],
    documentAttributes: nil
  )

We're not super sure what's actually the root cause of this issue, since during debugging we were able to see that this call was happening on the main thread. What we observed was that parsing an html string would actually cause a subsequent call to traitCollectionDidChange to occur in what would appear to be an infinite loop. Debouncing the trigger to the attributedConfirmationString function seems to solve the issue, although it seems to possibly be related to a deeper issue with constructing an attributed string with html options.

πŸ‘€ See

Pledge View Update Pledge View
a1Wnw8QpKc bLOVJEI5NG

βœ… Acceptance criteria

  • Navigate to the pledge screen and rotate to landscape. There should be no crash. Repeat by increasing dynamic type size - there should be no crash.
  • Navigate to the update pledge screen and rotate to landscape. There should be no crash. Repeat by increasing dynamic type size - there should be no crash.

Copy link
Contributor

@justinswart justinswart left a comment

Choose a reason for hiding this comment

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

A strange crash and strange fix! But great job digging into this πŸ‘

@ifbarrera ifbarrera merged commit aa8d135 into master Oct 21, 2019
@ifbarrera ifbarrera deleted the pledge-view-crash-fix branch October 21, 2019 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants