Add support for image, rules and footnotes #40919

GuillaumeGomez · 2017-03-30T01:49:44Z

Part of #40912.

r? @rust-lang/docs

PS: the footnotes are waiting for pulldown-cmark/pulldown-cmark#21 to be merged to be fully working.

critiqjo · 2017-03-30T09:41:58Z

src/test/rustdoc/check-rule-image-footnote.rs

+/// [link]: https://example.com "this is a title"
+///
+/// hard break:
+/// after hard break


I believe this generates a sequence like:

[..., Text("hard break:"), SoftBreak, Text("after hard break"), ...]

To generate a HardBreak in place of SoftBreak, there needs to be at least 2 trailing spaces right before the newline character:

"hard break \nafter hard break"

ref

The issue is even before the markdown parsing, a trim is being done somewhere so the text the parser receives never has lines ending with whitespaces.

I added a test for it. Should be fully working now.

critiqjo

Footnote implementation is incomplete.

critiqjo · 2017-03-30T09:53:55Z

src/librustdoc/html/markdown.rs

+                    let cur_len = parser.footnotes.len() + 1;
+                    parser.footnotes.push(format!("<li id=\"ref{}\">{}<a href=\"#supref{0}\" \
+                                                   rev=\"footnote\">↩</a></li>",
+                                                  cur_len, content));


This is wrong. For example:

foot[^note1], foot[^note2] [^note1]: text1 [^note2]: text2

Will give the following event sequence:

[ Start(Paragraph), Text("foot"), FootnoteReference("note1"), Text(", foot"), FootnoteReference("note2"), End(Paragraph), Start(FootnoteDefinition("note1")), Start(Paragraph), Text("text1"), End(Paragraph), End(FootnoteDefinition("note1")) Start(FootnoteDefinition("note2")), Start(Paragraph), Text("text2"), End(Paragraph), End(FootnoteDefinition("note2")) ]

Correct me if I am wrong, but according to this code snippet, both footnote definitions will have the same id cur_len.

Oh absolutely! Thanks for notifying it!

critiqjo · 2017-03-30T09:55:34Z

src/librustdoc/html/markdown.rs

+                Event::FootnoteReference(_) => {
+                    buffer.push_str(&format!("<sup id=\"supref{0}\"><a href=\"#ref{0}\">{0}</a>\
+                                              </sup>",
+                                             parser.get_next_footnote_id()));


Footnote ids cannot be this simple either. The same footnote can be referenced from multiple places. So you can't simply keep incrementing the id whenever a footnote reference is found. You'd need to use a map.

Indeed, I'll update to this.

critiqjo · 2017-03-30T09:57:34Z

src/librustdoc/html/markdown.rs

+                             id);
+                    let cur_len = parser.footnotes.len() + 1;
+                    parser.footnotes.push(format!("<li id=\"ref{}\">{}<a href=\"#supref{0}\" \
+                                                   rev=\"footnote\">↩</a></li>",


Where is parent ul generated?

At the end.

critiqjo · 2017-03-31T09:50:06Z

src/librustdoc/html/markdown.rs

+                                                     &content[..content.len() - 4]
+                                                 } else {
+                                                     &content
+                                                 }));


There's one more issue here, though it would be very rarely encountered, if at all. When there are multiple footnote references, there needs to be multiple backreferences from the definition too, to be 100% proper. (For a nice reference implementation, see Wikipedia.)

Well, for now let's keep it aside. This PR is already big enough.

