New XMLStyleRule variants for dynamic styling

[kosyak]

Hello and thank you for this fine library!

I propose addition of several XMLStyleRules to dynamically generate and apply styles, enter- and exit-insertions of elements.

With these changes, you can use them like this:

// We want to style an xml that contains tags with some important attributes

var quoteAuthorStack = [String]()

let attributedString = try NSAttributedString.composed(
    ofXML: rawXml,
    rules: [
        .style("quote", quoteStyle),

        // We want to insert an image with a beautiful quote before the actual quote text.
        // Also we have an author's name in "author" attribute and we want 
        // to append this name after the quote text.
        .enterBlock(element: "quote") { attributes in
            // Saving author's name for later use
            quoteAuthorStack.append(attributes["author"]!)

            // "author" handling was hacky, so to show `attributes` in action,
            // why don't we have different quote images for different occasions...
            return NSAttributedString.composed(of: [ 
                UIImage(named: attributes["quotestyle"]!)!, 
                Special.lineFeed 
            ])
        },
        
        .exitBlock(element: "quote") {
            // Using our saved "author" attribute here to append the name after the quote
            let author = quoteAuthorStack.popLast()!

            return NSAttributedString.composed(of: [ 
                Special.lineFeed, author.styled(with: authorStyle) 
            ])
        },

        // Also we have a "link" tag with some attributes that will help us
        // to generate an URL
        .styleBlock("link") { attributes in
            guard let type = attributes["type"], [ "event", "person" ].contains(type),
                let id = attributes["id"],
                let link = URL(string: "\(Const.Domain)/\(type)/\(id)") else { return defaultStyle }

            return defaultStyle.byAdding(.link(link))
        }
    ]
)

We have a BonMot patched with these changes for quite some time, they are very convenient for us :)

[ZevEisenberg]

@kosyak wow, this is a great addition! Thanks so much for contributing back your work so that others can benefit ❤️

I'd love to see some unit tests to make sure it keeps working as advertised. Is that something you'd be willing to add? I can help if you run into trouble.

[chrisballinger]

@kosyak This is great, thank you! I would also love to see the docs updated, with maybe an advanced/dynamic XML section explaining usage of this new functionality (similar to your PR description): https://github.com/Rightpoint/BonMot/blob/master/README.md#styling-parts-of-strings-with-xml

[kosyak]

Thanks for your support! I totally understand the need of tests and docs for this, will come up with those, won’t hesitate to ask for help :)

[kosyak]

@ZevEisenberg I've added unit tests (they're based on testAdditiveFontFeatures where I've found some code with .xmlRules). They may be in the wrong place or overcomplicated, please point me to the right direction.

[ZevEisenberg]

I had a quick look and left a few comments. I think they'll make the code a bit tidier 😄. Once you've done that, I'll give it a more thorough look. I'm wondering if we can come up with better names for the new cases?

[ZevEisenberg]

Raw strings make this a lot nicer:

Suggested change

	let string = "0<one attr1=\"11\">1<two attr2=\"12\">2</two></one>"
	let string = #"0<one attr1="11">1<two attr2="12">2</two></one>"#

[ZevEisenberg]

Also, I find the use of "11" and "12" in the mix to be confusing. I'd love longer and/or less ambiguous names, just to make it easier to read this code 🙂

[kosyak]

In this test, I've tried not only to extract and assert attribute values but also to have them included in the styling work.

So I've chosen one NSAttributeString parameter (baselineOffset) and tried to produce a style with a baselineOffset parsed from attribute's value in each of styleBlocks. Because of that, values of these attributes are numbers.

It doesn't matter for me what NSAttributedString style to use in the test, maybe there is a better fit instead of baselineOffset. E.g. we can have font names as attribute values and try to construct and return .font() style within blocks...

[ZevEisenberg]

I think you may be able to simplify this test. Rather than asserting things inside the closure, do some work in the closure, and afterward, assert that the result is as expected. Does that work for you, or would it miss some aspect of the way the closers work that you want to test?

[kosyak]

I pushed another commit where all guards are removed. A bit better, I think. Still, lots of typecasts and ??...

[kosyak]

@ZevEisenberg I've pushed fixes that hopefully will resolve some of your concerns.

I'm wondering if we can come up with better names for the new cases?

I'm looking at some Kotlin code right now, and there's lazyMessage here. We can have
case .lazyStyle(String, ([String: String]) -> StringStyle) (this is good, I think), .lazyEnter, .lazyExit (not so good...)

[ZevEisenberg]

Yeah, the Kotlin naming doesn't seem to fit too well here. What about styler(String, ([String: String]) -> StringStyle) to pair with the style(String, StringStyle) we already have? You could potentially even typealias Styler = ([String: String]) -> StringStyle.

[kosyak]

I'm fine with .styler. Should we rename .enterBlock, .exitBlock?

[ZevEisenberg]

Damn. I forgot about those. I'd be fine with enterStyler and exitStyler. But now styler by itself seems to be missing something. I'm probably fine with that, unless we can think of a good word for stylerThatIsNeitherEnterNorExit.

[kosyak]

typealias Styler = ([String: String]) -> StringStyle

I'm not sure where to put it - should it be near XMLStyleRule?. It could be within XMLStyleRule but there's already XMLStyleRule.Styler https://github.com/Rightpoint/BonMot/pull/391/files#diff-c9b2f909282ce5a1468d61cb0337c704R136.

Also, won't there be a confusion between our freshly renamed XMLStyleRule.styler and XMLStyleRule.Styler?

Here's one more naming option: .dynamicStyle, .dynamicEnter, .dynamicExit (or even .deferred...) - just yo be sure we thought of them all :)

[ZevEisenberg]

Wow, I'm glad one of the two of us is paying attention! 😅 I haven't worked on this code in a while, so I'm a little rusty. You make excellent points. I'll have a think about it when I've got some time, and I definitely hope to get this in soon.

[chrisballinger]

@kosyak Thanks so much! I think where you landed on the naming makes sense to me. To avoid having this sit open any longer I'm going to merge it as-is, but it would be great if you could follow up with a docs update PR to the README!

[kosyak]

Thanks for the merge, will work on readme/docs this week.