On Sat, Mar 25, 2017 at 12:34 PM, Gregory Szorc <gregory.sz...@gmail.com> wrote:
> On Sat, Mar 25, 2017 at 2:21 AM, Denis Laxalde <de...@laxalde.org> wrote: > >> # HG changeset patch >> # User Denis Laxalde <denis.laxa...@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 <cont...@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