(But it's a great suggestion!)

Yeah, the current footnote implementation does this. I think @GuillaumeGomez is right though, we can add that back in in another PR; it's not as big of a deal as this stuff is.

GuillaumeGomez · 2017-03-31T18:09:57Z

cc @alexcrichton @brson

steveklabnik

I think this looks good. Let's see what @frewsxcv says.

steveklabnik · 2017-03-31T18:22:50Z

src/librustdoc/html/markdown.rs

@@ -115,15 +116,19 @@ macro_rules! event_loop_break {
            match event {
                $($end_event)|* => break,
                Event::Text(ref s) => {
+                    debug!("Text");


was this meant to be left in?

I'm not opposed to keeping the debugging statements, but it'd be nicer if there were a little more descriptive, maybe something like debug!("markdown parser encountered 'Text'"), whatever works though.

steveklabnik · 2017-03-31T18:23:08Z

src/librustdoc/html/markdown.rs

-                Event::SoftBreak | Event::HardBreak if !$buf.is_empty() => {
-                    $buf.push(' ');
+                Event::SoftBreak => {
+                    debug!("SoftBreak");


steveklabnik · 2017-03-31T18:23:55Z

src/librustdoc/html/markdown.rs

 pub fn render(w: &mut fmt::Formatter,
              s: &str,
              print_toc: bool,
              shorter: MarkdownOutputStyle) -> fmt::Result {
-    fn code_block(parser: &mut Parser, buffer: &mut String, lang: &str) {
+    fn code_block(parser: &mut ParserWrapper, buffer: &mut String, lang: &str) {
+        debug!("CodeBlock");


I won't comment on all of these 😄

steveklabnik · 2017-03-31T18:25:18Z

src/librustdoc/html/markdown.rs

+                                                     &content[..content.len() - 4]
+                                                 } else {
+                                                     &content
+                                                 }));


Yeah, the current footnote implementation does this. I think @GuillaumeGomez is right though, we can add that back in in another PR; it's not as big of a deal as this stuff is.

steveklabnik · 2017-03-31T18:26:44Z

src/test/rustdoc/check-hard-break.rs

+
+// @has foo/fn.f.html
+// @has - '<p>hard break:<br>after hard break</p>'
+/// hard break:  


just a review note; this works because there are the two trailing spaces. I was confused at first 😄

That's the point of hard break. :p

Yeah, I just meant it's hard to see in this diff.

GuillaumeGomez · 2017-04-01T15:14:51Z

This PR is now ready.

frewsxcv · 2017-04-01T16:23:55Z

Looks great! r=me

Not going to r+ right now in case you want to change/remove the debug logging statements.

GuillaumeGomez · 2017-04-02T16:13:43Z

As I said to @steveklabnik, it'll certainly be helpful in a close future. Once this is all done, we can remove them.

@bors: r+

bors · 2017-04-02T16:13:44Z

📌 Commit ef01ae7 has been approved by GuillaumeGomez

frewsxcv · 2017-04-02T16:19:26Z

@bors r=frewsxcv,steveklabnik

bors · 2017-04-02T16:19:27Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: Add a note about overflow for fetch_add/fetch_sub #40927

bors · 2017-04-02T16:19:27Z

📌 Commit ef01ae7 has been approved by frewsxcv,steveklabnik

bors · 2017-04-02T18:16:14Z

⌛ Testing commit ef01ae7 with merge 72ecd79...

…veklabnik Add support for image, rules and footnotes Part of #40912. r? @rust-lang/docs PS: the footnotes are waiting for pulldown-cmark/pulldown-cmark#21 to be merged to be fully working.

bors · 2017-04-03T00:15:47Z

☀️ Test successful - status-appveyor, status-travis
Approved by: frewsxcv,steveklabnik
Pushing 72ecd79 to master...

Add support for image, rules and footnotes

08a741e

critiqjo reviewed Mar 30, 2017

View reviewed changes

Fix multiple footnotes and improve testing

36b15f0

critiqjo reviewed Mar 31, 2017

View reviewed changes

GuillaumeGomez added 2 commits March 31, 2017 12:16

Fix hard break issue

51d3cec

Add end whitespace ignore flag for tidy

4de4a95

GuillaumeGomez force-pushed the fix-new-rustdoc branch from 4911564 to 4de4a95 Compare March 31, 2017 18:16

steveklabnik reviewed Mar 31, 2017

View reviewed changes

steveklabnik mentioned this pull request Mar 31, 2017

Tracking issue for hoedown -> pulldown regressions #40912

Closed

13 tasks

Force footnote references to be sorted by id

ef01ae7

bors merged commit ef01ae7 into rust-lang:master Apr 3, 2017

GuillaumeGomez deleted the fix-new-rustdoc branch April 3, 2017 14:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for image, rules and footnotes #40919

Add support for image, rules and footnotes #40919

GuillaumeGomez commented Mar 30, 2017

critiqjo Mar 30, 2017

GuillaumeGomez Mar 31, 2017

GuillaumeGomez Mar 31, 2017

critiqjo left a comment •

edited

Loading

critiqjo Mar 30, 2017

GuillaumeGomez Mar 30, 2017

critiqjo Mar 30, 2017

GuillaumeGomez Mar 30, 2017

critiqjo Mar 30, 2017

GuillaumeGomez Mar 30, 2017

critiqjo Mar 31, 2017

GuillaumeGomez Mar 31, 2017

GuillaumeGomez Mar 31, 2017

steveklabnik Mar 31, 2017

GuillaumeGomez commented Mar 31, 2017

steveklabnik left a comment

steveklabnik Mar 31, 2017

frewsxcv Apr 1, 2017

steveklabnik Mar 31, 2017

steveklabnik Mar 31, 2017

steveklabnik Mar 31, 2017

steveklabnik Mar 31, 2017

GuillaumeGomez Mar 31, 2017

steveklabnik Mar 31, 2017

GuillaumeGomez commented Apr 1, 2017

frewsxcv commented Apr 1, 2017

GuillaumeGomez commented Apr 2, 2017

bors commented Apr 2, 2017

frewsxcv commented Apr 2, 2017

bors commented Apr 2, 2017

bors commented Apr 2, 2017

bors commented Apr 2, 2017

bors commented Apr 3, 2017

Add support for image, rules and footnotes #40919

Add support for image, rules and footnotes #40919

Conversation

GuillaumeGomez commented Mar 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

critiqjo left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Mar 31, 2017

steveklabnik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Apr 1, 2017

frewsxcv commented Apr 1, 2017

GuillaumeGomez commented Apr 2, 2017

bors commented Apr 2, 2017

frewsxcv commented Apr 2, 2017

bors commented Apr 2, 2017

bors commented Apr 2, 2017

bors commented Apr 2, 2017

bors commented Apr 3, 2017

critiqjo left a comment •

edited

Loading