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

Add optional stylesheet argument for NSAttributedString renderer #79

Merged
merged 2 commits into from
Mar 2, 2018
Merged

Add optional stylesheet argument for NSAttributedString renderer #79

merged 2 commits into from
Mar 2, 2018

Conversation

kengruven
Copy link
Collaborator

The issue that I’m looking at here is somewhat subtle. I’m mostly looking to start a conversation, at this point. I’m not 100% sure the right way for Down.framework to approach this, so I’m presenting one possible method.

The problem

There’s a lot of issues about fonts with NSAttributedString (#14, #15, #18, #21), so clearly there’s some frustration here.

The way Down.framework works right now is: toAttributedString() calls toHTML(), and then it makes an NSAttributedString from that. This is an easy way to get from a valid Down object to a valid NSAttributedString object, and by Down’s policy of “it’s just a converter”, this is a solution.

The problem is with how NSAttributedString’s interface works. Its default font (as documented in the API) is Helvetica 12, and so that's what you get if you make an NSAttributedString(string: “hello, world”).

But if you use the NSAttributedString(html:Data) initializer, this kicks it into some kind of special HTML rendering mode, which doesn’t use the default font. It uses a tiny serif font. Perhaps it’s trying to emulate Safari, which uses Times by default.

Why Down.framework should care

I can see 3 reasons why Down.framework should get involved here.

First, Markdown is as much a plain text format as an HTML format. People write it like plain text, and it’s designed to be perfectly readable that way. (The existence of Down.framework reinforces this: it renders into 6 different formats, not just HTML.) When people call .toAttributedString(), they aren’t asking for “an NSAttributedString of a Safari view of this Markdown”. They want “an NSAttributedString of this Markdown”.

Second, for every other renderer, Down (like cmark) uses the default font of that backend. The LaTeX output isn’t “LaTeX version of a Safari rendering of Markdown”. It uses the default LaTeX font.

Third, it’s not so easy to change this from outside the library (the previous suggested approach). NSAttributedString is immutable. It’s possible to make a mutable copy and update the fonts, but (AFAICT) it’s really a lot of work. (Personally, I’d write my own Markdown renderer before I went down that path.) Fixing it from inside DownAttributedStringRenderable is easy. Why bother providing an NSAttributedString renderer, if users are going to have to rewrite it themselves to make one that works the way they want?

One solution

My proposed solution is straightforward. Keep using the HTML data path, but prepend a tiny stylesheet which makes it use the default NSAttributedString font. (Menlo, for monospaced text, isn’t a documented default of NSAttributedString, but it’s the standard monospaced font on macOS, and it’s a built-in font on every platform that Down supports.)

This PR makes the stylesheet an optional string, so it’s easy to get the previous Safari-like behavior, too, if you want that:

  • To use NSAttributedString’s default font, call .toAttributedString()
  • To use the Safari-style rendering, call .toAttributedString(stylesheet: "")
  • To use some other custom styles, call .toAttributedString(stylesheet: whateverYouWant)

I’m not entirely happy with this solution, but it’s simple, and provides the default font by default, and allows users to customize it if they need to.

By default, it uses a stylesheet that mimics the NSAttributedString default font.
@coveralls
Copy link

coveralls commented Feb 27, 2018

Coverage Status

Coverage increased (+0.2%) to 47.923% when pulling 0f3fe14 on kengruven:nsattributedstring into d0c23a3 on iwasrobbed:master.

Copy link
Collaborator

@iwasrobbed iwasrobbed left a comment

Choose a reason for hiding this comment

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

Fair enough! Thanks for weighing the pros/cons and providing some default behavior. Also, have a look at BonMot sometime.

The only nitpick I'd ask is to update the README (or example app) w/ this info so people understand it's an option without digging in too much

@iwasrobbed
Copy link
Collaborator

(Btw, you should have an invite to be a collaborator so you can merge this when you're ready)

@kengruven
Copy link
Collaborator Author

Good call on the README. I realized after I submitted this that I'd forgotten to update the docstrings, too. So I've got some cleanup to do.

I'll take a look at BonMot -- looks interesting. I don't use NSAttributedString that much, myself, except for making some simple labels. (But maybe I will now!)

I saw the collaborator invite, but I wasn't sure about it. It's nice to know that there still seems to be an "approve" check before my changes go directly into master.

@kengruven
Copy link
Collaborator Author

Finally got around to updating the documentation! Look good? Any suggestions?

@iwasrobbed
Copy link
Collaborator

Looks good @kengruven !

@kengruven
Copy link
Collaborator Author

Great, and thanks! Merging.

@kengruven kengruven merged commit 09b9b54 into johnxnguyen:master Mar 2, 2018
@kengruven kengruven deleted the nsattributedstring branch March 2, 2018 20:29
@emlynmac
Copy link

emlynmac commented Mar 3, 2018

I was just looking for this very feature as I'm having to replace my markdown library. And then I saw it only was merged 4 hours ago!

When would this be available as a cocoa pod release? I guess I can just grab from GIT in the meantime.

@iwasrobbed
Copy link
Collaborator

Should be good now @emlynmac 🙌 Just run pod update Down --repo-update

@emlynmac
Copy link

emlynmac commented Mar 3, 2018

Thanks!

@iwasrobbed
Copy link
Collaborator

Please see the new AST functionality for rendering and customizing attributed strings: #132

It should be much faster than how WebKit renders

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.

4 participants