[PATCH V2] hgweb: add line wrapping switch to file source view

[Laurens Holst]

Op 14-07-13 00:07, Alexander Plavin schreef:
> # HG changeset patch
> # User Alexander Plavin <me at aplavin.ru>
> # Date 1373630293 -14400
> #      Fri Jul 12 15:58:13 2013 +0400
> # Node ID ff3a665349adccb190786a81bdb201efce895b1d
> # Parent  6c49903f66be1a7185d54e1e0df5419d64238e11
> hgweb: add line wrapping switch to file source view
>
> diff -r 6c49903f66be -r ff3a665349ad mercurial/templates/paper/filerevision.tmpl
> --- a/mercurial/templates/paper/filerevision.tmpl	Fri Jul 12 16:01:11 2013 +0400
> +++ b/mercurial/templates/paper/filerevision.tmpl	Fri Jul 12 15:58:13 2013 +0400
> @@ -67,8 +67,9 @@
>   </table>
>   
>   <div class="overflow">
> +<div class="sourcefirst linewraptoggle">line wrap: <a class="linewraplink" href="javascript:toggleLinewrap()">on</a></div>
>   <div class="sourcefirst"> line source</div>
> -<pre class="sourcelines">{text%fileline}</pre>
> +<pre class="sourcelines wrap">{text%fileline}</pre>
>   <div class="sourcelast"></div>
>   </div>
>   </div>
> diff -r 6c49903f66be -r ff3a665349ad mercurial/templates/static/mercurial.js
> --- a/mercurial/templates/static/mercurial.js	Fri Jul 12 16:01:11 2013 +0400
> +++ b/mercurial/templates/static/mercurial.js	Fri Jul 12 15:58:13 2013 +0400
> @@ -271,3 +271,21 @@
>       document.getElementById('diffstatdetails').style.display = curexpand;
>       document.getElementById('diffstatexpand').style.display = curdetails;
>   }
> +
> +function toggleLinewrap() {
> +    var new_flag;

Initialise new_flag to some value; if by chance the loop has 0 items, 
you will be operating on undefined.

Regardless of whether it can happen in practice right now, it’s a matter 
of robustness; it’s an external factor that is not under control by this 
function, and someone might use the function in some place or alter the 
template without realising the function would need to be adjusted as well.

> +    var nodes = document.querySelectorAll('.sourcelines');
> +    for (var i = 0; i < nodes.length; i++) {
> +        new_flag = !nodes[i].classList.contains('wrap');
> +        if (new_flag) {
> +            nodes[i].classList.add('wrap');
> +        } else {
> +            nodes[i].classList.remove('wrap');
> +        }
> +    }
> +
> +    var links = document.getElementsByClassName('linewraplink');

Why do you use getElementsByClassName() here but querySelectorAll() 
before? Seems inconsistent.

> +    for (var i = 0; i < links.length; i++) {
> +        links[i].innerHTML = new_flag ? 'on' : 'off';
> +    }
> +}

I don’t like how you overwrite new_flag every time you loop over the 
item. Theoretically, if there are two sourcelines sections you could end 
up in a situation where the one is toggled on and the other is toggled off.

I would split this up into three functions:

1. getLinewrap()
2. setLinewrap(enable)
3. toggleLinewrap()

Where the latter invokes the former two.

function toggleLinewrap() {
setLinewrap(!getLinewrap());
}

That way you always just have one state. (You could pass the for the 
sake of optimisation.)

p.s. Is classList supported by all browsers? It didn’t even exist just a 
couple of years ago...

~Laurens

> diff -r 6c49903f66be -r ff3a665349ad mercurial/templates/static/style-paper.css
> --- a/mercurial/templates/static/style-paper.css	Fri Jul 12 16:01:11 2013 +0400
> +++ b/mercurial/templates/static/style-paper.css	Fri Jul 12 15:58:13 2013 +0400
> @@ -214,11 +214,18 @@
>     position: relative;
>   }
>   
> +.wrap > span {
> +    white-space: pre-wrap;
> +}
> +
> +.linewraptoggle {
> +    float: right;
> +}
> +
>   .sourcelines > span {
>     display: inline-block;
>     width: 100%;
>     padding: 1px 0px;
> -  white-space: pre-wrap;
>     counter-increment: lineno;
>   }
>   
> diff -r 6c49903f66be -r ff3a665349ad tests/test-hgweb-commands.t
> --- a/tests/test-hgweb-commands.t	Fri Jul 12 16:01:11 2013 +0400
> +++ b/tests/test-hgweb-commands.t	Fri Jul 12 15:58:13 2013 +0400
> @@ -667,8 +667,9 @@
>     </table>
>     
>     <div class="overflow">
> +  <div class="sourcefirst linewraptoggle">line wrap: <a class="linewraplink" href="javascript:toggleLinewrap()">on</a></div>
>     <div class="sourcefirst"> line source</div>
> -  <pre class="sourcelines">
> +  <pre class="sourcelines wrap">
>     <span id="l1">foo</span><a href="#l1"></a></pre>
>     <div class="sourcelast"></div>
>     </div>
> diff -r 6c49903f66be -r ff3a665349ad tests/test-highlight.t
> --- a/tests/test-highlight.t	Fri Jul 12 16:01:11 2013 +0400
> +++ b/tests/test-highlight.t	Fri Jul 12 15:58:13 2013 +0400
> @@ -136,8 +136,9 @@
>     </table>
>     
>     <div class="overflow">
> +  <div class="sourcefirst linewraptoggle">line wrap: <a class="linewraplink" href="javascript:toggleLinewrap()">on</a></div>
>     <div class="sourcefirst"> line source</div>
> -  <pre class="sourcelines">
> +  <pre class="sourcelines wrap">
>     <span id="l1"><span class="c">#!/usr/bin/env python</span></span><a href="#l1"></a>
>     <span id="l2"></span><a href="#l2"></a>
>     <span id="l3"><span class="sd">"""Fun with generators. Corresponding Haskell implementation:</span></span><a href="#l3"></a>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
>