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.

Sign up for GitHub

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

Jump to bottom

Do not process italic syntax on links. #3

Merged
merged 7 commits into from
Aug 21, 2023
Merged

Do not process italic syntax on links. #3

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a3fa003
Change parsing order to not lose URL data.
kublaios Aug 17, 2023
77da9bf
Fix `MarkdownItalic` bug for capturing the preceding char.
kublaios Aug 17, 2023
11641fd
Do not process italic syntax on links.
kublaios Aug 17, 2023
9b47c31
Revert "Change parsing order to not lose URL data."
kublaios Aug 21, 2023
ae6c3b1
Revert "Do not process italic syntax on links."
kublaios Aug 21, 2023
fd14740
Update parsing order to avoid malformed autolinks.
kublaios Aug 21, 2023
42b6b4e
Update demo template to demonstrate more cases.
kublaios Aug 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion MarkdownKit/Sources/Common/Elements/Italic/MarkdownItalic.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import Foundation

open class MarkdownItalic: MarkdownCommonElement {

fileprivate static let regex = "(.?|^)(\\*|_)(?=\\S)(.+?)(?<![\\*_\\s])(\\2)"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This regex was implemented in bmoliveira@761a30e to "fix" when the italic syntax is combined with bold and/or other characters, but it was capturing the preceding character (e.g., when capturing _italic_, it would capture _italic_ with the space before. I reverted it as it doesn't make much sense for our use case, and it still works with bold text.

fileprivate static let regex = "(\\s|^)(\\*|_)(?=\\S)(.+?)(?<![\\*_\\s])(\\2)"

open var font: MarkdownFont?
open var color: MarkdownColor?
Expand All @@ -24,6 +24,13 @@ open class MarkdownItalic: MarkdownCommonElement {
}

public func match(_ match: NSTextCheckingResult, attributedString: NSMutableAttributedString) {
// We check if the matched substring already has a link attribute,
// which can occur when the URL has multiple underscores
// (e.g. https://website.com?user_id=abc&session_id=xyz)
guard !self.attributedString(attributedString, hasAttribute: .link, at: match.range) else {
return
}

attributedString.deleteCharacters(in: match.range(at: 4))

var attributes = self.attributes
Expand All @@ -45,4 +52,15 @@ open class MarkdownItalic: MarkdownCommonElement {

attributedString.deleteCharacters(in: match.range(at: 2))
}

private func attributedString(
_ attributedString: NSAttributedString,
hasAttribute attribute: NSAttributedString.Key,
at range: NSRange
Copy link

Choose a reason for hiding this comment

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

It looks to me that this function only checks that there is an attribute at the first position in the range, right?

If that's intended maybe we can change the param to at location: Int to make it clearer

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, it turned out that we don't need this check at all - just updating the regex was sufficient, so I reverted this part. Thanks for the nudge!

) -> Bool {
let attributedSubstring = attributedString.attributedSubstring(from: range)
var effectiveRange = NSRange(location: 0, length: 0)
let atributes = attributedSubstring.attributes(at: 0, effectiveRange: &effectiveRange)
return atributes.keys.contains(attribute)
Copy link

Choose a reason for hiding this comment

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

nit: (assuming we don't need to check the whole range) I think it can be simplified to:

attributedString.attribute(attribute, at: range.position, effectiveRange: nil) != nil

}
}
7 changes: 4 additions & 3 deletions MarkdownKit/Sources/Common/MarkdownParser.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,16 +151,17 @@ open class MarkdownParser {
}

fileprivate func updateDefaultElements() {
// Parsing order matters!
// Parsing order matters! For example, links should be parsed before italic,
// so URLs with multiple underscores don't lose the underscores.
let pairs: [(EnabledElements, MarkdownElement)] = [
(.header, header),
(.list, list),
(.quote, quote),
(.link, link),
(.automaticLink, automaticLink),
(.bold, bold),
(.italic, italic),
(.strikethrough, strikethrough),
(.link, link),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When the link was being parsed before, it would join the text coming after the link, and the autolink would incorrectly make the following text a part of the link.

(.automaticLink, automaticLink),
(.code, code),
]
defaultElements = pairs.compactMap { enabled, element in
Expand Down
Loading