618146 - Syntax highlighting for SDK docs

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: Documentation, defect)

Description

[Will Bamberg [:wbamberg]]

It would help readability if we implemented syntax highlighting for code samples in the SDK.

Comment 1

[Will Bamberg [:wbamberg]]

Attachment: Links to Git branch containing syntax highlighting

I tried implementing syntax highlighting for the code examples in the SDK. It uses Google's syntax highlighter (http://code.google.com/p/syntaxhighlighter/) and will try to highlight any <pre><code> blocks.

I copied the license (Apache license) to the same place as the JS and CSS files themselves.

I've gone through any Markdown-style code blocks that aren't actual code samples and turned them into explicit <pre> blocks, so they don't get highlighted too.

If you like this approach, let me know, and I'll make a pull request.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 496669 [details]
Links to Git branch containing syntax highlighting

I like this approach!  And I think the Apache 2.0 license is compatible with our tri-license.  But I've emailed the Mozilla licensing folks to confirm that.


In the process of giving feedback, I ended up doing a review of the code, since I was reading through it anyway, and I noticed one issue:

> +    $("pre>code").addClass("prettyprint");

This adds class "prettyprint" to <code> elements that are children of <pre> elements.  But prettify.css expects the class to be placed on the <pre> elements themselves:

> +pre.prettyprint { padding: 2px; border: 1px solid #888 }

We could modify prettify.css to style code.prettyprint instead, but that makes it harder to merge upstream changes, so we should probably do something like modify our HTML generation to do <pre class="code"> instead of <pre><code> and then $("pre.code").addClass("prettyprint").

(Ideally, we'd be able to select <pre> elements with a <code> child, but though it's possible in XPath, as far as I know it isn't possible in CSS and hence also not in jQuery.)


> +.str { color: #080; }

I wish these were restricted to pretty-printed sections, i.e.:

  pre.prettyprint .str { color: #080; }

But it's probably better to leave them as-is to make it easier to merge from upstream, especially since they probably don't trod on any existing or planned styles.  Or perhaps we could push this change upstream (provided it doesn't incur a significant performance cost).

Comment 3

[Will Bamberg [:wbamberg]]

Thanks Myk!

(In reply to comment #2)
> 
> In the process of giving feedback, I ended up doing a review of the code, since
> I was reading through it anyway, and I noticed one issue:
> 
> > +    $("pre>code").addClass("prettyprint");
> 
> This adds class "prettyprint" to <code> elements that are children of <pre>
> elements.  But prettify.css expects the class to be placed on the <pre>
> elements themselves:
> 
> > +pre.prettyprint { padding: 2px; border: 1px solid #888 }

True. It does mostly work when the class is on <code> but you don't get the border.

> We could modify prettify.css to style code.prettyprint instead, but that makes
> it harder to merge upstream changes, so we should probably do something like
> modify our HTML generation to do <pre class="code"> instead of <pre><code> and
> then $("pre.code").addClass("prettyprint").

Similarly, though: our HTML generation is currently using the Markdown conveter and I'd prefer not to add custom behaviour there either.

> (Ideally, we'd be able to select <pre> elements with a <code> child, but though
> it's possible in XPath, as far as I know it isn't possible in CSS and hence
> also not in jQuery.)

I'm not an expert on JQuery by any stretch, but it looks as if:

$("code").parent("pre").addClass("prettyprint");

works here... or am I missing something?

> > +.str { color: #080; }
> 
> I wish these were restricted to pretty-printed sections, i.e.:
> 
>   pre.prettyprint .str { color: #080; }
> 
> But it's probably better to leave them as-is to make it easier to merge from
> upstream, especially since they probably don't trod on any existing or planned
> styles.  Or perhaps we could push this change upstream (provided it doesn't
> incur a significant performance cost).

Good point. I think it's worth raising an issue with the prettify maintainer for this, and will do that.

Comment 4

[Will Bamberg [:wbamberg]]

By the way, the link in comment 1 is wrong, it should be: http://code.google.com/p/google-code-prettify/ . Sorry. Everything else is right, including the Apache license.

Comment 5

[Will Bamberg [:wbamberg]]

Attachment: Updated patch using syntaxhighlighter instead of prettify

I've updated the code to use syntaxhighlighter (http://alexgorbatchev.com/SyntaxHighlighter/) instead, which is available under the MIT license and is, I think, already used in the MDN. I think it looks nicer too, and has support for themes if we care about that.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 499353 [details] [diff] [review]
Updated patch using syntaxhighlighter instead of prettify

(In reply to comment #3)
> I'm not an expert on JQuery by any stretch, but it looks as if:
> 
> $("code").parent("pre").addClass("prettyprint");
> 
> works here... or am I missing something?

Nope, you're not missing anything.  That was my mistake.  It's awesome that jQuery can do this!  (I talked to dbaron about adding it to the CSS selector syntax at one point, and apparently it has tricky performance implications.)


> +  function highlight() {
> +    $("code").parent("pre").addClass("brush: js");

Hunh, what a strange class name.  Technically, this adds two classes, "brush:" and "js", since HTML parsers split class attributes on whitespace (although they also make the entire string available, which must be how syntaxhighlighter accesses it).


> +<pre>
>      python --version
> +</pre>
>  
>  `cfx` currently expects Python 2.5 or 2.6.  Older and newer versions may or may
>  not work.
> @@ -88,7 +90,9 @@ Run the SDK's Unit Tests
>  The SDK comes with a suite of tests which ensures that its APIs work correctly.
>  You can run it with the following command:
>  
> +<pre>
>      cfx testall
> +</pre>
>  
>  Some of the tests will open Firefox windows to check APIs related to the user
>  interface, so don't be alarmed.  Please let the suite finish before resuming
> @@ -97,11 +101,12 @@ your work.
>  When the suite is finished, your text console should contain output that looks
>  something like this:
>  
> +<pre>
>      Testing cfx...

In most cases, you change the indent for explicit <pre> sections from four to two spaces, but here (and in a few other places) you don't.  Is there a reason to leave them with four-space indent?

Comment 7

[Will Bamberg [:wbamberg]]

> In most cases, you change the indent for explicit <pre> sections from four to
> two spaces, but here (and in a few other places) you don't.  Is there a reason
> to leave them with four-space indent?

No good reason. I changed it from 4 to 2 only because the reason for having 4 is to indicate to Markdown that it should use <pre><code>, and changing it to 2 seemed to me to help emphasize that we don't want <code> any more. But that in itself is a bit weak really. Anyway, I've updated it so it's consistently 2 everywhere (I think).

Fixed by: https://github.com/mozilla/addon-sdk/commit/ec07f2a5c874455edb2d44d3362af376e5bdc51e