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

[Feature] Custom list prefixes for AttributedStringVisitor #255

Merged
merged 2 commits into from Apr 28, 2021

Conversation

dloic
Copy link
Contributor

@dloic dloic commented Apr 19, 2021

After investigation, I've tried to find the least intrusive way to add the feature of custom list prefixes for AttributedStringVisitor. I've decided to go for ListItemPrefixgenerator changes, as fitting the feature in the styler was not possible.

The PR is not changing the current behavior, but opening a bit the API so one can provide another implementation. See below for an example:

Definition of a custom ListPrefixGenerator:

public class CustomListItemPrefixGeneratorBuilder: ListItemPrefixGeneratorBuilder {
    public func build(listType: List.ListType, numberOfItems: Int, nestDepth: Int) -> ListItemPrefixGenerator {
        return CustomListItemPrefixGenerator(listType: listType, numberOfItems: numberOfItems, nestDepth: nestDepth)
    }
    
}

public class CustomListItemPrefixGenerator: ListItemPrefixGenerator {

    private var prefixes: IndexingIterator<[String]>

    required public init(listType: List.ListType, numberOfItems: Int, nestDepth: Int) {

        switch listType {
        case .bullet:
            let prefix = CustomListItemPrefixGenerator.unorderedPrefix(nestDepth: nestDepth)
            prefixes = [String](repeating: prefix, count: numberOfItems)
                .makeIterator()

        case .ordered(let start):
            prefixes = (start..<(start + numberOfItems))
                .map { CustomListItemPrefixGenerator.orderedPrefix(value: $0, nestDepth: nestDepth) }
                .makeIterator()
        }
    }

    public func next() -> String? {
        prefixes.next()
    }

    private static func unorderedPrefix(nestDepth: Int) -> String {
        switch nestDepth {
        case 0:
            return ""
        case 1:
            return ""
        case _ where nestDepth >= 2:
            return "▪︎"
        default:
            return ""
        }
    }

    private static func orderedPrefix(value: Int, nestDepth: Int) -> String {
        switch value {
        case 1:
            return "1️⃣"
        case 2:
            return "2️⃣"
        case 3:
            return "3️⃣"
        default:
            return "🔢"
        }
    }
}

Usage:

let document = try! markdown.toDocument(.default)
let visitor = AttributedStringVisitor(
    styler: DownStyler(),
    options: .default,
    listPrefixGeneratorBuilder: CustomListItemPrefixGeneratorBuilder()
)
subview.attributedText = document.accept(visitor)

Render:
Capture d’écran, le 2021-04-19 à 15 47 27

@codecov
Copy link

codecov bot commented Apr 19, 2021

Codecov Report

Merging #255 (71cd933) into master (2524b62) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #255      +/-   ##
==========================================
+ Coverage   91.29%   91.31%   +0.01%     
==========================================
  Files          60       60              
  Lines        1057     1059       +2     
==========================================
+ Hits          965      967       +2     
  Misses         92       92              
Impacted Files Coverage Δ
...es/Down/AST/Visitors/AttributedStringVisitor.swift 91.66% <100.00%> (+0.14%) ⬆️
...es/Down/AST/Visitors/ListItemPrefixGenerator.swift 100.00% <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 2524b62...71cd933. Read the comment docs.

@dloic
Copy link
Contributor Author

dloic commented Apr 20, 2021

Hi @johnxnguyen! Not sure who should be reviewing this one :)

I've checked the Travis CI build error, seems related to log file length, I cannot see the error. Do you confirm?

@johnxnguyen
Copy link
Owner

Hey @dloic , thanks for the PR! I'll try to take a look at this tonight and get back to you soon.

@dloic
Copy link
Contributor Author

dloic commented Apr 20, 2021

@johnxnguyen thanks a lot!

Copy link
Owner

@johnxnguyen johnxnguyen left a comment

Choose a reason for hiding this comment

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

Hey 👋🏻 Sorry to keep you waiting, I had quite a busy week.

Generally it looks good to me! I just have a few suggestions to tidy it up a little. Let me know what you think.

@johnxnguyen
Copy link
Owner

I've checked the Travis CI build error, seems related to log file length, I cannot see the error. Do you confirm?

@dloic hmm apparently Travis enforces a 4MB log length limit, it's the first time I've seen this issue though. Try to add the - quiet option at the end of the three xcodebuild commands in the travis config file, that should shrink the log output sufficiently.

@dloic
Copy link
Contributor Author

dloic commented Apr 26, 2021

@johnxnguyen Thanks for the review. Branch updated taking into account all your feedback.
I did also tweak the travis script as suggested. Let see the result!

Updated usage of the new feature with builder as closure:

let visitor = AttributedStringVisitor(
    styler: DownStyler(),
    options: .default,
    listPrefixGeneratorBuilder: { CustomListItemPrefixGenerator(list: $0) }
)

Copy link
Owner

@johnxnguyen johnxnguyen left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍🏻 Thanks @dloic for your contribution!

@johnxnguyen johnxnguyen merged commit 0586cec into johnxnguyen:master Apr 28, 2021
@dloic
Copy link
Contributor Author

dloic commented Apr 28, 2021

Thanks to you @johnxnguyen for the review and approval! There is no rush, but I was wondering if you plan to do a new release for the new changes.

@johnxnguyen
Copy link
Owner

I will draft a new release likely by the end of this week.

@dloic
Copy link
Contributor Author

dloic commented Apr 28, 2021

Perfect, thanks a lot!

@johnxnguyen
Copy link
Owner

FYI @dloic this has been release. Thanks again for the contribution!

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.

None yet

2 participants