Patchwork [7,of,7,V5] hgweb: expose a followlines UI in filerevision view (RFC)

login
register
mail settings
Submitter Denis Laxalde
Date March 25, 2017, 9:21 a.m.
Message ID <ec77aa4ff2993057b604.1490433662@marimba>
Download mbox | patch
Permalink /patch/19658/
State Accepted
Headers show

Comments

Denis Laxalde - March 25, 2017, 9:21 a.m.
# HG changeset patch
# User Denis Laxalde <denis.laxalde@logilab.fr>
# Date 1489594320 -3600
#      Wed Mar 15 17:12:00 2017 +0100
# Node ID ec77aa4ff2993057b604bdffb449d2d179525a9f
# Parent  2f75006a7f31c97d29fd6dd9b72fa7cc9e7ab840
# Available At https://hg.logilab.org/users/dlaxalde/hg
#              hg pull https://hg.logilab.org/users/dlaxalde/hg -r ec77aa4ff299
# EXP-Topic linerange-log/hgweb-filelog
hgweb: expose a followlines UI in filerevision view (RFC)

In filerevision view (/file/<rev>/<fname>) we add some event listeners on
mouse selection of <span> elements in the <pre class="sourcelines"> block.
Those listeners will capture the range of mouse-selected lines and, upon mouse
release, a box inviting to follow the history of selected lines will show up.
This action is advertised by a :after pseudo-element on file lines that shows
up on hover and invite to "select a block of lines to follow its history".

All JavaScript code lives in a new "linerangelog.js" file, sourced in
filerevision template (only in "paper" style for now).

This is proposal implementation, comments welcome on any aspects.
Gregory Szorc - March 25, 2017, 7:34 p.m.
On Sat, Mar 25, 2017 at 2:21 AM, Denis Laxalde <denis@laxalde.org> wrote:

> # HG changeset patch
> # User Denis Laxalde <denis.laxalde@logilab.fr>
> # Date 1489594320 -3600
> #      Wed Mar 15 17:12:00 2017 +0100
> # Node ID ec77aa4ff2993057b604bdffb449d2d179525a9f
> # Parent  2f75006a7f31c97d29fd6dd9b72fa7cc9e7ab840
> # Available At https://hg.logilab.org/users/dlaxalde/hg
> #              hg pull https://hg.logilab.org/users/dlaxalde/hg -r
> ec77aa4ff299
> # EXP-Topic linerange-log/hgweb-filelog
> hgweb: expose a followlines UI in filerevision view (RFC)
>
> In filerevision view (/file/<rev>/<fname>) we add some event listeners on
> mouse selection of <span> elements in the <pre class="sourcelines"> block.
> Those listeners will capture the range of mouse-selected lines and, upon
> mouse
> release, a box inviting to follow the history of selected lines will show
> up.
> This action is advertised by a :after pseudo-element on file lines that
> shows
> up on hover and invite to "select a block of lines to follow its history".
>
> All JavaScript code lives in a new "linerangelog.js" file, sourced in
> filerevision template (only in "paper" style for now).
>
> This is proposal implementation, comments welcome on any aspects.
>

This feature is frigging awesome!!! I can pretty much guarantee that some
engineers at Mozilla will go completely crazy for this feature.

As I'm playing with it locally, I found a few paper cuts.

First, on the initial revision introducing lines (last revision as rendered
in hgweb), the diff is often (always?) empty. I can also see empty diffs in
the intermediate changesets. For example, run this on mercurial/exchange.py
and ask it for the line range of the _pushdiscovery(pushop) function. For
this function, I get 2 empty diffs (revisions 233623d58c9a and
ddb56e7e1b92). Highlighting _pushdiscoverychangeset() yields only an empty
initial diff.

Second, the highlighting UI is a bit confusing. It took me a few seconds to
realize I had to actually hold down the mouse button and highlight the
lines. I don't think users are accustomed to do this on text in web pages
unless they are copying text. It was definitely surprising to me that
highlighting lines of text resulted in a special pop-up appearing. I'm no
web design expert, but how about this design.

As you mouseover lines, you see a graphic cursor somewhere on that line.
There is likely a label next to it saying "click anywhere to follow from
this line" or something like that. This is similar to the floating text
label you have now. When you mouse click, that floating cursor is dropped
and locked into place on that line. On subsequent mouseovers, another
cursor+label appears. The lines between the initial cursor and the current
mouse location are highlighted somehow (possibly adding a different
background color). The label says "click anywhere to follow lines XX to YY"
or something. When the user clicks to drop the 2nd cursor, either they're
taken directly to the filelog view or we get an inline box with links (like
the line number mouseovers on the annotate page). If the user accidentally
clicks to drop a cursor, they can clear the cursor be clicking the cursor
or an "x" next to it.

Another minor nitpick with the highlighting UI is the "follow lines" box
may not appear next to the mouse cursor. I think we want it to appear near
the cursor so it is easier to find/use.

The UX can obviously be improved as a follow-up. I don't want to detract
from getting the core of the feature landed. This is shaping up to be
pretty amazing!


