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

Add Mathjax Support to Rustdoc #16991

Closed
wants to merge 6 commits into from
Closed

Conversation

Gankra
Copy link
Contributor

@Gankra Gankra commented Sep 4, 2014

I'm not super clear on the best way to use TLDs and #![doc] configs, but this seems to work, and isn't obviously wrong:

http://cg.scs.carleton.ca/~abeinges/doc/collections/vec/struct.Vec.html

However, the current design interacts weirdly with re-exports:

http://cg.scs.carleton.ca/~abeinges/doc/std/vec/struct.Vec.html

Detailed design:

Mathjax support will only be enabled on crates that declare #!doc[(enable_mathjax)]. When enabled mathjax can be used by wrapping math in double-dollars like so: $$ \sqrt{x} + \log^2n $$. If the dollars are part of a larger block element, the content will be rendered inline. If they surround an entire block, the math will be rendered in "display mode" as seen in my example pages. Note: the opening $$'s must be lead by whitespace, and the closing $$'s must be followed by whitespace. Therefore blah blah $$x^2$$. will not render as math, because there is no whitespace between the last $ and the ..

Closes #16300
Trivializes #15285

@alexcrichton
Copy link
Member

To help with #15285 this may want to expose a --markdown-mathjax flag on the CLI to enable mathjax rendering for markdown files as well as rust crates (and then likely enable it by default for our docs).

As part of the hoedown upgrade, it looks like some structs will also need to be updated. These are the diffs I saw between the two revs:

More struct fields:

+typedef void *(*hoedown_realloc_callback)(void *, size_t);
+typedef void (*hoedown_free_callback)(void *);
+
 /* hoedown_buffer: character array buffer */
 struct hoedown_buffer {
        uint8_t *data;  /* actual character data */
        size_t size;    /* size of the string */
        size_t asize;   /* allocated size (0 = volatile buffer) */
        size_t unit;    /* reallocation unit size (0 = read-only buffer) */
+
+       hoedown_realloc_callback data_realloc;
+       hoedown_free_callback data_free;
+       hoedown_free_callback buffer_free;
 };

One more render callback. This needs to update the count of words int he relevant struct.

