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

Style boldItalics not rendered correctly #51

Open
funkenstrahlen opened this issue Apr 16, 2019 · 9 comments
Open

Style boldItalics not rendered correctly #51

funkenstrahlen opened this issue Apr 16, 2019 · 9 comments

Comments

@funkenstrahlen
Copy link
Contributor

funkenstrahlen commented Apr 16, 2019

The following example should render as bold and italics at the same time:

***foobar***

Like this: foobar

However it is only rendered in bold style.

@funkenstrahlen
Copy link
Contributor Author

This is actually supported, but no used in any of the example theme files. Therefore I close this.

@funkenstrahlen
Copy link
Contributor Author

After some more testing I have discovered a bug that arises from a library design flaw.

In the current implementation ***foobar*** matches three regular expressions for bold, italics and boldItalics. It only renders in the correct style if the regex for boldItalics is processed after the other ones. But this is currently not guaranteed in the code. Therefore it works sometimes and sometimes does not.

The styles are applied here:

        for (style) in theme.styles {
            style.regex.enumerateMatches(in: backingString, options: .withoutAnchoringBounds, range: range, using: { (match, flags, stop) in
                guard let match = match else { return }
                backingStore.addAttributes(style.attributes, range: match.range(at: 0))
            })
        }

The order of styles in theme is dependent on the JSONSerialization when reading from disk. As styles in the theme JSON is defined as a dictionary no order is guaranteed.

I think to fix this issue the boldItalics case needs to be rethought completely because it requires a different render architecture. Currently I do not have an idea on how to solve this.

@funkenstrahlen funkenstrahlen changed the title Combining bold and italics not working Style boldItalics not rendered correctly reliably Apr 16, 2019
@funkenstrahlen funkenstrahlen changed the title Style boldItalics not rendered correctly reliably Style boldItalics not rendered correctly Apr 16, 2019
@funkenstrahlen
Copy link
Contributor Author

Maybe the issue can be fixed by improving the regular expression for bold, italic and boldItalic.

@ruddfawcett
Copy link
Owner

ruddfawcett commented Apr 16, 2019

Thanks for bringing this to my attention; the styles do need to be reworked a bit and are probably a part of a massive overhaul that includes separating tokens such as *, **, ***, etc. (à la #28). I will think on this and get back to you.

@funkenstrahlen
Copy link
Contributor Author

Awesome! Let me know if you have an idea how to implement this. I am open to help writing the required code.

@DivineDominion
Copy link
Collaborator

I found it works well if you don't just command it to e.g. "set this to italics", but instead "fetch the current font and add the italic font trait".

For bold and italics, I call embolden and italicize:

    public func regularize(range: NSRange) {

        let font = self.font(at: range.location + range.length / 2)
        self.addStyleAttribute(.font, value: font.regularized(), range: range)
    }

    public func embolden(range: NSRange) {

        let font = self.font(at: range.location + range.length / 2)
        self.addStyleAttribute(.font, value: font.emboldened(), range: range)
    }

    public func italicize(range: NSRange) {

        let font = self.font(at: range.location + range.length / 2)
        self.addStyleAttribute(.font, value: font.italicized(), range: range)
    }

    public func font(at location: Int) -> UniversalFont {

        return self.styleAttribute(.font, at: location) as? UniversalFont
            ?? UniversalFont.systemFontOfDefaultSize
    }

With this:

public typealias UniversalColor = NSColor
public typealias UniversalFont = NSFont
public typealias UniversalFontDescriptor = NSFontDescriptor

extension UniversalFont {

    func italicized() -> UniversalFont {
        return NSFontManager().convert(self, toHaveTrait: .italicFontMask)
    }

    func emboldened() -> UniversalFont {
        return NSFontManager().convert(self, toHaveTrait: .boldFontMask)
    }

    func regularized() -> UniversalFont {
        return NSFontManager().convert(self, toHaveTrait: [.unboldFontMask, .unitalicFontMask])
    }

    static var systemFontOfDefaultSize: UniversalFont {
        return UniversalFont.systemFont(ofSize: UniversalFont.systemFontSize)
    }
}

@ruddfawcett
Copy link
Owner

@DivineDominion love that approach! @funkenstrahlen if you want to take a look...

@funkenstrahlen
Copy link
Contributor Author

I just took a look at this and still do not know how to integrate this in to the existing structure.

Currently it works like this:

  1. The Theme file is parsed. This creates an array of Style objects
  2. Each Style object represents a regex and all attributes for the NSTextStorage to apply to the matching text range

Problems with this approach:

Merging of font traits settings for two Style objects (matching the same regex) does not work, because font traits are part of the font attribute and are therefore not merged together but replaced when calling backingStore.addAttributes(style.attributes, range: match.range(at: 0))

This could be solved by applying font traits separately after all other styles have been set. However this is not possible because currently there is no access to the traits attribute in the json theme file other than through Styles.

@funkenstrahlen
Copy link
Contributor Author

funkenstrahlen commented Apr 22, 2019

Maybe there is a way to build this just for bold, italic and boldItalic elements. I could create exceptions for these elements and add the font traits manually. However this means loosing the option to customise font traits in the theme definition file for headlines for example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants