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

[Down] Rendering to attributed string is really slow, requires main thread #100

Closed
1 task done
lilyball opened this issue Aug 20, 2018 · 9 comments · Fixed by #132
Closed
1 task done

[Down] Rendering to attributed string is really slow, requires main thread #100

lilyball opened this issue Aug 20, 2018 · 9 comments · Fixed by #132
Labels
Down Issues specific to the Down renderer enhancement help wanted

Comments

@lilyball
Copy link

lilyball commented Aug 20, 2018

Please help prevent duplicate issues before submitting a new one:

  • I've searched other open/closed issues for duplicates before opening up this new issue.

Report

Down's implementation of .toAttributredString() invokes NSAttributedString(htmlString:). This is problematic because it's really slow, and it also is only safe to do so on the main thread (under the hood this ends up WebKit to do the rendering).

Failing to render on main thread will result in a crash like in #116

Instead, Down should convert to AST and walk the AST, building up an attributed string directly. This approach would be much faster and would also be safe to run from a background thread.

@iwasrobbed
Copy link
Collaborator

iwasrobbed commented Aug 24, 2018

Thanks for the issue, @lilyball 👍

Indeed, that support was originally added to bridge the gap at the time, since many folks needed NSAttributedString support. I personally don't use that feature, but I'm open to others helping to improve upon it with a pull request

@iwasrobbed iwasrobbed added the Down Issues specific to the Down renderer label Nov 10, 2018
@iwasrobbed iwasrobbed changed the title Rendering to attributed string is really slow, requires main thread [Down] Rendering to attributed string is really slow, requires main thread Nov 10, 2018
@kengruven
Copy link
Collaborator

It looks like #81 was aiming to do this -- and now it's at wireapp/Down.

I haven't looked at it or even tried it, so I have no idea if it's complete, or how well it works.

@iwasrobbed
Copy link
Collaborator

@johnxnguyen has graciously offered to make a PR for their changes in #81 (after a little cleanup) and I would appreciate your review on that once it's ready @lilyball 🙏

@mglass
Copy link

mglass commented Apr 4, 2019

@johnxnguyen @iwasrobbed Can I help with this somehow? I'd love to be able to close the issues in my app that are related to this 🙂

@johnxnguyen
Copy link
Owner

Hey @mglass , sorry I have been quite busy at work, but I have the day off today so I'll try to get as much done as I can by the end of the weekend. I'll let you know of course if I need some help 😄

@mglass
Copy link

mglass commented Apr 5, 2019

@johnxnguyen I have a fork in place at https://github.com/mglass/Down that merges your branch with the current master, but I am flailing around somewhat blindly with it. Maybe it'll help as a reference point?

@johnxnguyen
Copy link
Owner

@mglass a quick update: I'm re-implementing what I've done for Wire but with some tweaks to make it more flexible. The Wire implementation is quite specific and instead I want to make it simpler for the library user to be able to control how the attributed string is rendered. The idea is use a visitor pattern and a styler protocol. This protocol contains a style method for each markdown element and each method is invoked by a visitor class.

I'm working directly from the Down repo on a feature branch, I'll push up my current progress tonight after work and create a draft PR for you to check it out. Then we can move any discussions off this thread 😄

@johnxnguyen
Copy link
Owner

johnxnguyen commented Apr 8, 2019

@mglass feel free to have a look #132.

@iwasrobbed
Copy link
Collaborator

FYI: this has been further improved with a default Styler via #177

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Down Issues specific to the Down renderer enhancement help wanted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants