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

login
register
mail settings
Submitter Alexander Plavin
Date July 13, 2013, 10:07 p.m.
Message ID <ff3a665349adccb19078.1373753275@debian-alexander.dolgopa>
Download mbox | patch
Permalink /patch/1874/
State Superseded, archived
Headers show

Comments

Alexander Plavin - July 13, 2013, 10:07 p.m.
# HG changeset patch
# User Alexander Plavin <me@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
Laurens Holst - July 15, 2013, 10:02 a.m.
Op 14-07-13 00:07, Alexander Plavin schreef:
> # HG changeset patch
> # User Alexander Plavin <me@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">&quot;&quot;&quot;Fun with generators. Corresponding Haskell implementation:</span></span><a href="#l3"></a>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
>
Alexander Plavin - July 15, 2013, 2:11 p.m.
2013/7/15 Laurens Holst <laurens.nospam@grauw.nl>:
> Op 14-07-13 00:07, Alexander Plavin schreef:
>
>> # HG changeset patch
>> # User Alexander Plavin <me@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.
>

Probably we shouldn't do anything if there are no items to loop through, but ok.

>
>> +    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.

Yes, there was something different in the first selection, then I
edited it and forgot to change the function called.

>
>
>> +    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.

As (now) there can be only one switch on the page (not one per
section), I assumed that all the sections have the same value of this,
but your next suggestion looks better.

>
> 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.)

I like this solution. Through getLinewrap still needs to return
something if there are differences between sections, at least this
changes everything consistently.

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

Of course not by all, but both Chromium 8.0+, FireFox 3.6+ and Opera
11.5+ support it, as well as relatively modern versions of other
browsers, including mobile ones.

>
> ~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">&quot;&quot;&quot;Fun with generators.
>> Corresponding Haskell implementation:</span></span><a href="#l3"></a>
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@selenic.com
>> http://selenic.com/mailman/listinfo/mercurial-devel
>>
>

Patch

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;
+    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');
+    for (var i = 0; i < links.length; i++) {
+        links[i].innerHTML = new_flag ? 'on' : 'off';
+    }
+}
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">&quot;&quot;&quot;Fun with generators. Corresponding Haskell implementation:</span></span><a href="#l3"></a>