[PATCH 7 of 7 V5] hgweb: expose a followlines UI in filerevision view (RFC)
Gregory Szorc
gregory.szorc at gmail.com
Sat Mar 25 15:46:26 EDT 2017
On Sat, Mar 25, 2017 at 12:34 PM, Gregory Szorc <gregory.szorc at gmail.com>
wrote:
> On Sat, Mar 25, 2017 at 2:21 AM, Denis Laxalde <denis at laxalde.org> wrote:
>
>> # HG changeset patch
>> # User Denis Laxalde <denis.laxalde at 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 at 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 at mercurial-scm.org
>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20170325/2161e748/attachment.html>
More information about the Mercurial-devel
mailing list