johnxnguyen / Down

Blazing fast Markdown / CommonMark rendering in Swift, built upon cmark.
Other
2.26k stars 327 forks source link

DownStyler: Add paragraph style and font for quotes #223

Closed foxware00 closed 4 years ago

foxware00 commented 4 years ago

Happy to explain the use cases further

foxware00 commented 4 years ago

Hi @foxware00 , thanks for the pull request, I'm always happy to see people engaged in this code. Could you explain a bit more why you need to expose the BlockBackgroundColorAttribute?

The code is great, I've really enjoyed digging through it, some very interesting things going on in the DownLayoutManager that I didn't know was possible!

This is roughly what I'm trying to achieve, it's a little messy, but I'm using a third-party syntax highlighter for code blocks, I then apply the attributes from that, and add the same ones being used in DownStyler. I essentially want everything DownStyler is doing + font colours for syntax highlighting.

override func style(codeBlock str: NSMutableAttributedString, fenceInfo: String?) {
    if let codeBlock = codeHighlighter.highlight(str.string, as: fenceInfo) {
        let blockBackgroundAttribute = BlockBackgroundColorAttribute(
                color: colors.codeBlockBackground,
                inset: codeBlockOptions.containerInset)

        let adjustedParagraphStyle = paragraphStyles.code.inset(by: blockBackgroundAttribute.inset)
        let mutableCodeBlock = NSMutableAttributedString(attributedString: codeBlock)

        mutableCodeBlock.addAttributes(
                [
                    NSAttributedString.Key.paragraphStyle: adjustedParagraphStyle,
                    NSAttributedString.Key.blockBackgroundColor: blockBackgroundAttribute
                ],
                range: NSRange(location: 0, length: mutableCodeBlock.length))
        str.setAttributedString(mutableCodeBlock)
    } else {
        super.style(codeBlock: str, fenceInfo: fenceInfo)
    }
}
johnxnguyen commented 4 years ago

The code is great, I've really enjoyed digging through it, some very interesting things going on in the DownLayoutManager that I didn't know was possible!

I'm glad to hear it! I learned a lot about TextKit while writing this code. I always thought that we were limited to the predefined string attributes, but it turns out it's not so hard to create your own attributes. There's just not much documentation about how to do it.

Correct me if I'm wrong, with this line

let codeBlock = codeHighlighter.highlight(str.string, as: fenceInfo)

the codeHighlighter will return an NSAttributedString with various colors set for the .foregroundColor attribute. Does it also apply any other attributes?

If it is only the foreground color that we care about, then I think it would be better to apply those attributes (in their specific ranges) on the existing attributed string that has been styled by Down. This would keep the code simple since we would not have to rebuild the attributed string to preserve all the attributes that were lost due to the syntax highlighting.

This means your subclass of the DownStyler would override the method like this:

open override func style(codeBlock str: NSMutableAttributedString, fenceInfo: String?) {
    super.style(codeBlock: str, fenceInfo: fenceInfo)
    applySyntaxHighlighting(in: str, using: fenceInfo)
}

You'd of course need to figure out the ranges and values of all .foregroundColor attributes in codeBlock. But there are some helper extensions defined in NSAttributedString+Helpers.swift that will make this easier. Something like:

func applySyntaxHighlighting(in str: NSMutableAttributedString, using fenceInfo: String?) {
    let codeBlock = codeHighlighter.highlight(str.string, as: fenceInfo)

    codeBlock.enumerateAttributes<UIColor>(for: .foregroundColor) { attribute, range in
        str.addAttribute(for: .foregroundColor, value: attribute, range: range)
    }
}

The enumerate method will go through all values of .foregroundColor giving you the value and range of that value. So if you use this on the string returned by your highlighter, you can "copy" those colors on the already styled code block. Be careful though to ensure that codeBlock and str have the same underlying string, otherwise you may get out of index errors.

I haven't tested this code of course, it's just an idea. Do you think it would work?

You could then

foxware00 commented 4 years ago

I'm glad to hear it! I learned a lot about TextKit while writing this code. I always thought that we were limited to the predefined string attributes, but it turns out it's not so hard to create your own attributes. There's just not much documentation about how to do it.

Absolutely it's a bit of a dark arts from my reading. On a seperate note, I will be looking at creating a markdown editor, hopefully leaveraging all the hard work done here. Is this something any of the contributors at Down have thought about or have alternatives?

Thank you for that, that was super details and almost perfect. You're correct it's merely modifying the font colour so it should line up nicely. The only thing is enumerateAttributes is not public so I can push something to that effect. I'm not sure your thoughts on making some or all of those methods public. I'll leave that with you, happy to apply anything into this PR.

