Patchwork hgweb: add line wrapping switch with javascript

login
register
mail settings
Submitter Alexander Plavin
Date July 12, 2013, 12:07 p.m.
Message ID <161cf6af00a4187cf2eb.1373630872@debian-alexander.dolgopa>
Download mbox | patch
Permalink /patch/1841/
State Superseded, archived
Headers show

Comments

Alexander Plavin - July 12, 2013, 12: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 161cf6af00a4187cf2ebc19fe95f1b990de84133
# Parent  6187d6255fd2a14d7d21e27cd0d86ce6f282a86c
hgweb: add line wrapping switch with javascript
Martin Geisler - July 12, 2013, 1:22 p.m.
Alexander Plavin <me@aplavin.ru> writes:

> # HG changeset patch
> # User Alexander Plavin <me@aplavin.ru>
> # Date 1373630293 -14400
> #      Fri Jul 12 15:58:13 2013 +0400
> # Node ID 161cf6af00a4187cf2ebc19fe95f1b990de84133
> # Parent  6187d6255fd2a14d7d21e27cd0d86ce6f282a86c
> hgweb: add line wrapping switch with javascript
>
> diff -r 6187d6255fd2 -r 161cf6af00a4 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,6 +67,7 @@
>  </table>
>  
>  <div class="overflow">
> +<div class="sourcefirst" style="float: right;">line wrap: <a class="linewraplink" href="javascript:setLinewrap('off')">on</a></div>
>  <div class="sourcefirst"> line source</div>
>  <pre class="sourcelines">{text%fileline}</pre>
>  <div class="sourcelast"></div>
> diff -r 6187d6255fd2 -r 161cf6af00a4 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
> @@ -269,3 +269,16 @@
>  	document.getElementById('diffstatdetails').style.display = flag ? 'inline' : 'none';
>  	document.getElementById('diffstatexpand').style.display = flag ? 'none' : 'inline';
>  }
> +
> +function setLinewrap(flag) {
> +    var nodes = document.querySelectorAll('.sourcelines > span');
> +    for (var i = 0; i < nodes.length; i++) {
> +        nodes[i].style.whiteSpace = flag == 'on' ? 'pre-wrap' : 'pre';
> +    }

Would it not be better to set a class (such as "wrap") on the pre
element and then add

  pre.wrap span {
      white-spare: pre-wrap;
  }

to the stylesheet? That way you don't need to run through a potentially
huge number of lines and we can collect the style information in the
stylesheet.

> +    var links = document.getElementsByClassName('linewraplink');

I would prefer something like

       var not_flag = flag == 'on' ? 'off' : 'on';
> +    for (var i = 0; i < links.length; i++) {
           links[i].innerHTML = flag;
           links[i].href = 'javascript:setLinewrap("' + not_flag + '");'
> +    }
> +}

