Support titles for links and images #42

bensyverson · 2020-01-23T23:12:30Z

I didn't learn this until yesterday, but apparently stock Markdown supports titles in links and images:

Here is a [link](https://swiftbysundell.com/ "Good stuff here")

Same thing for images, meaning ![Pupper](foo.jpg "A cute puppy") should turn into:

<img src="foo.jpg" alt="Pupper" title="A cute puppy"/>

It's also supported in the link definition syntax, with any amount of same-line whitespace, and two other delimiters:

[sbs]: https://swiftbysundell.com     "Now powered by Publish"
[sbs]: https://swiftbysundell.com  'Now powered by Publish'
[sbs]: https://swiftbysundell.com (Now powered by Publish)

It even supports putting the title on the next line and using spaces for padding:

[sbs]: https://swiftbysundell.com
              "Now powered by Publish"

I took a very quick pass at implementing this functionality in Ink, and added a few tests to kick the tires. But I probably haven't spent enough time in the Ink codebase to do things 100% idiomatically. So please take or leave any of this!

Thanks @JohnSundell for creating such a powerful Markdown parser—I'm already customizing Ink via a Modifier in a Publish plug-in, which is a testament to how powerful this suite of tools really is!

john-mueller · 2020-02-02T05:53:50Z

This is an awesome start! I haven't looked at the actual code yet, but spec tests are promising (see corresponding examples here).

Newly passing CommonMark tests are 166, 327, 481, 485, 504, 523, and 535.
Newly failing CommonMark tests are 179, 483, 487, 488, and 563.

It looks like the newly failing tests should be pretty easy to fix; I'll try to look at your code and give specifics tomorrow.

Thanks for working on this, @bensyverson!

bensyverson · 2020-02-03T17:04:28Z

Nice, thanks @john-mueller ! I'll take a look at the newly failing tests too and poke around in the codebase. 🙏

john-mueller

With these tweaks, this PR passes 7 new CommonMark tests (166, 327, 481, 485, 504, 523, 535), and does not cause any CommonMark regressions.

All the changes, as well as an additional 4 tests to guard against regressions, are on the link-titles-fixup branch at https://github.com/john-mueller/Ink. I suggest merging these changes into your feature branch before merging this PR. If it would be easier, I can open a PR to add them to your feature branch.

Thanks again for adding this!

john-mueller · 2020-02-03T16:52:39Z

Sources/Ink/Internal/Character+Classification.swift

@@ -8,9 +8,31 @@ internal extension Character {
    var isSameLineWhitespace: Bool {
        isWhitespace && !isNewline
    }
+
+    var isLegalInURL: Bool {
+        self != ")" && self != " "


Suggested change

self != ")" && self != " "

self != ")" && self != " " && self != "\n"

This helps fix the new failure of CommonMark test 487:
"The destination cannot contain line breaks" (spec)

john-mueller · 2020-02-03T17:05:39Z

Sources/Ink/Internal/Link.swift

@@ -19,20 +20,35 @@ internal struct Link: Fragment {

        if reader.currentCharacter == "(" {
            reader.advanceIndex()
-            let url = try reader.read(until: ")")
-            return Link(target: .url(url), text: text)
+            let url = try reader.readCharacters(matching: \.isLegalInURL)


Suggested change

let url = try reader.readCharacters(matching: \.isLegalInURL)

let url = try? reader.readCharacters(matching: \.isLegalInURL)

This helps fix the new failure of CommonMark tests 483 and 563:
"Both the title and the destination may be omitted" (spec)
"Inline links also take precedence" (spec)

If both title and destination are missing, then this line encounters ")" immediately and throws, which ends link parsing. So instead of throwing, we assign nil to the url if it is not present, and use a default value of "" on line 33.

john-mueller · 2020-02-03T17:06:41Z

Sources/Ink/Internal/Link.swift

+                titleText = try reader.read(until: "\"")
+            }
+            try reader.read(")")
+            return Link(target: .url(url), text: text, title: titleText)


Suggested change

return Link(target: .url(url), text: text, title: titleText)

return Link(target: .url(url ?? ""), text: text, title: titleText)

Needed since url is now optional.

john-mueller · 2020-02-03T17:33:02Z

Sources/Ink/Internal/URLDeclaration.swift

+			if reader.currentCharacter.isSameLineWhitespace {
+				try reader.readWhitespaces()
+			}
+            if let delimeter = TitleDelimeter(rawValue: reader.currentCharacter) {


Suggested change

if let delimeter = TitleDelimeter(rawValue: reader.currentCharacter) {

if let delimeter = TitleDelimeter(rawValue: reader.currentCharacter) {

let index = reader.currentIndex

We capture the index at the start of the title parsing, in case there is an otherwise valid title followed by non-whitespace. If that happens, we rewind the reader to the beginning of what would have been the title, and set titleText = nil.

john-mueller · 2020-02-03T17:38:16Z

Sources/Ink/Internal/URLDeclaration.swift

+			}
+            if let delimeter = TitleDelimeter(rawValue: reader.currentCharacter) {
+                reader.advanceIndex()
+                titleText = try reader.read(until: delimeter.closing)


Suggested change

titleText = try reader.read(until: delimeter.closing)

titleText = try reader.read(until: delimeter.closing)

reader.discardWhitespaces()

if !reader.didReachEnd && !reader.currentCharacter.isWhitespace {

reader.moveToIndex(index)

titleText = nil

}

This helps fix the new failure of CommonMark test 179:
"No further non-whitespace characters may occur on the line." (spec & example)

bensyverson · 2020-02-03T19:06:35Z

Amazing, thanks @john-mueller! Just merged in your changes 🥳

john-mueller

👍 Looks good to me!

finestructure · 2020-02-22T16:32:41Z

Just wanted to chime in and alert you to PR #47 which I've just opened and which is somewhat related.

Some Markdown editors support width=400 height=300 attributes for images and it would be nice to roll both title and those into one single change perhaps.

PR #47 is already agnostic over what attributes it can handle. What it doesn't do is deal with quoting. I.e. while it currently does support title=foo, it won't handle title="foo bar" correctly.

I'm pretty sure with what's in this PR here my PR #47 could be extended (although I haven't looked at the changes in here in detail yet).

What are your thoughts to merge the two approaches?

finestructure · 2020-02-22T16:37:12Z

Hold on, I misread the syntax for the title definition. It's not declared the same way as the width and height parameters, which are = pairs.

Still, supporting both title and = pairs at the same time would probably need some parsing coordination so that one doesn't clobber or impede the other.

bensyverson · 2020-02-24T21:25:42Z

@finestructure Attributes on images/links would be fantastic—I've often wanted this for image sizes. I'll take a look and see if there's a nice clean way to merge the two!

john-mueller · 2020-02-24T22:45:24Z

I left a note on #47 that is relevant.

bensyverson added 4 commits January 23, 2020 16:45

Adds support for link and image titles

ac551ff

Fixes whitespace

ebce0cd

Supports link definition titles on next line with padding

a041d96

Adds tests to allTests, fixes title attribute for images

a0914fa

bensyverson requested a review from JohnSundell January 24, 2020 02:30

Fixup for bensyverson/link-titles

402e0b3

john-mueller suggested changes Feb 3, 2020

View reviewed changes

john-mueller approved these changes Feb 3, 2020

View reviewed changes

finestructure mentioned this pull request Feb 22, 2020

Add support for attributes in links (width, height, etc) #47

Closed

john-mueller mentioned this pull request Mar 26, 2020

Make images inline elements #43

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support titles for links and images #42

Support titles for links and images #42

bensyverson commented Jan 23, 2020

john-mueller commented Feb 2, 2020

bensyverson commented Feb 3, 2020

john-mueller left a comment

john-mueller Feb 3, 2020

john-mueller Feb 3, 2020

john-mueller Feb 3, 2020

john-mueller Feb 3, 2020

john-mueller Feb 3, 2020

bensyverson commented Feb 3, 2020

john-mueller left a comment

finestructure commented Feb 22, 2020

finestructure commented Feb 22, 2020 •

edited

Loading

bensyverson commented Feb 24, 2020

john-mueller commented Feb 24, 2020

	self != ")" && self != " "
	self != ")" && self != " " && self != "\n"

	let url = try reader.readCharacters(matching: \.isLegalInURL)
	let url = try? reader.readCharacters(matching: \.isLegalInURL)

	return Link(target: .url(url), text: text, title: titleText)
	return Link(target: .url(url ?? ""), text: text, title: titleText)

	if let delimeter = TitleDelimeter(rawValue: reader.currentCharacter) {
	if let delimeter = TitleDelimeter(rawValue: reader.currentCharacter) {
	let index = reader.currentIndex

-                titleText = try reader.read(until: delimeter.closing)
+                titleText = try reader.read(until: delimeter.closing)
+                reader.discardWhitespaces()
+                if !reader.didReachEnd && !reader.currentCharacter.isWhitespace {
+                    reader.moveToIndex(index)
+                    titleText = nil
+                }

Support titles for links and images #42

Are you sure you want to change the base?

Support titles for links and images #42

Conversation

bensyverson commented Jan 23, 2020

john-mueller commented Feb 2, 2020

bensyverson commented Feb 3, 2020

john-mueller left a comment

Choose a reason for hiding this comment

john-mueller Feb 3, 2020

Choose a reason for hiding this comment

john-mueller Feb 3, 2020

Choose a reason for hiding this comment

john-mueller Feb 3, 2020

Choose a reason for hiding this comment

john-mueller Feb 3, 2020

Choose a reason for hiding this comment

john-mueller Feb 3, 2020

Choose a reason for hiding this comment

bensyverson commented Feb 3, 2020

john-mueller left a comment

Choose a reason for hiding this comment

finestructure commented Feb 22, 2020

finestructure commented Feb 22, 2020 • edited Loading

bensyverson commented Feb 24, 2020

john-mueller commented Feb 24, 2020

finestructure commented Feb 22, 2020 •

edited

Loading