For anyone following after the enumerate should look like this.

codeBlock.enumerateAttributes(for: .foregroundColor) { (attribute: UIColor, range) in
        str.addAttribute(.foregroundColor, value: attribute, range: range)
}
foxware00 commented 4 years ago

@johnxnguyen I've been playing around with fixing the tests and I see what you mean about the inline styles. It's a bit dirty, but if we check the current font is equal to the body font, then we can override the body font. The alternative is providing a set of Quote fonts and headings which seems overkill. It's a bit of a shame as internally we want our quotes to look like below. I guess this goes a little against the commonmark spec so it gets a little tricky. It might be I do something similar to the syntax highlighting and apply the "quote" styling

Screenshot 2020-08-13 at 17 42 09

I think this might be cleaner than adding the quote font. I need to make updateExistingAttributes public again. I've massively simplified the lot and just made those two methods public. I think it might make sense to make some of the other ones public for other people, but I guess it might make more sense on a case by case basis

override func style(blockQuote str: NSMutableAttributedString, nestDepth: Int) {
    super.style(blockQuote: str, nestDepth: nestDepth)
    str.updateExistingAttributes(for: .font) { (currentFont: DownFont) in
        guard currentFont == fonts.body else {
            return currentFont
        }
        return UIFont.regularItalic(.font18)
    }
}
johnxnguyen commented 4 years ago

On a seperate note, I will be looking at creating a markdown editor, hopefully leaveraging all the hard work done here. Is this something any of the contributors at Down have thought about or have alternatives?

I've been thinking about an implementing an editor for a while and have tried various ways, but never reached a final implementation that I was happy with. I guess it depends on how complex the editor needs to be. I'd be happy to share any learnings I have on the topic.

The only thing is enumerateAttributes is not public

That helper extension is just a little wrapper around the standard NSAttributedString method, you can directly use that method rather than exposing the extensions in Down.

foxware00 commented 4 years ago

I've been thinking about an implementing an editor for a while and have tried various ways, but never reached a final implementation that I was happy with. I guess it depends on how complex the editor needs to be. I'd be happy to share any learnings I have on the topic.

That would be fanstatic if you could. I think our use case will start simple.

Possible additions

I need to carve out some time to really dig into it, but reading through DownLayoutManager and the likes has given me a lot of inspiration for it.

That helper extension is just a little wrapper around the standard NSAttributedString method, you can directly use that method rather than exposing the extensions in Down

Right you are, I'll close this PR then and expose those helpers my side. Many thanks for all your help!

codecov[bot] commented 4 years ago

Codecov Report

Merging #223 into master will not change coverage. The diff coverage is 50.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #223   +/-   ##
=======================================
  Coverage   68.63%   68.63%           
=======================================
  Files          74       74           
  Lines        2047     2047           
=======================================
  Hits         1405     1405           
  Misses        642      642           
Impacted Files Coverage Δ
...elpers/Extensions/NSAttributedString+Helpers.swift 80.00% <0.00%> (ø)
...ensions/NSMutableAttributedString+Attributes.swift 70.83% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 296ad54...56cdf61. Read the comment docs.

johnxnguyen commented 4 years ago

@foxware00 regarding the quote font:

It's possible for a given node to know what kind of nodes its ancestors are. This means a text node can know if it is a child of a quote node. With this information, it is possible to dynamically select between a normal base font and a quote base font. This would preserve the inline styles with are applied afterwards.

But then you'd also need quote versions of the heading fonts... and I'm a little hesitant to do that. I'll try to think if there is another way.

johnxnguyen commented 4 years ago

I need to carve out some time to really dig into it, but reading through DownLayoutManager and the likes has given me a lot of inspiration for it.

I'm glad to hear it. I guess I'd need to know a bit more about how your editor should behave, so if you want to discuss it feel free to get in contact.

foxware00 commented 4 years ago

It's possible for a given node to know what kind of nodes its ancestors are. This means a text node can know if it is a child of a quote node. With this information, it is possible to dynamically select between a normal base font and a quote base font. This would preserve the inline styles with are applied afterwards.

But then you'd also need quote versions of the heading fonts... and I'm a little hesitant to do that. I'll try to think if there is another way.

I think you're right, holding a bunch of quote heading font's isn't ideal. I've gone for the approach i've mentioned above it very simply achieves the solution. I'll close this PR and migrate the helper functions into my project, thanks for all your help. I might be in touch regarding the editor when I can carve out some time.