Alternatively, and probably better: avoid the flag parameter and let the
DOM keep the state instead. That is, if the "wrap" class is there, then
remove it and update the innerHTML appropriatedly -- there is then no
need to update the link since toggleLinewrap (new name for the function)
would work without any arguments.
Alexander Plavin - July 12, 2013, 2:22 p.m.
2013/7/12 Martin Geisler <martin@geisler.net>:
> Alexander Plavin <me@aplavin.ru> writes:
>
>> # HG changeset patch
>> # User Alexander Plavin <me@aplavin.ru>
>> # Date 1373630293 -14400
>> #      Fri Jul 12 15:58:13 2013 +0400
>> # Node ID 161cf6af00a4187cf2ebc19fe95f1b990de84133
>> # Parent  6187d6255fd2a14d7d21e27cd0d86ce6f282a86c
>> hgweb: add line wrapping switch with javascript
>>
>> diff -r 6187d6255fd2 -r 161cf6af00a4 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,6 +67,7 @@
>>  </table>
>>
>>  <div class="overflow">
>> +<div class="sourcefirst" style="float: right;">line wrap: <a class="linewraplink" href="javascript:setLinewrap('off')">on</a></div>
>>  <div class="sourcefirst"> line source</div>
>>  <pre class="sourcelines">{text%fileline}</pre>
>>  <div class="sourcelast"></div>
>> diff -r 6187d6255fd2 -r 161cf6af00a4 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
>> @@ -269,3 +269,16 @@
>>       document.getElementById('diffstatdetails').style.display = flag ? 'inline' : 'none';
>>       document.getElementById('diffstatexpand').style.display = flag ? 'none' : 'inline';
>>  }
>> +
>> +function setLinewrap(flag) {
>> +    var nodes = document.querySelectorAll('.sourcelines > span');
>> +    for (var i = 0; i < nodes.length; i++) {
>> +        nodes[i].style.whiteSpace = flag == 'on' ? 'pre-wrap' : 'pre';
>> +    }
>
> Would it not be better to set a class (such as "wrap") on the pre
> element and then add
>
>   pre.wrap span {
>       white-spare: pre-wrap;
>   }
>
> to the stylesheet? That way you don't need to run through a potentially
> huge number of lines and we can collect the style information in the
> stylesheet.

Agree, changed this.

>
>> +    var links = document.getElementsByClassName('linewraplink');
>
> I would prefer something like
>
>        var not_flag = flag == 'on' ? 'off' : 'on';
>> +    for (var i = 0; i < links.length; i++) {
>            links[i].innerHTML = flag;
>            links[i].href = 'javascript:setLinewrap("' + not_flag + '");'
>> +    }
>> +}
>
> Alternatively, and probably better: avoid the flag parameter and let the
> DOM keep the state instead. That is, if the "wrap" class is there, then
> remove it and update the innerHTML appropriatedly -- there is then no
> need to update the link since toggleLinewrap (new name for the function)
> would work without any arguments.

Yes, this really looks better :)

Thanks for your comments here. Sent a new version considering these suggestions.

>
> --
> Martin Geisler
Martin Geisler - July 15, 2013, 7:28 a.m.
Alexander Plavin <me@aplavin.ru> writes:

> 2013/7/12 Martin Geisler <martin@geisler.net>:
>> Alexander Plavin <me@aplavin.ru> writes:
>>
>>> +    var links = document.getElementsByClassName('linewraplink');
>>
>> I would prefer something like
>>
>>        var not_flag = flag == 'on' ? 'off' : 'on';
>>> +    for (var i = 0; i < links.length; i++) {
>>            links[i].innerHTML = flag;
>>            links[i].href = 'javascript:setLinewrap("' + not_flag + '");'
>>> +    }
>>> +}
>>
>> Alternatively, and probably better: avoid the flag parameter and let
>> the DOM keep the state instead. That is, if the "wrap" class is
>> there, then remove it and update the innerHTML appropriatedly --
>> there is then no need to update the link since toggleLinewrap (new
>> name for the function) would work without any arguments.
>
> Yes, this really looks better :)
>
> Thanks for your comments here. Sent a new version considering these
> suggestions.

I was just thinking out loud, so I'm glad you liked the suggestions and
thanks for turning them into a proper patch :)

Patch

diff -r 6187d6255fd2 -r 161cf6af00a4 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,6 +67,7 @@ 
 </table>
 
 <div class="overflow">
+<div class="sourcefirst" style="float: right;">line wrap: <a class="linewraplink" href="javascript:setLinewrap('off')">on</a></div>
 <div class="sourcefirst"> line source</div>
 <pre class="sourcelines">{text%fileline}</pre>
 <div class="sourcelast"></div>
diff -r 6187d6255fd2 -r 161cf6af00a4 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
@@ -269,3 +269,16 @@ 
 	document.getElementById('diffstatdetails').style.display = flag ? 'inline' : 'none';
 	document.getElementById('diffstatexpand').style.display = flag ? 'none' : 'inline';
 }
+
+function setLinewrap(flag) {
+    var nodes = document.querySelectorAll('.sourcelines > span');
+    for (var i = 0; i < nodes.length; i++) {
+        nodes[i].style.whiteSpace = flag == 'on' ? 'pre-wrap' : 'pre';
+    }
+    
+    var links = document.getElementsByClassName('linewraplink');
+    for (var i = 0; i < links.length; i++) {
+        links[i].innerHTML = flag == 'on' ? 'on' : 'off';
+        links[i].href = 'javascript:setLinewrap("' + (flag == 'on' ? 'off' : 'on') + '");'
+    }
+}