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