@@ -119,6 +123,7 @@ struct hoedown_renderer {
        int (*strikethrough)(hoedown_buffer *ob, const hoedown_buffer *text, void *opaque);
        int (*superscript)(hoedown_buffer *ob, const hoedown_buffer *text, void *opaque);
        int (*footnote_ref)(hoedown_buffer *ob, unsigned int num, void *opaque);
+       int (*math)(hoedown_buffer *ob, const hoedown_buffer *text, int displaymode, void *opaque);

        /* low level callbacks - NULL copies input directly into the output */

@Gankra
Copy link
Contributor Author

Gankra commented Sep 4, 2014

Huh. I'm surprised this compiled/ran without updating the relevant structs. Are we invoking UB or something right now?

@alexcrichton
Copy link
Member

It could be that we're getting lucky, or that we're just not instantiating those structs directly, but I think that not faithfully representing hoedown_renderer may indeed lead to problems (I think we create one directly)

@Gankra
Copy link
Contributor Author

Gankra commented Sep 4, 2014

@alexcrichton I added more padding to correspond to the fields (that we don't seem to care about). Is that what you had in mind?

Also I'm all for having rust itself default to using mathjax everywhere, and tossing in a CLI flag for good measure. Gonna take me a while to figure out how to do that, though. I assume all that stuff is in lib.rs? Gotta add it to opts and then toss some logic in main_args, I suppose?

@alexcrichton
Copy link
Member

That'll be it! Just some munging/passing of flags.

@Gankra
Copy link
Contributor Author

Gankra commented Sep 4, 2014

Uh sure, I can try, but I'm pretty out of my element here. Is that a... pointer to a function pointer? Yuck.

Poking at CLI stuff now

@alexcrichton
Copy link
Member

You'll probably want to extend it with something like:

type hoedown_realloc_callback = extern fn(*mut c_void, size_t) -> *mut size_t;
type hoedown_free_callback = extern fn(*mut c_void);

 struct hoedown_buffer {
    // ...
    data_realloc: Option<hoedown_realloc_callback>,
    data_free: Option<hoedown_free_callback>,
    buffer_free: Option<hoedown_free_callback>,
 }

@Gankra
Copy link
Contributor Author

Gankra commented Sep 4, 2014

Wait, where did you get the -> *mut size_t part fr-- oh god it's a function that returns void*. C, why?

@alexcrichton
Copy link
Member

Given a C typedef like this:

typedef void *(*hoedown_realloc_callback)(void *, size_t);

You can visually transform it to something like:

typedef hoedown_realloc_callback = void *(*)(void *, size_t);

And then a thing about C is to basically ignore the (*), in which case you get:

typedef hoedown_realloc_callback = void *(void *, size_t);

I suspect it's returning a void* b/c it mirrors the signature of realloc

@Gankra
Copy link
Contributor Author

Gankra commented Sep 4, 2014

Yeah it wasn't so much that it was returning a void* (although the existence of void* in an otherwise statically typed language is itself lamentable), I was just lamenting how much that stray * completely changes the return type.

I fear that I'm going to accidentally learn how to actually read/write C by the end of this, @alexcrichton.

Anyway, I applied your changes. :P

Edit: working on CLI stuff

@huonw
Copy link
Member

huonw commented Sep 4, 2014

Note: the opening $$'s must be lead by whitespace, and the closing $$'s must be followed by whitespace. Therefore blah blah $$x^2$$. will not render as math, because there is no whitespace between the last $ and the ..

This is a hoedown limitation?

(Also, fwiw, I'm still interested in always enabling math parsing, and emitting an error if enable_mathjax isn't set if any math is actually found.)

@Gankra
Copy link
Contributor Author

Gankra commented Sep 4, 2014

@huonw As far as I can tell, the whitespace thing is by design, although I had to discover it by trial and error and source reading, myself. Probably to avoid "accidental" math?

I have mixed feelings on the design you're proposing, particularly because a general philosophy of markdown is "all input is valid". But more than anything I'm genuinely terrified to try to write a hook into a C library. Would you be able/willing to write the hook described in the original issue?

@Gankra
Copy link
Contributor Author

Gankra commented Sep 4, 2014

@huonw This was just logged: hoedown/hoedown#120

@huonw
Copy link
Member

huonw commented Sep 4, 2014

I suppose it could actually just be a warning.

Anyway, the basic infrastructure might be something like, AFAICT there's not a good way to emit messages about specific parts of the markdown, so I'm happy punting on that (we can also continue to conditionally enable the extension and remove the !opaque.math_enabled branch in math):

diff --git a/src/librustdoc/html/layout.rs b/src/librustdoc/html/layout.rs
index 896d070..df84b9b 100644
--- a/src/librustdoc/html/layout.rs
+++ b/src/librustdoc/html/layout.rs
@@ -12,6 +12,7 @@ use std::fmt;
 use std::io;

 use externalfiles::ExternalHtml;
+use html::markdown;

 #[deriving(Clone)]
 pub struct Layout {
@@ -34,6 +35,8 @@ pub fn render<T: fmt::Show, S: fmt::Show>(
     dst: &mut io::Writer, layout: &Layout, page: &Page, sidebar: &S, t: &T)
     -> io::IoResult<()>
 {
+    markdown::math_seen.replace(Some(false));
+
     write!(dst,
 r##"<!DOCTYPE html>
 <html lang="en">
@@ -156,6 +159,12 @@ r##"<!DOCTYPE html>
     } else {
         format!(r#"<script src="{}playpen.js"></script>"#, page.root_path)
     },
+    // this must be last so that `math_seen` captures all possible $$'s on this page.
+    mathjax_js = if layout.use_mathjax && markdown::math_seen.get().map_or(false, |x| *x) {
+        "..."
+    } else {
+        ""
+    }
     )
 }

diff --git a/src/librustdoc/html/markdown.rs b/src/librustdoc/html/markdown.rs
index 305c184..f205078 100644
--- a/src/librustdoc/html/markdown.rs
+++ b/src/librustdoc/html/markdown.rs
@@ -101,6 +101,8 @@ struct MyOpaque {
     dfltblk: extern "C" fn(*mut hoedown_buffer, *const hoedown_buffer,
                            *const hoedown_buffer, *mut libc::c_void),
     toc_builder: Option<TocBuilder>,
+    math_enabled: bool,
+    math_seen: bool,
 }

 #[repr(C)]
@@ -151,6 +153,7 @@ local_data_key!(used_header_map: RefCell<HashMap<String, uint>>)
 local_data_key!(test_idx: Cell<uint>)
 // None == render an example, but there's no crate name
 local_data_key!(pub playground_krate: Option<String>)
+local_data_key!(pub math_seen: bool)

 pub fn render(w: &mut fmt::Formatter, s: &str, print_toc: bool) -> fmt::Result {
     extern fn block(ob: *mut hoedown_buffer, text: *const hoedown_buffer,
@@ -274,16 +277,48 @@ pub fn render(w: &mut fmt::Formatter, s: &str, print_toc: bool) -> fmt::Result {
         text.with_c_str(|p| unsafe { hoedown_buffer_puts(ob, p) });
     }

+    extern fn math(ob: *mut hoedown_buffer, text: *const hoedown_buffer,
+                   display_mode: libc::c_int, opaque: *mut libc::c_void) -> libc::c_int {
+
+        let opaque = opaque as *mut hoedown_html_renderer_state;
+        let opaque = unsafe { &mut *((*opaque).opaque as *mut MyOpaque) };
+
+        opaque.math_seen = true;
+
+        let (open, close) = if !opaque.math_enabled {
+            ("$$", "$$")
+        } else if displaymode == 1 {
+            ("\\[", "\\]")
+        } else {
+            ("\\(", "\\)")
+        };
+
+        open.with_c_str(|open| {
+            close.with_c_str(|close| {
+                unsafe {
+                    hoedown_buffer_puts(ob, open, 2);
+                    escape_html(ob, (*text).data, (*text).size);
+                    hoedown_buffer_puts(ob, close, 2);
+                }
+            })
+        });
+
+        1
+    }
+
     unsafe {
         let ob = hoedown_buffer_new(DEF_OUNIT);
         let renderer = hoedown_html_renderer_new(0, 0);
         let mut opaque = MyOpaque {
             dfltblk: (*renderer).blockcode.unwrap(),
-            toc_builder: if print_toc {Some(TocBuilder::new())} else {None}
+            toc_builder: if print_toc {Some(TocBuilder::new())} else {None},
+            math_enabled: use_mathjax.get().map_or(false, |x| *x),
+            math_seen: false,
         };
         (*(*renderer).opaque).opaque = &mut opaque as *mut _ as *mut libc::c_void;
         (*renderer).blockcode = Some(block);
         (*renderer).header = Some(header);
+        (*renderer).math = Some(math);

         let document = hoedown_document_new(renderer, HOEDOWN_EXTENSIONS, 16);
         hoedown_document_render(document, ob, s.as_ptr(),
@@ -303,6 +338,10 @@ pub fn render(w: &mut fmt::Formatter, s: &str, print_toc: bool) -> fmt::Result {
             });
         }
         hoedown_buffer_free(ob);
+
+        let old = math_seen.get().map_or(false, |x| *x);
+        math_seen.replace(Some(old || opaque.math_seen));
+
         ret
     }
 }

(Not compiled.)

@Gankra
Copy link
Contributor Author

Gankra commented Sep 4, 2014

<3

I didn't expect you to just impl that all right away! so cool

swoons

@Gankra
Copy link
Contributor Author

Gankra commented Sep 4, 2014

@huonw Reviewing your code, it seems that you never reset the TLD for math_seen. What do we map tasks to? Modules? Crates? Pages? It would be unfortunate if a single use of math in an entire crate triggered the dependency on an entire crate.

Forgot about the diff in Layout.

@huonw
Copy link
Member

huonw commented Sep 4, 2014

It is reset before rendering each page in layout.

@Gankra
Copy link
Contributor Author

Gankra commented Sep 5, 2014

Hmm, I merged in your patch and fixed up the minor errors (which since I didn't really tweak all my stuff means there's some redundant stuff), but something somewhere seems to have gone wrong... The math is getting parsed/transformed correctly, but the script isn't getting included in the page. Can't work it out at the moment. Hopefully it'll make sense in the morning.

@Gankra
Copy link
Contributor Author

Gankra commented Sep 5, 2014

@alexcrichton @huonw Could you guys make a hard call on what you want the default behavior to be here?

@alexcrichton
Copy link
Member

It looks like there may actually be a markdown standard soon-ish, and the standard doesn't mention much about math formatting, so I would be tempted to leave it turned off by default because if we switch to something other than hoedown in the future it may break compatibility. Does that sounds ok @huonw?

@huonw
Copy link
Member

huonw commented Sep 8, 2014

Sounds good to me.

@steveklabnik
Copy link
Member

this needs a rebase.

@Gankra
Copy link
Contributor Author

Gankra commented Sep 15, 2014

Oh geez, I totally blanked on this PR. I should probably finish this up soon unless someone with more time wants to take the reigns.

@Gankra
Copy link
Contributor Author

Gankra commented Sep 16, 2014

As discussed in #16300 we might want to stop to consider evaluating https://github.com/Khan/KaTeX as a significantly more lightweight and performant solution than mathjax. Between this, the rebase, and all the work this still needs (command line flags and junk), I'm inclined to just close this PR. I'm a bit swamped now that the school year's back on, and I'd rather focus on the collections stuff if anything.

If anyone wants to carry the flag over the finish line, most of the stuff you need is in this branch, but there's still a fair amount of work to do. This commit: Gankra@7affc42 is probably the best one to work off of, as it produces correct results, and isn't cluttered up with the experimental math detection stuff that @huonw suggested but I wasn't able to get working quite right.

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.

Add Mathjax (KaTeX?) Support to Rustdoc
4 participants