code review 5303044: godoc: generate package toc in template, not in JavaScript

godoc: generate package toc in template, not in JavaScript

1. Generate TOC for package pages using template,
instead of using JavaScript magic.  This makes the
pages generated by godoc -html easier to export
to other systems.

2. Make TOC one column.  It's hard to do two columns
portably without invoking JavaScript.

3. Since the TOC is only one column, show full type
signatures for functions and methods.  Many times
that's all you need to see anyway.

4. Name the section after the TOC "Overview".
Naming it something is important, to set it off
from the TOC and so that there's a quick link to
it in the TOC.

For now, some illustrative examples:

http://swtch.com:6060/pkg/io/
http://swtch.com:6060/pkg/strings/
http://swtch.com:6060/pkg/tabwriter/
http://swtch.com:6060/pkg/unicode/

Fixes issue 1982.

[rsc, 2011-10-18 18:39:29]

Hello gri (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[bradfitz, 2011-10-18 18:58:49]

I like generating it server-side and not jumping around, and I think I could
like the full signatures, but I'm not a fan of the typography as-is.  It
looks like a big block of text.

On my 30" monitor, the block of text continues well past the "fold":
http://swtch.com:6060/pkg/http/

Looks so much lighter at http://golang.org/pkg/http/ and the start of the
page is above the fold too.


On Tue, Oct 18, 2011 at 11:39 AM, <rsc@golang.org> wrote:

> Reviewers: gri,
>
> Message:
> Hello gri (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> PROPOSAL: godoc: generate package toc manually
>
> 1. Generate TOC for package pages using template,
> instead of using JavaScript magic.  This makes the
> pages generated by godoc -html easier to export
> to other systems.
>
> 2. Make TOC one column.  It's hard to do two columns
> portably without invoking JavaScript.
>
> 3. Since the TOC is only one column, show full type
> signatures for functions and methods.  Many times
> that's all you need to see anyway.
>
> 4. Name the section after the TOC "Overview".
> Naming it something is important, to set it off
> from the TOC and so that there's a quick link to
> it in the TOC.  Happy to change the name.
>
> For now, some illustrative examples:
>
> http://swtch.com:6060/pkg/io/
> http://swtch.com:6060/pkg/**strings/ <http://swtch.com:6060/pkg/strings/>
> http://swtch.com:6060/pkg/**tabwriter/<http://swtch.com:6060/pkg/tabwriter/>
> http://swtch.com:6060/pkg/**unicode/ <http://swtch.com:6060/pkg/unicode/>
>
> Would fix issue 1982.
>
> Please review this at
http://codereview.appspot.com/**5303044/<http://codereview.appspot.com/5303044/>
>
> Affected files:
>  M doc/godocs.js
>  M lib/godoc/package.html
>
>
> Index: doc/godocs.js
> ==============================**==============================**=======
> --- a/doc/godocs.js
> +++ b/doc/godocs.js
> @@ -57,6 +57,7 @@
>  * links.  "Decorates" the element with id=="nav" with this table of
> contents.
>  */
>  function godocs_generateTOC() {
> +  if (document.getElementById('**manual-nav')) { return; }
>   var navbar = document.getElementById('nav')**;
>   if (!navbar) { return; }
>
> Index: lib/godoc/package.html
> ==============================**==============================**=======
> --- a/lib/godoc/package.html
> +++ b/lib/godoc/package.html
> @@ -3,10 +3,51 @@
>        Use of this source code is governed by a BSD-style
>        license that can be found in the LICENSE file.
>  -->
> +<!-- Table of contents; must be named manual-nav to turn off auto nav. -->
> +<div id="manual-nav">
> +{{with .PDoc}}
> +       <dl>
> +       <dd><a href="#Overview">Overview</a><**/dd>
> +       {{with .Consts}}
> +               <dd><a href="#Constants">Constants</**a></dd>
> +       {{end}}
> +       {{with .Vars}}
> +               <dd><a href="#Variables">Variables</**a></dd>
> +       {{end}}
> +       {{with .Funcs}}
> +               {{range .}}
> +                       {{$name_html := html .Name}}
> +                       <dd><a href="#{{$name_html}}">{{node_**html .Decl
> $.FSet}}</a></dd>
> +               {{end}}
> +       {{end}}
> +       {{with .Types}}
> +               {{range .}}
> +                       {{$tname := printf "%s" .Type.Name}}
> +                       {{$tname_html := node_html .Type.Name $.FSet}}
> +                       <dd><a href="#{{$tname_html}}">type
> {{$tname_html}}</a></dd>
> +                       {{range .Factories}}
> +                               {{$name_html := html .Name}}
> +                               <dd>&nbsp; &nbsp; <a
> href="#{{$name_html}}">{{node_**html .Decl $.FSet}}</a></dd>
> +                       {{end}}
> +                       {{range .Methods}}
> +                               {{$name_html := html .Name}}
> +                               <dd>&nbsp; &nbsp; <a
> href="#{{$tname_html}}.{{$**name_html}}">{{node_html .Decl
> $.FSet}}</a></dd>
> +                       {{end}}
> +               {{end}}
> +       {{end}}
> +       {{with .Bugs}}
> +               <dd><a href="#Bugs">Bugs</a></dd>
> +       {{end}}
> +       </dl>
> +{{end}}
> +</div>
> +
> +<!-- Main page -->
>  {{with .PAst}}
>        <pre>{{node_html . $.FSet}}</pre>
>  {{end}}
>  {{with .PDoc}}
> +       <h2 id="Overview">Overview</h2>
>        <!-- PackageName is printed as title by the top-level template -->
>        {{if $.IsPkg}}
>                <p><code>import "{{html .ImportPath}}"</code></p>
>
>
>

[rsc, 2011-10-18 19:07:18]

I think package http is a bad example.
Package http is way way way too big.
A presentation that makes that clearer is not a bad thing.

[r, 2011-10-18 19:32:09]

i think this is an improvement.

http://codereview.appspot.com/5303044/diff/4001/lib/godoc/package.html
File lib/godoc/package.html (right):

http://codereview.appspot.com/5303044/diff/4001/lib/godoc/package.html#newcode23
lib/godoc/package.html:23: {{with .Types}}
this could be just {{range .Types}}
similar changes elsewhere.

[gri, 2011-10-18 19:56:20]

I am fine with this change. Presumably you don't want to have the summary in the
textual template? (not sure it's needed or even makes sense).

[rsc, 2011-10-18 20:05:06]

On Oct 18, 2011 at 15:56, gri@golang.org wrote:
> I am fine with this change. Presumably you don't want to have the
> summary in the textual template? (not sure it's needed or even makes
> sense).

I think that's okay (no TOC in text output), since
on the command line people do 'godoc strings Split'
if they know what they're looking for.

I will remove the PROPOSAL part, apply Rob's fixes,
and resend.

[rsc, 2011-10-18 20:08:36]

Hello gri@golang.org, bradfitz@golang.org, r@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[gri, 2011-10-18 20:16:40]

LGTM

http://codereview.appspot.com/5303044/diff/3/lib/godoc/package.html
File lib/godoc/package.html (right):

http://codereview.appspot.com/5303044/diff/3/lib/godoc/package.html#newcode11
lib/godoc/package.html:11: {{with .Consts}}
this "with" is not needed

http://codereview.appspot.com/5303044/diff/3/lib/godoc/package.html#newcode14
lib/godoc/package.html:14: {{with .Vars}}
same here

http://codereview.appspot.com/5303044/diff/3/lib/godoc/package.html#newcode34
lib/godoc/package.html:34: {{with .Bugs}}
and here

[rsc, 2011-10-18 20:19:14]

> http://codereview.appspot.com/5303044/diff/3/lib/godoc/package.html#newcode11
> lib/godoc/package.html:11: {{with .Consts}}
> this "with" is not needed
> 
> http://codereview.appspot.com/5303044/diff/3/lib/godoc/package.html#newcode14
> lib/godoc/package.html:14: {{with .Vars}}
> same here
> 
> http://codereview.appspot.com/5303044/diff/3/lib/godoc/package.html#newcode34
> lib/godoc/package.html:34: {{with .Bugs}}
> and here

I think those {{with}} are needed.  They serve as if statements.
I forgot there was an {{if}}.  Will change to that.

[rsc, 2011-10-18 20:20:13]

Hello gri@golang.org, bradfitz@golang.org, r@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[gri, 2011-10-18 20:20:50]

LGTM

[rsc, 2011-10-18 20:23:38]

*** Submitted as http://code.google.com/p/go/source/detail?r=eec8e79200f0 ***

godoc: generate package toc in template, not in JavaScript

1. Generate TOC for package pages using template,
instead of using JavaScript magic.  This makes the
pages generated by godoc -html easier to export
to other systems.

2. Make TOC one column.  It's hard to do two columns
portably without invoking JavaScript.

3. Since the TOC is only one column, show full type
signatures for functions and methods.  Many times
that's all you need to see anyway.

4. Name the section after the TOC "Overview".
Naming it something is important, to set it off
from the TOC and so that there's a quick link to
it in the TOC.

For now, some illustrative examples:

http://swtch.com:6060/pkg/io/
http://swtch.com:6060/pkg/strings/
http://swtch.com:6060/pkg/tabwriter/
http://swtch.com:6060/pkg/unicode/

Fixes issue 1982.

R=gri, bradfitz, r
CC=golang-dev
http://codereview.appspot.com/5303044

[adg, 2011-10-18 21:12:50]

I like it.

I think the identifier names in the list should be bold or some other
color, or the surrounding text should be de-emphasized somehow.
It's all useful information, but more often than not I'm just looking
for a name.

On 19 October 2011 05:39,  <rsc@golang.org> wrote:
> Reviewers: gri,
>
> Message:
> Hello gri (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> PROPOSAL: godoc: generate package toc manually
>
> 1. Generate TOC for package pages using template,
> instead of using JavaScript magic.  This makes the
> pages generated by godoc -html easier to export
> to other systems.
>
> 2. Make TOC one column.  It's hard to do two columns
> portably without invoking JavaScript.
>
> 3. Since the TOC is only one column, show full type
> signatures for functions and methods.  Many times
> that's all you need to see anyway.
>
> 4. Name the section after the TOC "Overview".
> Naming it something is important, to set it off
> from the TOC and so that there's a quick link to
> it in the TOC.  Happy to change the name.
>
> For now, some illustrative examples:
>
> http://swtch.com:6060/pkg/io/
> http://swtch.com:6060/pkg/strings/
> http://swtch.com:6060/pkg/tabwriter/
> http://swtch.com:6060/pkg/unicode/
>
> Would fix issue 1982.
>
> Please review this at http://codereview.appspot.com/5303044/
>
> Affected files:
>  M doc/godocs.js
>  M lib/godoc/package.html
>
>
> Index: doc/godocs.js
> ===================================================================
> --- a/doc/godocs.js
> +++ b/doc/godocs.js
> @@ -57,6 +57,7 @@
>  * links.  "Decorates" the element with id=="nav" with this table of
> contents.
>  */
>  function godocs_generateTOC() {
> +  if (document.getElementById('manual-nav')) { return; }
>   var navbar = document.getElementById('nav');
>   if (!navbar) { return; }
>
> Index: lib/godoc/package.html
> ===================================================================
> --- a/lib/godoc/package.html
> +++ b/lib/godoc/package.html
> @@ -3,10 +3,51 @@
>        Use of this source code is governed by a BSD-style
>        license that can be found in the LICENSE file.
>  -->
> +<!-- Table of contents; must be named manual-nav to turn off auto nav. -->
> +<div id="manual-nav">
> +{{with .PDoc}}
> +       <dl>
> +       <dd><a href="#Overview">Overview</a></dd>
> +       {{with .Consts}}
> +               <dd><a href="#Constants">Constants</a></dd>
> +       {{end}}
> +       {{with .Vars}}
> +               <dd><a href="#Variables">Variables</a></dd>
> +       {{end}}
> +       {{with .Funcs}}
> +               {{range .}}
> +                       {{$name_html := html .Name}}
> +                       <dd><a href="#{{$name_html}}">{{node_html .Decl
> $.FSet}}</a></dd>
> +               {{end}}
> +       {{end}}
> +       {{with .Types}}
> +               {{range .}}
> +                       {{$tname := printf "%s" .Type.Name}}
> +                       {{$tname_html := node_html .Type.Name $.FSet}}
> +                       <dd><a href="#{{$tname_html}}">type
> {{$tname_html}}</a></dd>
> +                       {{range .Factories}}
> +                               {{$name_html := html .Name}}
> +                               <dd>&nbsp; &nbsp; <a
> href="#{{$name_html}}">{{node_html .Decl $.FSet}}</a></dd>
> +                       {{end}}
> +                       {{range .Methods}}
> +                               {{$name_html := html .Name}}
> +                               <dd>&nbsp; &nbsp; <a
> href="#{{$tname_html}}.{{$name_html}}">{{node_html .Decl $.FSet}}</a></dd>
> +                       {{end}}
> +               {{end}}
> +       {{end}}
> +       {{with .Bugs}}
> +               <dd><a href="#Bugs">Bugs</a></dd>
> +       {{end}}
> +       </dl>
> +{{end}}
> +</div>
> +
> +<!-- Main page -->
>  {{with .PAst}}
>        <pre>{{node_html . $.FSet}}</pre>
>  {{end}}
>  {{with .PDoc}}
> +       <h2 id="Overview">Overview</h2>
>        <!-- PackageName is printed as title by the top-level template -->
>        {{if $.IsPkg}}
>                <p><code>import "{{html .ImportPath}}"</code></p>
>
>
>

[rsc, 2011-10-18 21:42:38]

> I think the identifier names in the list should be bold or some other
> color, or the surrounding text should be de-emphasized somehow.
> It's all useful information, but more often than not I'm just looking
> for a name.

I am skeptical.  They're all lined up, at least in chunks,
so it's easy to scan.  Syntax highlighting and all that.

Russ

[adg, 2011-10-18 22:02:26]

I'm working on a CL.

On 19 October 2011 08:42, Russ Cox <rsc@golang.org> wrote:
>> I think the identifier names in the list should be bold or some other
>> color, or the surrounding text should be de-emphasized somehow.
>> It's all useful information, but more often than not I'm just looking
>> for a name.
>
> I am skeptical.  They're all lined up, at least in chunks,
> so it's easy to scan.  Syntax highlighting and all that.
>
> Russ
>