>
> diff --git a/contrib/wix/templates.wxs b/contrib/wix/templates.wxs
> --- a/contrib/wix/templates.wxs
> +++ b/contrib/wix/templates.wxs
> @@ -225,6 +225,7 @@
>              <File Id="static.coal.file.png"      Name="coal-file.png" />
>              <File Id="static.coal.folder.png"    Name="coal-folder.png" />
>              <File Id="static.excanvas.js"        Name="excanvas.js" />
> +            <File Id="static.linerangelog.js"    Name="linerangelog.js" />
>              <File Id="static.mercurial.js"       Name="mercurial.js" />
>              <File Id="static.hgicon.png"         Name="hgicon.png" />
>              <File Id="static.hglogo.png"         Name="hglogo.png" />
> diff --git a/mercurial/templates/paper/filerevision.tmpl
> b/mercurial/templates/paper/filerevision.tmpl
> --- a/mercurial/templates/paper/filerevision.tmpl
> +++ b/mercurial/templates/paper/filerevision.tmpl
> @@ -71,8 +71,11 @@
>  <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 stripes4 wrap bottomline">{text%fileline}</pre>
> +<pre class="sourcelines stripes4 wrap bottomline"
> data-logurl="{url|urlescape}log/{symrev}/{file|urlescape}"
> >{text%fileline}</pre>
>  </div>
> +
> +<script type="text/javascript" src="{staticurl|urlescape}
> linerangelog.js"></script>
> +
>  </div>
>  </div>
>
> diff --git a/mercurial/templates/static/linerangelog.js
> b/mercurial/templates/static/linerangelog.js
> new file mode 100644
> --- /dev/null
> +++ b/mercurial/templates/static/linerangelog.js
> @@ -0,0 +1,83 @@
> +// Copyright 2017 Logilab SA <contact@logilab.fr>
> +//
> +// This software may be used and distributed according to the terms
> +// of the GNU General Public License, incorporated herein by reference.
> +
> +document.addEventListener('DOMContentLoaded', function() {
> +    // install mouse event listeners on <pre class="sourcelines">
> +    var sourcelines = document.getElementsByClassName('sourcelines')[0];
> +    if (typeof sourcelines === 'undefined') {
> +        return;
> +    }
> +    var targetUri = sourcelines.dataset.logurl;
> +    if (typeof targetUri === 'undefined') {
> +        return;
> +    }
> +    // add event listener to the whole <pre class="sourcelines"> block to
> have
> +    // it available in all children <span>
> +    sourcelines.addEventListener('mousedown', lineSelectStart);
> +
> +    //** event handler for "mousedown" */
> +    function lineSelectStart(e) {
> +        var startElement = e.target;
> +        if (startElement.tagName !== 'SPAN') {
> +            // we may be on a <a>
> +            return;
> +        }
> +        // retarget "mouseup" to sourcelines element so that if this event
> +        // occurs outside the <pre> we don't miss it
> +        sourcelines.setCapture();
> +        // drop any prior <div id="followlines">
> +        var previousInvite = document.getElementById('followlines');
> +        if (previousInvite !== null) {
> +            previousInvite.parentNode.removeChild(previousInvite);
> +        }
> +
> +        var startId = parseInt(startElement.id.slice(1));
> +
> +        //** event handler for "mouseup" */
> +        function lineSelectEnd(e) {
> +            // remove "mouseup" listener
> +            sourcelines.removeEventListener('mouseup', lineSelectEnd);
> +
> +            var endElement = e.target;
> +            if (endElement.tagName !== 'SPAN') {
> +                // not on <span>, probably outside <pre
> class="sourcelines">,
> +                // we cannot compute endId
> +                return;
> +            }
> +
> +            // compute line range (startId, endId) and insert the
> +            // "followlines" element
> +            var endId = parseInt(endElement.id.slice(1));
> +            if (endId == startId) {
> +                return;
> +            }
> +            var inviteElement = endElement;
> +            if (endId < startId) {
> +                var tmp = endId;
> +                endId = startId;
> +                startId = tmp;
> +                inviteElement = startElement;
> +            }
> +            var div = followlinesBox(startId, endId);
> +            inviteElement.appendChild(div);
> +        }
> +
> +        sourcelines.addEventListener('mouseup', lineSelectEnd);
> +
> +    }
> +
> +    //** return a <div id="followlines"> with a link for followlines
> action */
> +    function followlinesBox(startId, endId) {
> +        var div = document.createElement('div');
> +        div.id = 'followlines';
> +        var a = document.createElement('a');
> +        var url = targetUri + '?patch=&linerange=' + startId + ':' +
> endId;
> +        a.setAttribute('href', url);
> +        a.textContent = 'follow lines ' + startId + ':' + endId;
> +        div.appendChild(a);
> +        return div;
> +    }
> +
> +}, false);
> diff --git a/mercurial/templates/static/style-paper.css
> b/mercurial/templates/static/style-paper.css
> --- a/mercurial/templates/static/style-paper.css
> +++ b/mercurial/templates/static/style-paper.css
> @@ -276,10 +276,28 @@ td.annotate:hover div.annotate-info { di
>    float: left;
>  }
>
> +div.overflow pre.sourcelines > span:hover:after {
> +  content: "select a block of lines to follow its history";
> +  display: inline;
> +  float: right;
> +  line-height: 1em;
> +  background-color: #FFFFFF;
> +  color: #001199;
> +}
> +
>  .sourcelines > span:target, tr:target td {
>    background-color: #bfdfff;
>  }
>
> +div#followlines {
> +  display: inline;
> +  position: absolute;
> +  background-color: #FFFFFF;
> +  border: 1px solid #999;
> +  color: #000000;
> +  padding: 2px;
> +}
> +
>  .sourcelines > a {
>      display: inline-block;
>      position: absolute;
> diff --git a/tests/test-hgweb-commands.t b/tests/test-hgweb-commands.t
> --- a/tests/test-hgweb-commands.t
> +++ b/tests/test-hgweb-commands.t
> @@ -1343,9 +1343,12 @@ File-related
>    <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 stripes4 wrap bottomline">
> +  <pre class="sourcelines stripes4 wrap bottomline"
> data-logurl="/log/1/foo">
>    <span id="l1">foo</span><a href="#l1"></a></pre>
>    </div>
> +
> +  <script type="text/javascript" src="/static/linerangelog.js"></script>
> +
>    </div>
>    </div>
>
> @@ -1468,9 +1471,12 @@ File-related
>    <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 stripes4 wrap bottomline">
> +  <pre class="sourcelines stripes4 wrap bottomline"
> data-logurl="/log/2/foo">
>    <span id="l1">another</span><a href="#l1"></a></pre>
>    </div>
> +
> +  <script type="text/javascript" src="/static/linerangelog.js"></script>
> +
>    </div>
>    </div>
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Gregory Szorc - March 25, 2017, 7:46 p.m.
On Sat, Mar 25, 2017 at 12:34 PM, Gregory Szorc <gregory.szorc@gmail.com>
wrote:

> On Sat, Mar 25, 2017 at 2:21 AM, Denis Laxalde <denis@laxalde.org> wrote:
>
>> # HG changeset patch
>> # User Denis Laxalde <denis.laxalde@logilab.fr>
>> # Date 1489594320 -3600
>> #      Wed Mar 15 17:12:00 2017 +0100
>> # Node ID ec77aa4ff2993057b604bdffb449d2d179525a9f
>> # Parent  2f75006a7f31c97d29fd6dd9b72fa7cc9e7ab840
>> # Available At https://hg.logilab.org/users/dlaxalde/hg
>> #              hg pull https://hg.logilab.org/users/dlaxalde/hg -r
>> ec77aa4ff299
>> # EXP-Topic linerange-log/hgweb-filelog
>> hgweb: expose a followlines UI in filerevision view (RFC)
>>
>> In filerevision view (/file/<rev>/<fname>) we add some event listeners on
>> mouse selection of <span> elements in the <pre class="sourcelines"> block.
>> Those listeners will capture the range of mouse-selected lines and, upon
>> mouse
>> release, a box inviting to follow the history of selected lines will show
>> up.
>> This action is advertised by a :after pseudo-element on file lines that
>> shows
>> up on hover and invite to "select a block of lines to follow its history".
>>
>> All JavaScript code lives in a new "linerangelog.js" file, sourced in
>> filerevision template (only in "paper" style for now).
>>
>> This is proposal implementation, comments welcome on any aspects.
>>
>
> This feature is frigging awesome!!! I can pretty much guarantee that some
> engineers at Mozilla will go completely crazy for this feature.
>
> As I'm playing with it locally, I found a few paper cuts.
>
> First, on the initial revision introducing lines (last revision as
> rendered in hgweb), the diff is often (always?) empty. I can also see empty
> diffs in the intermediate changesets. For example, run this on
> mercurial/exchange.py and ask it for the line range of the
> _pushdiscovery(pushop) function. For this function, I get 2 empty diffs
> (revisions 233623d58c9a and ddb56e7e1b92). Highlighting
> _pushdiscoverychangeset() yields only an empty initial diff.
>
> Second, the highlighting UI is a bit confusing. It took me a few seconds
> to realize I had to actually hold down the mouse button and highlight the
> lines. I don't think users are accustomed to do this on text in web pages
> unless they are copying text. It was definitely surprising to me that
> highlighting lines of text resulted in a special pop-up appearing. I'm no
> web design expert, but how about this design.
>
> As you mouseover lines, you see a graphic cursor somewhere on that line.
> There is likely a label next to it saying "click anywhere to follow from
> this line" or something like that. This is similar to the floating text
> label you have now. When you mouse click, that floating cursor is dropped
> and locked into place on that line. On subsequent mouseovers, another
> cursor+label appears. The lines between the initial cursor and the current
> mouse location are highlighted somehow (possibly adding a different
> background color). The label says "click anywhere to follow lines XX to YY"
> or something. When the user clicks to drop the 2nd cursor, either they're
> taken directly to the filelog view or we get an inline box with links (like
> the line number mouseovers on the annotate page). If the user accidentally
> clicks to drop a cursor, they can clear the cursor be clicking the cursor
> or an "x" next to it.
>
> Another minor nitpick with the highlighting UI is the "follow lines" box
> may not appear next to the mouse cursor. I think we want it to appear near
> the cursor so it is easier to find/use.
>
> The UX can obviously be improved as a follow-up. I don't want to detract
> from getting the core of the feature landed. This is shaping up to be
> pretty amazing!
>

There's also a performance issue on very large files (such as
layout/base/nsCSSFrameConstructor.cpp from a Firefox repo) in Firefox. When
you start highlighting, the browser slows down dramatically. This doesn't
happen in Chrome. (But I can't get the pop-up box to render in Chrome.)
This is definitely a regression from this patch.

FWIW, introducing poorly-performing code in a web-based tool that Firefox
developers use is a good way to make Firefox faster. I've seen web
performance issues in hgweb and ReviewBoard result in performance
improvements to Firefox. Funny how that works.


>
>
>>
>> diff --git a/contrib/wix/templates.wxs b/contrib/wix/templates.wxs
>> --- a/contrib/wix/templates.wxs
>> +++ b/contrib/wix/templates.wxs
>> @@ -225,6 +225,7 @@
>>              <File Id="static.coal.file.png"      Name="coal-file.png" />
>>              <File Id="static.coal.folder.png"    Name="coal-folder.png"
>> />
>>              <File Id="static.excanvas.js"        Name="excanvas.js" />
>> +            <File Id="static.linerangelog.js"    Name="linerangelog.js"
>> />
>>              <File Id="static.mercurial.js"       Name="mercurial.js" />
>>              <File Id="static.hgicon.png"         Name="hgicon.png" />
>>              <File Id="static.hglogo.png"         Name="hglogo.png" />
>> diff --git a/mercurial/templates/paper/filerevision.tmpl
>> b/mercurial/templates/paper/filerevision.tmpl
>> --- a/mercurial/templates/paper/filerevision.tmpl
>> +++ b/mercurial/templates/paper/filerevision.tmpl
>> @@ -71,8 +71,11 @@
>>  <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 stripes4 wrap bottomline">{text%fileline}</pre>
>> +<pre class="sourcelines stripes4 wrap bottomline"
>> data-logurl="{url|urlescape}log/{symrev}/{file|urlescape}">{
>> text%fileline}</pre>
>>  </div>
>> +
>> +<script type="text/javascript" src="{staticurl|urlescape}line
>> rangelog.js"></script>
>> +
>>  </div>
>>  </div>
>>
>> diff --git a/mercurial/templates/static/linerangelog.js
>> b/mercurial/templates/static/linerangelog.js
>> new file mode 100644
>> --- /dev/null
>> +++ b/mercurial/templates/static/linerangelog.js
>> @@ -0,0 +1,83 @@
>> +// Copyright 2017 Logilab SA <contact@logilab.fr>
>> +//
>> +// This software may be used and distributed according to the terms
>> +// of the GNU General Public License, incorporated herein by reference.
>> +
>> +document.addEventListener('DOMContentLoaded', function() {
>> +    // install mouse event listeners on <pre class="sourcelines">
>> +    var sourcelines = document.getElementsByClassName('sourcelines')[0];
>> +    if (typeof sourcelines === 'undefined') {
>> +        return;
>> +    }
>> +    var targetUri = sourcelines.dataset.logurl;
>> +    if (typeof targetUri === 'undefined') {
>> +        return;
>> +    }
>> +    // add event listener to the whole <pre class="sourcelines"> block
>> to have
>> +    // it available in all children <span>
>> +    sourcelines.addEventListener('mousedown', lineSelectStart);
>> +
>> +    //** event handler for "mousedown" */
>> +    function lineSelectStart(e) {
>> +        var startElement = e.target;
>> +        if (startElement.tagName !== 'SPAN') {
>> +            // we may be on a <a>
>> +            return;
>> +        }
>> +        // retarget "mouseup" to sourcelines element so that if this
>> event
>> +        // occurs outside the <pre> we don't miss it
>> +        sourcelines.setCapture();
>> +        // drop any prior <div id="followlines">
>> +        var previousInvite = document.getElementById('followlines');
>> +        if (previousInvite !== null) {
>> +            previousInvite.parentNode.removeChild(previousInvite);
>> +        }
>> +
>> +        var startId = parseInt(startElement.id.slice(1));
>> +
>> +        //** event handler for "mouseup" */
>> +        function lineSelectEnd(e) {
>> +            // remove "mouseup" listener
>> +            sourcelines.removeEventListener('mouseup', lineSelectEnd);
>> +
>> +            var endElement = e.target;
>> +            if (endElement.tagName !== 'SPAN') {
>> +                // not on <span>, probably outside <pre
>> class="sourcelines">,
>> +                // we cannot compute endId
>> +                return;
>> +            }
>> +
>> +            // compute line range (startId, endId) and insert the
>> +            // "followlines" element
>> +            var endId = parseInt(endElement.id.slice(1));
>> +            if (endId == startId) {
>> +                return;
>> +            }
>> +            var inviteElement = endElement;
>> +            if (endId < startId) {
>> +                var tmp = endId;
>> +                endId = startId;
>> +                startId = tmp;
>> +                inviteElement = startElement;
>> +            }
>> +            var div = followlinesBox(startId, endId);
>> +            inviteElement.appendChild(div);
>> +        }
>> +
>> +        sourcelines.addEventListener('mouseup', lineSelectEnd);
>> +
>> +    }
>> +
>> +    //** return a <div id="followlines"> with a link for followlines
>> action */
>> +    function followlinesBox(startId, endId) {
>> +        var div = document.createElement('div');
>> +        div.id = 'followlines';
>> +        var a = document.createElement('a');
>> +        var url = targetUri + '?patch=&linerange=' + startId + ':' +
>> endId;
>> +        a.setAttribute('href', url);
>> +        a.textContent = 'follow lines ' + startId + ':' + endId;
>> +        div.appendChild(a);
>> +        return div;
>> +    }
>> +
>> +}, false);
>> diff --git a/mercurial/templates/static/style-paper.css
>> b/mercurial/templates/static/style-paper.css
>> --- a/mercurial/templates/static/style-paper.css
>> +++ b/mercurial/templates/static/style-paper.css
>> @@ -276,10 +276,28 @@ td.annotate:hover div.annotate-info { di
>>    float: left;
>>  }
>>
>> +div.overflow pre.sourcelines > span:hover:after {
>> +  content: "select a block of lines to follow its history";
>> +  display: inline;
>> +  float: right;
>> +  line-height: 1em;
>> +  background-color: #FFFFFF;
>> +  color: #001199;
>> +}
>> +
>>  .sourcelines > span:target, tr:target td {
>>    background-color: #bfdfff;
>>  }
>>
>> +div#followlines {
>> +  display: inline;
>> +  position: absolute;
>> +  background-color: #FFFFFF;
>> +  border: 1px solid #999;
>> +  color: #000000;
>> +  padding: 2px;
>> +}
>> +
>>  .sourcelines > a {
>>      display: inline-block;
>>      position: absolute;
>> diff --git a/tests/test-hgweb-commands.t b/tests/test-hgweb-commands.t
>> --- a/tests/test-hgweb-commands.t
>> +++ b/tests/test-hgweb-commands.t
>> @@ -1343,9 +1343,12 @@ File-related
>>    <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 stripes4 wrap bottomline">
>> +  <pre class="sourcelines stripes4 wrap bottomline"
>> data-logurl="/log/1/foo">
>>    <span id="l1">foo</span><a href="#l1"></a></pre>
>>    </div>
>> +
>> +  <script type="text/javascript" src="/static/linerangelog.js"></script>
>> +
>>    </div>
>>    </div>
>>
>> @@ -1468,9 +1471,12 @@ File-related
>>    <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 stripes4 wrap bottomline">
>> +  <pre class="sourcelines stripes4 wrap bottomline"
>> data-logurl="/log/2/foo">
>>    <span id="l1">another</span><a href="#l1"></a></pre>
>>    </div>
>> +
>> +  <script type="text/javascript" src="/static/linerangelog.js"></script>
>> +
>>    </div>
>>    </div>
>>
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@mercurial-scm.org
>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>>
>
>
Augie Fackler - March 27, 2017, 4:40 p.m.
On Sat, Mar 25, 2017 at 12:34:47PM -0700, Gregory Szorc wrote:
> On Sat, Mar 25, 2017 at 2:21 AM, Denis Laxalde <denis@laxalde.org> wrote:
>
> > # HG changeset patch
> > # User Denis Laxalde <denis.laxalde@logilab.fr>
> > # Date 1489594320 -3600
> > #      Wed Mar 15 17:12:00 2017 +0100
> > # Node ID ec77aa4ff2993057b604bdffb449d2d179525a9f
> > # Parent  2f75006a7f31c97d29fd6dd9b72fa7cc9e7ab840
> > # Available At https://hg.logilab.org/users/dlaxalde/hg
> > #              hg pull https://hg.logilab.org/users/dlaxalde/hg -r
> > ec77aa4ff299
> > # EXP-Topic linerange-log/hgweb-filelog
> > hgweb: expose a followlines UI in filerevision view (RFC)
> >
> > In filerevision view (/file/<rev>/<fname>) we add some event listeners on
> > mouse selection of <span> elements in the <pre class="sourcelines"> block.
> > Those listeners will capture the range of mouse-selected lines and, upon
> > mouse
> > release, a box inviting to follow the history of selected lines will show
> > up.
> > This action is advertised by a :after pseudo-element on file lines that
> > shows
> > up on hover and invite to "select a block of lines to follow its history".
> >
> > All JavaScript code lives in a new "linerangelog.js" file, sourced in
> > filerevision template (only in "paper" style for now).
> >
> > This is proposal implementation, comments welcome on any aspects.
> >
>
> This feature is frigging awesome!!! I can pretty much guarantee that some
> engineers at Mozilla will go completely crazy for this feature.
>
> As I'm playing with it locally, I found a few paper cuts.

I've queued patches 1-6, which lay all the groundwork for this. Greg,
can I have you drive the review of this last patch and let me know
when I should take a look at it?

Thanks!

(Very excited for this cool feature)

>
> First, on the initial revision introducing lines (last revision as rendered
> in hgweb), the diff is often (always?) empty. I can also see empty diffs in
> the intermediate changesets. For example, run this on mercurial/exchange.py
> and ask it for the line range of the _pushdiscovery(pushop) function. For
> this function, I get 2 empty diffs (revisions 233623d58c9a and
> ddb56e7e1b92). Highlighting _pushdiscoverychangeset() yields only an empty
> initial diff.
>
> Second, the highlighting UI is a bit confusing. It took me a few seconds to
> realize I had to actually hold down the mouse button and highlight the
> lines. I don't think users are accustomed to do this on text in web pages
> unless they are copying text. It was definitely surprising to me that
> highlighting lines of text resulted in a special pop-up appearing. I'm no
> web design expert, but how about this design.
>
> As you mouseover lines, you see a graphic cursor somewhere on that line.
> There is likely a label next to it saying "click anywhere to follow from
> this line" or something like that. This is similar to the floating text
> label you have now. When you mouse click, that floating cursor is dropped
> and locked into place on that line. On subsequent mouseovers, another
> cursor+label appears. The lines between the initial cursor and the current
> mouse location are highlighted somehow (possibly adding a different
> background color). The label says "click anywhere to follow lines XX to YY"
> or something. When the user clicks to drop the 2nd cursor, either they're
> taken directly to the filelog view or we get an inline box with links (like
> the line number mouseovers on the annotate page). If the user accidentally
> clicks to drop a cursor, they can clear the cursor be clicking the cursor
> or an "x" next to it.
>
> Another minor nitpick with the highlighting UI is the "follow lines" box
> may not appear next to the mouse cursor. I think we want it to appear near
> the cursor so it is easier to find/use.
>
> The UX can obviously be improved as a follow-up. I don't want to detract
> from getting the core of the feature landed. This is shaping up to be
> pretty amazing!
>
>
> >
> > diff --git a/contrib/wix/templates.wxs b/contrib/wix/templates.wxs
> > --- a/contrib/wix/templates.wxs
> > +++ b/contrib/wix/templates.wxs
> > @@ -225,6 +225,7 @@
> >              <File Id="static.coal.file.png"      Name="coal-file.png" />
> >              <File Id="static.coal.folder.png"    Name="coal-folder.png" />
> >              <File Id="static.excanvas.js"        Name="excanvas.js" />
> > +            <File Id="static.linerangelog.js"    Name="linerangelog.js" />
> >              <File Id="static.mercurial.js"       Name="mercurial.js" />
> >              <File Id="static.hgicon.png"         Name="hgicon.png" />
> >              <File Id="static.hglogo.png"         Name="hglogo.png" />
> > diff --git a/mercurial/templates/paper/filerevision.tmpl
> > b/mercurial/templates/paper/filerevision.tmpl
> > --- a/mercurial/templates/paper/filerevision.tmpl
> > +++ b/mercurial/templates/paper/filerevision.tmpl
> > @@ -71,8 +71,11 @@
> >  <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 stripes4 wrap bottomline">{text%fileline}</pre>
> > +<pre class="sourcelines stripes4 wrap bottomline"
> > data-logurl="{url|urlescape}log/{symrev}/{file|urlescape}"
> > >{text%fileline}</pre>
> >  </div>
> > +
> > +<script type="text/javascript" src="{staticurl|urlescape}
> > linerangelog.js"></script>
> > +
> >  </div>
> >  </div>
> >
> > diff --git a/mercurial/templates/static/linerangelog.js
> > b/mercurial/templates/static/linerangelog.js
> > new file mode 100644
> > --- /dev/null
> > +++ b/mercurial/templates/static/linerangelog.js
> > @@ -0,0 +1,83 @@
> > +// Copyright 2017 Logilab SA <contact@logilab.fr>
> > +//
> > +// This software may be used and distributed according to the terms
> > +// of the GNU General Public License, incorporated herein by reference.
> > +
> > +document.addEventListener('DOMContentLoaded', function() {
> > +    // install mouse event listeners on <pre class="sourcelines">
> > +    var sourcelines = document.getElementsByClassName('sourcelines')[0];
> > +    if (typeof sourcelines === 'undefined') {
> > +        return;
> > +    }
> > +    var targetUri = sourcelines.dataset.logurl;
> > +    if (typeof targetUri === 'undefined') {
> > +        return;
> > +    }
> > +    // add event listener to the whole <pre class="sourcelines"> block to
> > have
> > +    // it available in all children <span>
> > +    sourcelines.addEventListener('mousedown', lineSelectStart);
> > +
> > +    //** event handler for "mousedown" */
> > +    function lineSelectStart(e) {
> > +        var startElement = e.target;
> > +        if (startElement.tagName !== 'SPAN') {
> > +            // we may be on a <a>
> > +            return;
> > +        }
> > +        // retarget "mouseup" to sourcelines element so that if this event
> > +        // occurs outside the <pre> we don't miss it
> > +        sourcelines.setCapture();
> > +        // drop any prior <div id="followlines">
> > +        var previousInvite = document.getElementById('followlines');
> > +        if (previousInvite !== null) {
> > +            previousInvite.parentNode.removeChild(previousInvite);
> > +        }
> > +
> > +        var startId = parseInt(startElement.id.slice(1));
> > +
> > +        //** event handler for "mouseup" */
> > +        function lineSelectEnd(e) {
> > +            // remove "mouseup" listener
> > +            sourcelines.removeEventListener('mouseup', lineSelectEnd);
> > +
> > +            var endElement = e.target;
> > +            if (endElement.tagName !== 'SPAN') {
> > +                // not on <span>, probably outside <pre
> > class="sourcelines">,
> > +                // we cannot compute endId
> > +                return;
> > +            }
> > +
> > +            // compute line range (startId, endId) and insert the
> > +            // "followlines" element
> > +            var endId = parseInt(endElement.id.slice(1));
> > +            if (endId == startId) {
> > +                return;
> > +            }
> > +            var inviteElement = endElement;
> > +            if (endId < startId) {
> > +                var tmp = endId;
> > +                endId = startId;
> > +                startId = tmp;
> > +                inviteElement = startElement;
> > +            }
> > +            var div = followlinesBox(startId, endId);
> > +            inviteElement.appendChild(div);
> > +        }
> > +
> > +        sourcelines.addEventListener('mouseup', lineSelectEnd);
> > +
> > +    }
> > +
> > +    //** return a <div id="followlines"> with a link for followlines
> > action */
> > +    function followlinesBox(startId, endId) {
> > +        var div = document.createElement('div');
> > +        div.id = 'followlines';
> > +        var a = document.createElement('a');
> > +        var url = targetUri + '?patch=&linerange=' + startId + ':' +
> > endId;
> > +        a.setAttribute('href', url);
> > +        a.textContent = 'follow lines ' + startId + ':' + endId;
> > +        div.appendChild(a);
> > +        return div;
> > +    }
> > +
> > +}, false);
> > diff --git a/mercurial/templates/static/style-paper.css
> > b/mercurial/templates/static/style-paper.css
> > --- a/mercurial/templates/static/style-paper.css
> > +++ b/mercurial/templates/static/style-paper.css
> > @@ -276,10 +276,28 @@ td.annotate:hover div.annotate-info { di
> >    float: left;
> >  }
> >
> > +div.overflow pre.sourcelines > span:hover:after {
> > +  content: "select a block of lines to follow its history";
> > +  display: inline;
> > +  float: right;
> > +  line-height: 1em;
> > +  background-color: #FFFFFF;
> > +  color: #001199;
> > +}
> > +
> >  .sourcelines > span:target, tr:target td {
> >    background-color: #bfdfff;
> >  }
> >
> > +div#followlines {
> > +  display: inline;
> > +  position: absolute;
> > +  background-color: #FFFFFF;
> > +  border: 1px solid #999;
> > +  color: #000000;
> > +  padding: 2px;
> > +}
> > +
> >  .sourcelines > a {
> >      display: inline-block;
> >      position: absolute;
> > diff --git a/tests/test-hgweb-commands.t b/tests/test-hgweb-commands.t
> > --- a/tests/test-hgweb-commands.t
> > +++ b/tests/test-hgweb-commands.t
> > @@ -1343,9 +1343,12 @@ File-related
> >    <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 stripes4 wrap bottomline">
> > +  <pre class="sourcelines stripes4 wrap bottomline"
> > data-logurl="/log/1/foo">
> >    <span id="l1">foo</span><a href="#l1"></a></pre>
> >    </div>
> > +
> > +  <script type="text/javascript" src="/static/linerangelog.js"></script>
> > +
> >    </div>
> >    </div>
> >
> > @@ -1468,9 +1471,12 @@ File-related
> >    <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 stripes4 wrap bottomline">
> > +  <pre class="sourcelines stripes4 wrap bottomline"
> > data-logurl="/log/2/foo">
> >    <span id="l1">another</span><a href="#l1"></a></pre>
> >    </div>
> > +
> > +  <script type="text/javascript" src="/static/linerangelog.js"></script>
> > +
> >    </div>
> >    </div>
> >
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel@mercurial-scm.org
> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
> >

> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Gregory Szorc - March 28, 2017, 3:58 a.m.
On Mon, Mar 27, 2017 at 9:40 AM, Augie Fackler <raf@durin42.com> wrote:

> On Sat, Mar 25, 2017 at 12:34:47PM -0700, Gregory Szorc wrote:
> > On Sat, Mar 25, 2017 at 2:21 AM, Denis Laxalde <denis@laxalde.org>
> wrote:
> >
> > > # HG changeset patch
> > > # User Denis Laxalde <denis.laxalde@logilab.fr>
> > > # Date 1489594320 -3600
> > > #      Wed Mar 15 17:12:00 2017 +0100
> > > # Node ID ec77aa4ff2993057b604bdffb449d2d179525a9f
> > > # Parent  2f75006a7f31c97d29fd6dd9b72fa7cc9e7ab840
> > > # Available At https://hg.logilab.org/users/dlaxalde/hg
> > > #              hg pull https://hg.logilab.org/users/dlaxalde/hg -r
> > > ec77aa4ff299
> > > # EXP-Topic linerange-log/hgweb-filelog
> > > hgweb: expose a followlines UI in filerevision view (RFC)
> > >
> > > In filerevision view (/file/<rev>/<fname>) we add some event listeners
> on
> > > mouse selection of <span> elements in the <pre class="sourcelines">
> block.
> > > Those listeners will capture the range of mouse-selected lines and,
> upon
> > > mouse
> > > release, a box inviting to follow the history of selected lines will
> show
> > > up.
> > > This action is advertised by a :after pseudo-element on file lines that
> > > shows
> > > up on hover and invite to "select a block of lines to follow its
> history".
> > >
> > > All JavaScript code lives in a new "linerangelog.js" file, sourced in
> > > filerevision template (only in "paper" style for now).
> > >
> > > This is proposal implementation, comments welcome on any aspects.
> > >
> >
> > This feature is frigging awesome!!! I can pretty much guarantee that some
> > engineers at Mozilla will go completely crazy for this feature.
> >
> > As I'm playing with it locally, I found a few paper cuts.
>
> I've queued patches 1-6, which lay all the groundwork for this. Greg,
> can I have you drive the review of this last patch and let me know
> when I should take a look at it?
>

OK.

I'm not sure how others feel about landing this in its current form. If
another reviewer is OK with it, I'll give it a thorough review. Otherwise,
I'll wait for UI enhancements.

Also, I'd encourage others to comment on the UI.


>
> Thanks!
>
> (Very excited for this cool feature)
>
> >
> > First, on the initial revision introducing lines (last revision as
> rendered
> > in hgweb), the diff is often (always?) empty. I can also see empty diffs
> in
> > the intermediate changesets. For example, run this on
> mercurial/exchange.py
> > and ask it for the line range of the _pushdiscovery(pushop) function. For
> > this function, I get 2 empty diffs (revisions 233623d58c9a and
> > ddb56e7e1b92). Highlighting _pushdiscoverychangeset() yields only an
> empty
> > initial diff.
> >
> > Second, the highlighting UI is a bit confusing. It took me a few seconds
> to
> > realize I had to actually hold down the mouse button and highlight the
> > lines. I don't think users are accustomed to do this on text in web pages
> > unless they are copying text. It was definitely surprising to me that
> > highlighting lines of text resulted in a special pop-up appearing. I'm no
> > web design expert, but how about this design.
> >
> > As you mouseover lines, you see a graphic cursor somewhere on that line.
> > There is likely a label next to it saying "click anywhere to follow from
> > this line" or something like that. This is similar to the floating text
> > label you have now. When you mouse click, that floating cursor is dropped
> > and locked into place on that line. On subsequent mouseovers, another
> > cursor+label appears. The lines between the initial cursor and the
> current
> > mouse location are highlighted somehow (possibly adding a different
> > background color). The label says "click anywhere to follow lines XX to
> YY"
> > or something. When the user clicks to drop the 2nd cursor, either they're
> > taken directly to the filelog view or we get an inline box with links
> (like
> > the line number mouseovers on the annotate page). If the user
> accidentally
> > clicks to drop a cursor, they can clear the cursor be clicking the cursor
> > or an "x" next to it.
> >
> > Another minor nitpick with the highlighting UI is the "follow lines" box
> > may not appear next to the mouse cursor. I think we want it to appear
> near
> > the cursor so it is easier to find/use.
> >
> > The UX can obviously be improved as a follow-up. I don't want to detract
> > from getting the core of the feature landed. This is shaping up to be
> > pretty amazing!
> >
> >
> > >
> > > diff --git a/contrib/wix/templates.wxs b/contrib/wix/templates.wxs
> > > --- a/contrib/wix/templates.wxs
> > > +++ b/contrib/wix/templates.wxs
> > > @@ -225,6 +225,7 @@
> > >              <File Id="static.coal.file.png"      Name="coal-file.png"
> />
> > >              <File Id="static.coal.folder.png"
> Name="coal-folder.png" />
> > >              <File Id="static.excanvas.js"        Name="excanvas.js" />
> > > +            <File Id="static.linerangelog.js"
> Name="linerangelog.js" />
> > >              <File Id="static.mercurial.js"       Name="mercurial.js"
> />
> > >              <File Id="static.hgicon.png"         Name="hgicon.png" />
> > >              <File Id="static.hglogo.png"         Name="hglogo.png" />
> > > diff --git a/mercurial/templates/paper/filerevision.tmpl
> > > b/mercurial/templates/paper/filerevision.tmpl
> > > --- a/mercurial/templates/paper/filerevision.tmpl
> > > +++ b/mercurial/templates/paper/filerevision.tmpl
> > > @@ -71,8 +71,11 @@
> > >  <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 stripes4 wrap bottomline">{text%fileline}</
> pre>
> > > +<pre class="sourcelines stripes4 wrap bottomline"
> > > data-logurl="{url|urlescape}log/{symrev}/{file|urlescape}"
> > > >{text%fileline}</pre>
> > >  </div>
> > > +
> > > +<script type="text/javascript" src="{staticurl|urlescape}
> > > linerangelog.js"></script>
> > > +
> > >  </div>
> > >  </div>
> > >
> > > diff --git a/mercurial/templates/static/linerangelog.js
> > > b/mercurial/templates/static/linerangelog.js
> > > new file mode 100644
> > > --- /dev/null
> > > +++ b/mercurial/templates/static/linerangelog.js
> > > @@ -0,0 +1,83 @@
> > > +// Copyright 2017 Logilab SA <contact@logilab.fr>
> > > +//
> > > +// This software may be used and distributed according to the terms
> > > +// of the GNU General Public License, incorporated herein by
> reference.
> > > +
> > > +document.addEventListener('DOMContentLoaded', function() {
> > > +    // install mouse event listeners on <pre class="sourcelines">
> > > +    var sourcelines = document.getElementsByClassName('
> sourcelines')[0];
> > > +    if (typeof sourcelines === 'undefined') {
> > > +        return;
> > > +    }
> > > +    var targetUri = sourcelines.dataset.logurl;
> > > +    if (typeof targetUri === 'undefined') {
> > > +        return;
> > > +    }
> > > +    // add event listener to the whole <pre class="sourcelines">
> block to
> > > have
> > > +    // it available in all children <span>
> > > +    sourcelines.addEventListener('mousedown', lineSelectStart);
> > > +
> > > +    //** event handler for "mousedown" */
> > > +    function lineSelectStart(e) {
> > > +        var startElement = e.target;
> > > +        if (startElement.tagName !== 'SPAN') {
> > > +            // we may be on a <a>
> > > +            return;
> > > +        }
> > > +        // retarget "mouseup" to sourcelines element so that if this
> event
> > > +        // occurs outside the <pre> we don't miss it
> > > +        sourcelines.setCapture();
> > > +        // drop any prior <div id="followlines">
> > > +        var previousInvite = document.getElementById('followlines');
> > > +        if (previousInvite !== null) {
> > > +            previousInvite.parentNode.removeChild(previousInvite);
> > > +        }
> > > +
> > > +        var startId = parseInt(startElement.id.slice(1));
> > > +
> > > +        //** event handler for "mouseup" */
> > > +        function lineSelectEnd(e) {
> > > +            // remove "mouseup" listener
> > > +            sourcelines.removeEventListener('mouseup',
> lineSelectEnd);
> > > +
> > > +            var endElement = e.target;
> > > +            if (endElement.tagName !== 'SPAN') {
> > > +                // not on <span>, probably outside <pre
> > > class="sourcelines">,
> > > +                // we cannot compute endId
> > > +                return;
> > > +            }
> > > +
> > > +            // compute line range (startId, endId) and insert the
> > > +            // "followlines" element
> > > +            var endId = parseInt(endElement.id.slice(1));
> > > +            if (endId == startId) {
> > > +                return;
> > > +            }
> > > +            var inviteElement = endElement;
> > > +            if (endId < startId) {
> > > +                var tmp = endId;
> > > +                endId = startId;
> > > +                startId = tmp;
> > > +                inviteElement = startElement;
> > > +            }
> > > +            var div = followlinesBox(startId, endId);
> > > +            inviteElement.appendChild(div);
> > > +        }
> > > +
> > > +        sourcelines.addEventListener('mouseup', lineSelectEnd);
> > > +
> > > +    }
> > > +
> > > +    //** return a <div id="followlines"> with a link for followlines
> > > action */
> > > +    function followlinesBox(startId, endId) {
> > > +        var div = document.createElement('div');
> > > +        div.id = 'followlines';
> > > +        var a = document.createElement('a');
> > > +        var url = targetUri + '?patch=&linerange=' + startId + ':' +
> > > endId;
> > > +        a.setAttribute('href', url);
> > > +        a.textContent = 'follow lines ' + startId + ':' + endId;
> > > +        div.appendChild(a);
> > > +        return div;
> > > +    }
> > > +
> > > +}, false);
> > > diff --git a/mercurial/templates/static/style-paper.css
> > > b/mercurial/templates/static/style-paper.css
> > > --- a/mercurial/templates/static/style-paper.css
> > > +++ b/mercurial/templates/static/style-paper.css
> > > @@ -276,10 +276,28 @@ td.annotate:hover div.annotate-info { di
> > >    float: left;
> > >  }
> > >
> > > +div.overflow pre.sourcelines > span:hover:after {
> > > +  content: "select a block of lines to follow its history";
> > > +  display: inline;
> > > +  float: right;
> > > +  line-height: 1em;
> > > +  background-color: #FFFFFF;
> > > +  color: #001199;
> > > +}
> > > +
> > >  .sourcelines > span:target, tr:target td {
> > >    background-color: #bfdfff;
> > >  }
> > >
> > > +div#followlines {
> > > +  display: inline;
> > > +  position: absolute;
> > > +  background-color: #FFFFFF;
> > > +  border: 1px solid #999;
> > > +  color: #000000;
> > > +  padding: 2px;
> > > +}
> > > +
> > >  .sourcelines > a {
> > >      display: inline-block;
> > >      position: absolute;
> > > diff --git a/tests/test-hgweb-commands.t b/tests/test-hgweb-commands.t
> > > --- a/tests/test-hgweb-commands.t
> > > +++ b/tests/test-hgweb-commands.t
> > > @@ -1343,9 +1343,12 @@ File-related
> > >    <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 stripes4 wrap bottomline">
> > > +  <pre class="sourcelines stripes4 wrap bottomline"
> > > data-logurl="/log/1/foo">
> > >    <span id="l1">foo</span><a href="#l1"></a></pre>
> > >    </div>
> > > +
> > > +  <script type="text/javascript" src="/static/linerangelog.js">
> </script>
> > > +
> > >    </div>
> > >    </div>
> > >
> > > @@ -1468,9 +1471,12 @@ File-related
> > >    <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 stripes4 wrap bottomline">
> > > +  <pre class="sourcelines stripes4 wrap bottomline"
> > > data-logurl="/log/2/foo">
> > >    <span id="l1">another</span><a href="#l1"></a></pre>
> > >    </div>
> > > +
> > > +  <script type="text/javascript" src="/static/linerangelog.js">
> </script>
> > > +
> > >    </div>
> > >    </div>
> > >
> > > _______________________________________________
> > > Mercurial-devel mailing list
> > > Mercurial-devel@mercurial-scm.org
> > > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
> > >
>
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel@mercurial-scm.org
> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Denis Laxalde - March 28, 2017, 10:38 a.m.
Gregory Szorc a écrit :
> On Sat, Mar 25, 2017 at 12:34 PM, Gregory Szorc <gregory.szorc@gmail.com>
> wrote:
>
>> On Sat, Mar 25, 2017 at 2:21 AM, Denis Laxalde <denis@laxalde.org> wrote:
>>
>>> # HG changeset patch
>>> # User Denis Laxalde <denis.laxalde@logilab.fr>
>>> # Date 1489594320 -3600
>>> #      Wed Mar 15 17:12:00 2017 +0100
>>> # Node ID ec77aa4ff2993057b604bdffb449d2d179525a9f
>>> # Parent  2f75006a7f31c97d29fd6dd9b72fa7cc9e7ab840
>>> # Available At https://hg.logilab.org/users/dlaxalde/hg
>>> #              hg pull https://hg.logilab.org/users/dlaxalde/hg -r
>>> ec77aa4ff299
>>> # EXP-Topic linerange-log/hgweb-filelog
>>> hgweb: expose a followlines UI in filerevision view (RFC)
>>>
>>> In filerevision view (/file/<rev>/<fname>) we add some event listeners on
>>> mouse selection of <span> elements in the <pre class="sourcelines"> block.
>>> Those listeners will capture the range of mouse-selected lines and, upon
>>> mouse
>>> release, a box inviting to follow the history of selected lines will show
>>> up.
>>> This action is advertised by a :after pseudo-element on file lines that
>>> shows
>>> up on hover and invite to "select a block of lines to follow its history".
>>>
>>> All JavaScript code lives in a new "linerangelog.js" file, sourced in
>>> filerevision template (only in "paper" style for now).
>>>
>>> This is proposal implementation, comments welcome on any aspects.
>>>
>>
>> This feature is frigging awesome!!! I can pretty much guarantee that some
>> engineers at Mozilla will go completely crazy for this feature.
>>
>> As I'm playing with it locally, I found a few paper cuts.
>>
>> First, on the initial revision introducing lines (last revision as
>> rendered in hgweb), the diff is often (always?) empty. I can also see empty
>> diffs in the intermediate changesets. For example, run this on
>> mercurial/exchange.py and ask it for the line range of the
>> _pushdiscovery(pushop) function. For this function, I get 2 empty diffs
>> (revisions 233623d58c9a and ddb56e7e1b92). Highlighting
>> _pushdiscoverychangeset() yields only an empty initial diff.
>>
>> Second, the highlighting UI is a bit confusing. It took me a few seconds
>> to realize I had to actually hold down the mouse button and highlight the
>> lines. I don't think users are accustomed to do this on text in web pages
>> unless they are copying text. It was definitely surprising to me that
>> highlighting lines of text resulted in a special pop-up appearing. I'm no
>> web design expert, but how about this design.
>>
>> As you mouseover lines, you see a graphic cursor somewhere on that line.
>> There is likely a label next to it saying "click anywhere to follow from
>> this line" or something like that. This is similar to the floating text
>> label you have now. When you mouse click, that floating cursor is dropped
>> and locked into place on that line. On subsequent mouseovers, another
>> cursor+label appears. The lines between the initial cursor and the current
>> mouse location are highlighted somehow (possibly adding a different
>> background color). The label says "click anywhere to follow lines XX to YY"
>> or something. When the user clicks to drop the 2nd cursor, either they're
>> taken directly to the filelog view or we get an inline box with links (like
>> the line number mouseovers on the annotate page). If the user accidentally
>> clicks to drop a cursor, they can clear the cursor be clicking the cursor
>> or an "x" next to it.
>>
>> Another minor nitpick with the highlighting UI is the "follow lines" box
>> may not appear next to the mouse cursor. I think we want it to appear near
>> the cursor so it is easier to find/use.
>>
>> The UX can obviously be improved as a follow-up. I don't want to detract
>> from getting the core of the feature landed. This is shaping up to be
>> pretty amazing!
>>
>
> There's also a performance issue on very large files (such as
> layout/base/nsCSSFrameConstructor.cpp from a Firefox repo) in Firefox. When
> you start highlighting, the browser slows down dramatically. This doesn't
> happen in Chrome. (But I can't get the pop-up box to render in Chrome.)
> This is definitely a regression from this patch.

(chrome issue is because Element.setCapture is not implemented there 
apparently, but MDN does not mention this.)

> FWIW, introducing poorly-performing code in a web-based tool that Firefox
> developers use is a good way to make Firefox faster. I've seen web
> performance issues in hgweb and ReviewBoard result in performance
> improvements to Firefox. Funny how that works.
>

This appears to be because of the following CSS rule, more specifically
the :hover selector on a large number of pseudo-elements.

>>> diff --git a/mercurial/templates/static/style-paper.css
>>> b/mercurial/templates/static/style-paper.css
>>> --- a/mercurial/templates/static/style-paper.css
>>> +++ b/mercurial/templates/static/style-paper.css
>>> @@ -276,10 +276,28 @@ td.annotate:hover div.annotate-info { di
>>>    float: left;
>>>  }
>>>
>>> +div.overflow pre.sourcelines > span:hover:after {
>>> +  content: "select a block of lines to follow its history";
>>> +  display: inline;
>>> +  float: right;
>>> +  line-height: 1em;
>>> +  background-color: #FFFFFF;
>>> +  color: #001199;
>>> +}
>>> +
Denis Laxalde - March 28, 2017, 4:03 p.m.
Gregory Szorc a écrit :
> On Sat, Mar 25, 2017 at 2:21 AM, Denis Laxalde <denis@laxalde.org> wrote:
>
>> > # HG changeset patch
>> > # User Denis Laxalde <denis.laxalde@logilab.fr>
>> > # Date 1489594320 -3600
>> > #      Wed Mar 15 17:12:00 2017 +0100
>> > # Node ID ec77aa4ff2993057b604bdffb449d2d179525a9f
>> > # Parent  2f75006a7f31c97d29fd6dd9b72fa7cc9e7ab840
>> > # Available At https://hg.logilab.org/users/dlaxalde/hg
>> > #              hg pull https://hg.logilab.org/users/dlaxalde/hg -r
>> > ec77aa4ff299
>> > # EXP-Topic linerange-log/hgweb-filelog
>> > hgweb: expose a followlines UI in filerevision view (RFC)
>> >
>> > In filerevision view (/file/<rev>/<fname>) we add some event listeners on
>> > mouse selection of <span> elements in the <pre class="sourcelines"> block.
>> > Those listeners will capture the range of mouse-selected lines and, upon
>> > mouse
>> > release, a box inviting to follow the history of selected lines will show
>> > up.
>> > This action is advertised by a :after pseudo-element on file lines that
>> > shows
>> > up on hover and invite to "select a block of lines to follow its history".
>> >
>> > All JavaScript code lives in a new "linerangelog.js" file, sourced in
>> > filerevision template (only in "paper" style for now).
>> >
>> > This is proposal implementation, comments welcome on any aspects.
>> >
> This feature is frigging awesome!!! I can pretty much guarantee that some
> engineers at Mozilla will go completely crazy for this feature.
>
> As I'm playing with it locally, I found a few paper cuts.
>
> First, on the initial revision introducing lines (last revision as rendered
> in hgweb), the diff is often (always?) empty. I can also see empty diffs in
> the intermediate changesets. For example, run this on mercurial/exchange.py
> and ask it for the line range of the _pushdiscovery(pushop) function. For
> this function, I get 2 empty diffs (revisions 233623d58c9a and
> ddb56e7e1b92). Highlighting _pushdiscoverychangeset() yields only an empty
> initial diff.

There's a bug in patch 5, can it be unqueued? Otherwise I'll fix it in a
followup. In general I think diffs should not be empty unless we're on a
merge changeset and there is no diff with p1.

> Second, the highlighting UI is a bit confusing. It took me a few seconds to
> realize I had to actually hold down the mouse button and highlight the
> lines. I don't think users are accustomed to do this on text in web pages
> unless they are copying text. It was definitely surprising to me that
> highlighting lines of text resulted in a special pop-up appearing. I'm no
> web design expert, but how about this design.
>
> As you mouseover lines, you see a graphic cursor somewhere on that line.
> There is likely a label next to it saying "click anywhere to follow from
> this line" or something like that. This is similar to the floating text
> label you have now. When you mouse click, that floating cursor is dropped
> and locked into place on that line. On subsequent mouseovers, another
> cursor+label appears. The lines between the initial cursor and the current
> mouse location are highlighted somehow (possibly adding a different
> background color). The label says "click anywhere to follow lines XX to YY"
> or something. When the user clicks to drop the 2nd cursor, either they're
> taken directly to the filelog view or we get an inline box with links (like
> the line number mouseovers on the annotate page). If the user accidentally
> clicks to drop a cursor, they can clear the cursor be clicking the cursor
> or an "x" next to it.
>
> Another minor nitpick with the highlighting UI is the "follow lines" box
> may not appear next to the mouse cursor. I think we want it to appear near
> the cursor so it is easier to find/use.
>
> The UX can obviously be improved as a follow-up. I don't want to detract
> from getting the core of the feature landed. This is shaping up to be
> pretty amazing!
>

I'll give your idea a try. It's interesting, looks feasible and is
probably less confusing. Maybe we can make more it accessible as well.
Augie Fackler - March 28, 2017, 5:27 p.m.
On Tue, Mar 28, 2017 at 12:03 PM, Denis Laxalde <denis@laxalde.org> wrote:
> There's a bug in patch 5, can it be unqueued? Otherwise I'll fix it in a
> followup. In general I think diffs should not be empty unless we're on a
> merge changeset and there is no diff with p1.


Please mail a followup, it's easier for me. Thanks!
Denis Laxalde - March 30, 2017, 8:40 a.m.
Gregory Szorc a écrit :
> On Sat, Mar 25, 2017 at 2:21 AM, Denis Laxalde <denis@laxalde.org> wrote:
>
>> # HG changeset patch
>> # User Denis Laxalde <denis.laxalde@logilab.fr>
>> # Date 1489594320 -3600
>> #      Wed Mar 15 17:12:00 2017 +0100
>> # Node ID ec77aa4ff2993057b604bdffb449d2d179525a9f
>> # Parent  2f75006a7f31c97d29fd6dd9b72fa7cc9e7ab840
>> # Available At https://hg.logilab.org/users/dlaxalde/hg
>> #              hg pull https://hg.logilab.org/users/dlaxalde/hg -r
>> ec77aa4ff299
>> # EXP-Topic linerange-log/hgweb-filelog
>> hgweb: expose a followlines UI in filerevision view (RFC)
>>
>> In filerevision view (/file/<rev>/<fname>) we add some event listeners on
>> mouse selection of <span> elements in the <pre class="sourcelines"> block.
>> Those listeners will capture the range of mouse-selected lines and, upon
>> mouse
>> release, a box inviting to follow the history of selected lines will show
>> up.
>> This action is advertised by a :after pseudo-element on file lines that
>> shows
>> up on hover and invite to "select a block of lines to follow its history".
>>
>> All JavaScript code lives in a new "linerangelog.js" file, sourced in
>> filerevision template (only in "paper" style for now).
>>
>> This is proposal implementation, comments welcome on any aspects.
>>
> This feature is frigging awesome!!! I can pretty much guarantee that some
> engineers at Mozilla will go completely crazy for this feature.
>
> As I'm playing with it locally, I found a few paper cuts.
>
> First, on the initial revision introducing lines (last revision as rendered
> in hgweb), the diff is often (always?) empty. I can also see empty diffs in
> the intermediate changesets. For example, run this on mercurial/exchange.py
> and ask it for the line range of the _pushdiscovery(pushop) function. For
> this function, I get 2 empty diffs (revisions 233623d58c9a and
> ddb56e7e1b92). Highlighting _pushdiscoverychangeset() yields only an empty
> initial diff.
>
> Second, the highlighting UI is a bit confusing. It took me a few seconds to
> realize I had to actually hold down the mouse button and highlight the
> lines. I don't think users are accustomed to do this on text in web pages
> unless they are copying text. It was definitely surprising to me that
> highlighting lines of text resulted in a special pop-up appearing. I'm no
> web design expert, but how about this design.
>
> As you mouseover lines, you see a graphic cursor somewhere on that line.
> There is likely a label next to it saying "click anywhere to follow from
> this line" or something like that. This is similar to the floating text
> label you have now. When you mouse click, that floating cursor is dropped
> and locked into place on that line. On subsequent mouseovers, another
> cursor+label appears. The lines between the initial cursor and the current
> mouse location are highlighted somehow (possibly adding a different
> background color). The label says "click anywhere to follow lines XX to YY"
> or something. When the user clicks to drop the 2nd cursor, either they're
> taken directly to the filelog view or we get an inline box with links (like
> the line number mouseovers on the annotate page). If the user accidentally
> clicks to drop a cursor, they can clear the cursor be clicking the cursor
> or an "x" next to it.
>
> Another minor nitpick with the highlighting UI is the "follow lines" box
> may not appear next to the mouse cursor. I think we want it to appear near
> the cursor so it is easier to find/use.

Just sent a V6 of this patch implementing most of you proposal. I only
left aside the floating cursor and box ideas; the patch being already
quite large, I'd like to postpone this for a follow-up.

> The UX can obviously be improved as a follow-up. I don't want to detract
> from getting the core of the feature landed. This is shaping up to be
> pretty amazing!
>
>

Patch

diff --git a/contrib/wix/templates.wxs b/contrib/wix/templates.wxs
--- a/contrib/wix/templates.wxs
+++ b/contrib/wix/templates.wxs
@@ -225,6 +225,7 @@ 
             <File Id="static.coal.file.png"      Name="coal-file.png" />
             <File Id="static.coal.folder.png"    Name="coal-folder.png" />
             <File Id="static.excanvas.js"        Name="excanvas.js" />
+            <File Id="static.linerangelog.js"    Name="linerangelog.js" />
             <File Id="static.mercurial.js"       Name="mercurial.js" />
             <File Id="static.hgicon.png"         Name="hgicon.png" />
             <File Id="static.hglogo.png"         Name="hglogo.png" />
diff --git a/mercurial/templates/paper/filerevision.tmpl b/mercurial/templates/paper/filerevision.tmpl
--- a/mercurial/templates/paper/filerevision.tmpl
+++ b/mercurial/templates/paper/filerevision.tmpl
@@ -71,8 +71,11 @@ 
 <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 stripes4 wrap bottomline">{text%fileline}</pre>
+<pre class="sourcelines stripes4 wrap bottomline" data-logurl="{url|urlescape}log/{symrev}/{file|urlescape}">{text%fileline}</pre>
 </div>
+
+<script type="text/javascript" src="{staticurl|urlescape}linerangelog.js"></script>
+
 </div>
 </div>
 
diff --git a/mercurial/templates/static/linerangelog.js b/mercurial/templates/static/linerangelog.js
new file mode 100644
--- /dev/null
+++ b/mercurial/templates/static/linerangelog.js
@@ -0,0 +1,83 @@ 
+// Copyright 2017 Logilab SA <contact@logilab.fr>
+//
+// This software may be used and distributed according to the terms
+// of the GNU General Public License, incorporated herein by reference.
+
+document.addEventListener('DOMContentLoaded', function() {
+    // install mouse event listeners on <pre class="sourcelines">
+    var sourcelines = document.getElementsByClassName('sourcelines')[0];
+    if (typeof sourcelines === 'undefined') {
+        return;
+    }
+    var targetUri = sourcelines.dataset.logurl;
+    if (typeof targetUri === 'undefined') {
+        return;
+    }
+    // add event listener to the whole <pre class="sourcelines"> block to have
+    // it available in all children <span>
+    sourcelines.addEventListener('mousedown', lineSelectStart);
+
+    //** event handler for "mousedown" */
+    function lineSelectStart(e) {
+        var startElement = e.target;
+        if (startElement.tagName !== 'SPAN') {
+            // we may be on a <a>
+            return;
+        }
+        // retarget "mouseup" to sourcelines element so that if this event
+        // occurs outside the <pre> we don't miss it
+        sourcelines.setCapture();
+        // drop any prior <div id="followlines">
+        var previousInvite = document.getElementById('followlines');
+        if (previousInvite !== null) {
+            previousInvite.parentNode.removeChild(previousInvite);
+        }
+
+        var startId = parseInt(startElement.id.slice(1));
+
+        //** event handler for "mouseup" */
+        function lineSelectEnd(e) {
+            // remove "mouseup" listener
+            sourcelines.removeEventListener('mouseup', lineSelectEnd);
+
+            var endElement = e.target;
+            if (endElement.tagName !== 'SPAN') {
+                // not on <span>, probably outside <pre class="sourcelines">,
+                // we cannot compute endId
+                return;
+            }
+
+            // compute line range (startId, endId) and insert the
+            // "followlines" element
+            var endId = parseInt(endElement.id.slice(1));
+            if (endId == startId) {
+                return;
+            }
+            var inviteElement = endElement;
+            if (endId < startId) {
+                var tmp = endId;
+                endId = startId;
+                startId = tmp;
+                inviteElement = startElement;
+            }
+            var div = followlinesBox(startId, endId);
+            inviteElement.appendChild(div);
+        }
+
+        sourcelines.addEventListener('mouseup', lineSelectEnd);
+
+    }
+
+    //** return a <div id="followlines"> with a link for followlines action */
+    function followlinesBox(startId, endId) {
+        var div = document.createElement('div');
+        div.id = 'followlines';
+        var a = document.createElement('a');
+        var url = targetUri + '?patch=&linerange=' + startId + ':' + endId;
+        a.setAttribute('href', url);
+        a.textContent = 'follow lines ' + startId + ':' + endId;
+        div.appendChild(a);
+        return div;
+    }
+
+}, false);
diff --git a/mercurial/templates/static/style-paper.css b/mercurial/templates/static/style-paper.css
--- a/mercurial/templates/static/style-paper.css
+++ b/mercurial/templates/static/style-paper.css
@@ -276,10 +276,28 @@  td.annotate:hover div.annotate-info { di
   float: left;
 }
 
+div.overflow pre.sourcelines > span:hover:after {
+  content: "select a block of lines to follow its history";
+  display: inline;
+  float: right;
+  line-height: 1em;
+  background-color: #FFFFFF;
+  color: #001199;
+}
+
 .sourcelines > span:target, tr:target td {
   background-color: #bfdfff;
 }
 
+div#followlines {
+  display: inline;
+  position: absolute;
+  background-color: #FFFFFF;
+  border: 1px solid #999;
+  color: #000000;
+  padding: 2px;
+}
+
 .sourcelines > a {
     display: inline-block;
     position: absolute;
diff --git a/tests/test-hgweb-commands.t b/tests/test-hgweb-commands.t
--- a/tests/test-hgweb-commands.t
+++ b/tests/test-hgweb-commands.t
@@ -1343,9 +1343,12 @@  File-related
   <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 stripes4 wrap bottomline">
+  <pre class="sourcelines stripes4 wrap bottomline" data-logurl="/log/1/foo">
   <span id="l1">foo</span><a href="#l1"></a></pre>
   </div>
+  
+  <script type="text/javascript" src="/static/linerangelog.js"></script>
+  
   </div>
   </div>
   
@@ -1468,9 +1471,12 @@  File-related
   <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 stripes4 wrap bottomline">
+  <pre class="sourcelines stripes4 wrap bottomline" data-logurl="/log/2/foo">
   <span id="l1">another</span><a href="#l1"></a></pre>
   </div>
+  
+  <script type="text/javascript" src="/static/linerangelog.js"></script>
+  
   </div>
   </div>