Patchwork [4,of,4,V6] hgweb: add links to diff and changeset views from annotate view table

login
register
mail settings
Submitter Denis Laxalde
Date June 16, 2016, 1:58 p.m.
Message ID <b4dc235a8f7e30495b91.1466085499@sh77.tls.logilab.fr>
Download mbox | patch
Permalink /patch/15530/
State Changes Requested
Delegated to: Pierre-Yves David
Headers show

Comments

Denis Laxalde - June 16, 2016, 1:58 p.m.
# HG changeset patch
# User Denis Laxalde <denis.laxalde@logilab.fr>
# Date 1465894890 -7200
#      Tue Jun 14 11:01:30 2016 +0200
# Node ID b4dc235a8f7e30495b91c7e07602e871a5c328b6
# Parent  385edba1c6803dc008fd71a6066a5a6afdc4255a
hgweb: add links to diff and changeset views from annotate view table

Links appear upon hover of the annotated revision, above the links to parents.
Matt Mackall - June 22, 2016, 7:21 p.m.
On Thu, 2016-06-16 at 15:58 +0200, Denis Laxalde wrote:
> # HG changeset patch
> # User Denis Laxalde <denis.laxalde@logilab.fr>
> # Date 1465894890 -7200
> #      Tue Jun 14 11:01:30 2016 +0200
> # Node ID b4dc235a8f7e30495b91c7e07602e871a5c328b6
> # Parent  385edba1c6803dc008fd71a6066a5a6afdc4255a
> hgweb: add links to diff and changeset views from annotate view table

I've queued the first two here, and I'm pretty excited by the next two, but I'm
a little bothered by what I'm seeing with the hover-over on paper:

- there are two hover-overs
- the hover-overs are attached to different elements (?)
- they can overlap
- they look completely different
- the new one has link underlines, which paper avoids
- the styles for the diff / rev / parent foo@rev lines are inconsistent

I think we should have one hover-over that uses the existing hover-over style
and looks like this:

  abcdef1234: some change description
  parent diff changeset

We don't really need to know the parent revision before we click on it, and it's
easily the most useful of the three links so it should be on the left. Change
"rev" to "changeset" to match the menu on the left.

-- 
Mathematics is the supreme nostalgia of our time.
Gregory Szorc - June 25, 2016, 4:36 p.m.
On Thu, Jun 16, 2016 at 6:58 AM, Denis Laxalde <denis.laxalde@logilab.fr>
wrote:

> # HG changeset patch
> # User Denis Laxalde <denis.laxalde@logilab.fr>
> # Date 1465894890 -7200
> #      Tue Jun 14 11:01:30 2016 +0200
> # Node ID b4dc235a8f7e30495b91c7e07602e871a5c328b6
> # Parent  385edba1c6803dc008fd71a6066a5a6afdc4255a
> hgweb: add links to diff and changeset views from annotate view table
>
> Links appear upon hover of the annotated revision, above the links to
> parents.
>

I *really* like where these patches are going!

Recently, someone at Mozilla wrote their own code indexing tool which
includes a better blame view. You can see it in action at
http://searchfox.org/mozilla-central/source/mach. The code for this tool is
at https://github.com/bill-mccloskey/searchfox. They either independently
came up with many of the same ideas Mercurial is planning or stole them.
Either way, you can see the interface in action and it provides a good
point of reference for these hgweb improvements. I'll also reference GitHub
(https://github.com/mozilla/gecko-dev/blame/master/mach) and Bitbucket for
comparison.

Anyway, here are my comments on the last 2 patches in this series:

* Now that we only render the blame info link once per block, you have to
move your mouse or potentially scroll to the first line of the block get
the hover info box. I prefer searchfox's interface where hovering anywhere
in that blame block shows the hover box because it is easier to use. GitHub
and BB are also more usable here in that all data is pre-rendered in each
block so no extra work to find element to hover over.

* I *really* like having the summary line of the commit message accessible
(searchfox and GitHub do this).

* Ditto for the full author name (if we moved author name to the hover box
we could arguably remove it from the column and do away with our variable
width badness - but this is arguably for a follow-up).

* I feel like we need better styling on the hover box. Something like the
changeset info from the top of the annotate page would be great. (Basically
what mpm said.)

* To cut down on HTML size and DOM bloat, you may want to consider encoding
info in a "data" attribute and using JS to dynamically create the hover
boxes. Searchfox even uses XHR to access this data (although we probably
don't want to introduce XHR just yet).

* I wish there were more padding between the text in the left column and
the hover box. There are only a few pixels currently and it is really
cramped. Searchfox gets this right.

* I think the link to the parent annotation should go to the
currently-hovered line. You can also make the argument for having 2 links:
top of file and current line. I think current line is more important
because annotate is almost always used to see how specific parts of the
file evolved over time and we want the lines/block under examination to be
front and center when changing revisions. Searchfox's "show latest version
without this line" gets this right.

* It wasn't obvious to me that "diff" is a file diff as opposed to the full
changeset diff. Although I suppose in the context of a per-file annotate
view that "diff" should implicitly mean "file diff." I think what threw me
off was the link to the changeset being right next to the "diff" link.

* Having looked at Searchfox, I'm questioning whether any text content in
the leftmost column are warranted. Author could be moved to hover (see
bullet point above). And integer revisions aren't particularly useful (I
thought we were supposed to avoid integer revisions in hgweb since they
aren't consistent). If we put hashes there, we lose the value of knowing
N+1 is newer than N. So since nothing really useful can go there, why have
anything at all? Following the same logic, we could potentially replace the
existing text with links to important revisions for this line and use the
hover to show changeset info. Something to think about.

Again, this is looking really nice. hgweb is shaping up to be better than
Gitweb, GitHub, Bitbucket, Phabricator's Diffusion, and many other HTML
source viewers for showing annotate info!
Denis Laxalde - July 12, 2016, 3:38 p.m.
Matt Mackall a écrit :
> On Thu, 2016-06-16 at 15:58 +0200, Denis Laxalde wrote:
>> # HG changeset patch
>> # User Denis Laxalde <denis.laxalde@logilab.fr>
>> # Date 1465894890 -7200
>> #      Tue Jun 14 11:01:30 2016 +0200
>> # Node ID b4dc235a8f7e30495b91c7e07602e871a5c328b6
>> # Parent  385edba1c6803dc008fd71a6066a5a6afdc4255a
>> hgweb: add links to diff and changeset views from annotate view table
>
> I've queued the first two here, and I'm pretty excited by the next two, but I'm
> a little bothered by what I'm seeing with the hover-over on paper:
>
> - there are two hover-overs
> - the hover-overs are attached to different elements (?)
> - they can overlap
> - they look completely different
> - the new one has link underlines, which paper avoids
> - the styles for the diff / rev / parent foo@rev lines are inconsistent
>
> I think we should have one hover-over that uses the existing hover-over style
> and looks like this:
>
>   abcdef1234: some change description
>   parent diff changeset
>
> We don't really need to know the parent revision before we click on it, and it's
> easily the most useful of the three links so it should be on the left. Change
> "rev" to "changeset" to match the menu on the left.

The new series just submitted includes all suggested changes.
Denis Laxalde - July 12, 2016, 3:56 p.m.
(Following-up on this discussion after submission of a new series.)

Gregory Szorc a écrit :
> Anyway, here are my comments on the last 2 patches in this series:
>
> * Now that we only render the blame info link once per block, you have
> to move your mouse or potentially scroll to the first line of the block
> get the hover info box. I prefer searchfox's interface where hovering
> anywhere in that blame block shows the hover box because it is easier to
> use. GitHub and BB are also more usable here in that all data is
> pre-rendered in each block so no extra work to find element to hover over.

I've implemented this idea. Now the hover-box shows up whenever one
hovers the first column and the box contains the same link as the head
of the "same-revision-block". It's kind of a hidden feature at this
point, maybe it could be made clearer that there's information on this
column even if there's no text in the cell...

> * I *really* like having the summary line of the commit message
> accessible (searchfox and GitHub do this).
>
> * Ditto for the full author name (if we moved author name to the hover
> box we could arguably remove it from the column and do away with our
> variable width badness - but this is arguably for a follow-up).

So I included the full author information in the hover-box and removed
it from the link in the first column (which thus only consists of the
"rev").

> * I feel like we need better styling on the hover box. Something like
> the changeset info from the top of the annotate page would be great.
> (Basically what mpm said.)

Done.

> * I wish there were more padding between the text in the left column and
> the hover box. There are only a few pixels currently and it is really
> cramped. Searchfox gets this right.

Done.

> * I think the link to the parent annotation should go to the
> currently-hovered line. You can also make the argument for having 2
> links: top of file and current line. I think current line is more
> important because annotate is almost always used to see how specific
> parts of the file evolved over time and we want the lines/block under
> examination to be front and center when changing revisions. Searchfox's
> "show latest version without this line" gets this right.

But, unless I'm mistaken, the line information that we have on the
"annotated" revision may not be relevant on its parent so it seems to me
that this would confuse navigation in many cases.

> * Having looked at Searchfox, I'm questioning whether any text content
> in the leftmost column are warranted. Author could be moved to hover
> (see bullet point above). And integer revisions aren't particularly
> useful (I thought we were supposed to avoid integer revisions in hgweb
> since they aren't consistent). If we put hashes there, we lose the value
> of knowing N+1 is newer than N. So since nothing really useful can go
> there, why have anything at all? Following the same logic, we could
> potentially replace the existing text with links to important revisions
> for this line and use the hover to show changeset info. Something to
> think about.

I agree that we could reconsider the information in the first column. If
we go and drop the revision number, we'd still need a visual hint to
clearly identify identical revisions on the left column. Using colors
like searchfox/github looks like a good idea.

Patch

diff --git a/mercurial/templates/gitweb/map b/mercurial/templates/gitweb/map
--- a/mercurial/templates/gitweb/map
+++ b/mercurial/templates/gitweb/map
@@ -101,6 +101,10 @@  annotateline = '
           '<a href="{url|urlescape}annotate/{node|short}/{file|urlescape}{sessionvars%urlparameter}#l{targetline}"
               title="{node|short}: {desc|escape|firstline}">{author|user}@{rev}</a>
            <div class="annotate-info">
+              <a href="{url|urlescape}diff/{node|short}/{file|urlescape}{sessionvars%urlparameter}"
+                 title="diff {file|urlescape}@{node|short}">diff</a>
+              <a href="{url|urlescape}rev/{node|short}{sessionvars%urlparameter}"
+                 title="{node|short}: {desc|escape|firstline}">rev</a>
               {parents%annotateparent}
            </div>',
           '')}
diff --git a/mercurial/templates/monoblue/map b/mercurial/templates/monoblue/map
--- a/mercurial/templates/monoblue/map
+++ b/mercurial/templates/monoblue/map
@@ -97,6 +97,10 @@  annotateline = '
           '<a href="{url|urlescape}annotate/{node|short}/{file|urlescape}{sessionvars%urlparameter}#l{targetline}"
               title="{node|short}: {desc|escape|firstline}">{author|user}@{rev}</a>
            <div class="annotate-info">
+              <a href="{url|urlescape}diff/{node|short}/{file|urlescape}{sessionvars%urlparameter}"
+                 title="diff {file|urlescape}@{node|short}">diff</a>
+              <a href="{url|urlescape}rev/{node|short}{sessionvars%urlparameter}"
+                 title="{node|short}: {desc|escape|firstline}">rev</a>
              {parents%annotateparent}
            </div>',
           '')}
diff --git a/mercurial/templates/paper/map b/mercurial/templates/paper/map
--- a/mercurial/templates/paper/map
+++ b/mercurial/templates/paper/map
@@ -82,6 +82,10 @@  annotateline = '
           '<a href="{url|urlescape}annotate/{node|short}/{file|urlescape}{sessionvars%urlparameter}#l{targetline}"
               title="{node|short}: {desc|escape|firstline}">{author|user}@{rev}</a>
            <div class="annotate-info">
+              <a href="{url|urlescape}diff/{node|short}/{file|urlescape}{sessionvars%urlparameter}"
+                 title="diff {file|urlescape}@{node|short}">diff</a>
+              <a href="{url|urlescape}rev/{node|short}{sessionvars%urlparameter}"
+                 title="{node|short}: {desc|escape|firstline}">rev</a>
              {parents%annotateparent}
            </div>',
           '')}
diff --git a/mercurial/templates/spartan/map b/mercurial/templates/spartan/map
--- a/mercurial/templates/spartan/map
+++ b/mercurial/templates/spartan/map
@@ -60,6 +60,10 @@  annotateline = '
           '<a href="{url|urlescape}annotate/{node|short}/{file|urlescape}{sessionvars%urlparameter}#l{targetline}"
               title="{node|short}: {desc|escape|firstline}">{author|user}@{rev}</a>
            <div class="annotate-info">
+              <a href="{url|urlescape}diff/{node|short}/{file|urlescape}{sessionvars%urlparameter}"
+                 title="diff {file|urlescape}@{node|short}">diff</a>
+              <a href="{url|urlescape}rev/{node|short}{sessionvars%urlparameter}"
+                 title="{node|short}: {desc|escape|firstline}">rev</a>
              {parents%annotateparent}
            </div>',
           '')}
diff --git a/tests/test-hgweb-symrev.t b/tests/test-hgweb-symrev.t
--- a/tests/test-hgweb-symrev.t
+++ b/tests/test-hgweb-symrev.t
@@ -191,7 +191,11 @@  Set up the repo
    <td class="author"><a href="/file/43c799df6e75/foo?style=paper">43c799df6e75</a> </td>
    <td class="author"><a href="/file/9d8c40cba617/foo?style=paper">9d8c40cba617</a> </td>
   <a href="/annotate/43c799df6e75/foo?style=paper#l1"
+  <a href="/diff/43c799df6e75/foo?style=paper"
+  <a href="/rev/43c799df6e75?style=paper"
   <a href="/annotate/a7c1559b7bba/foo?style=paper#l2"
+  <a href="/diff/a7c1559b7bba/foo?style=paper"
+  <a href="/rev/a7c1559b7bba?style=paper"
   <div>parent: <a href="/annotate/43c799df6e75/foo?style=paper"
 
   $ "$TESTDIR/get-with-headers.py" 127.0.0.1:$HGPORT 'diff/xyzzy/foo?style=paper' | egrep $REVLINKS
@@ -380,7 +384,11 @@  Set up the repo
    <td class="author"><a href="/file/43c799df6e75/foo?style=coal">43c799df6e75</a> </td>
    <td class="author"><a href="/file/9d8c40cba617/foo?style=coal">9d8c40cba617</a> </td>
   <a href="/annotate/43c799df6e75/foo?style=coal#l1"
+  <a href="/diff/43c799df6e75/foo?style=coal"
+  <a href="/rev/43c799df6e75?style=coal"
   <a href="/annotate/a7c1559b7bba/foo?style=coal#l2"
+  <a href="/diff/a7c1559b7bba/foo?style=coal"
+  <a href="/rev/a7c1559b7bba?style=coal"
   <div>parent: <a href="/annotate/43c799df6e75/foo?style=coal"
 
   $ "$TESTDIR/get-with-headers.py" 127.0.0.1:$HGPORT 'diff/xyzzy/foo?style=coal' | egrep $REVLINKS
@@ -619,7 +627,11 @@  Set up the repo
   <a class="list" href="/annotate/43c799df6e75/foo?style=gitweb">
   <a class="list" href="/annotate/9d8c40cba617/foo?style=gitweb">9d8c40cba617</a></td>
   <a href="/annotate/43c799df6e75/foo?style=gitweb#l1"
+  <a href="/diff/43c799df6e75/foo?style=gitweb"
+  <a href="/rev/43c799df6e75?style=gitweb"
   <a href="/annotate/a7c1559b7bba/foo?style=gitweb#l2"
+  <a href="/diff/a7c1559b7bba/foo?style=gitweb"
+  <a href="/rev/a7c1559b7bba?style=gitweb"
   <div>parent: <a href="/annotate/43c799df6e75/foo?style=gitweb"
 
   $ "$TESTDIR/get-with-headers.py" 127.0.0.1:$HGPORT 'diff/xyzzy/foo?style=gitweb' | egrep $REVLINKS
@@ -836,7 +848,11 @@  Set up the repo
   <a href="/annotate/43c799df6e75/foo?style=monoblue">
   <a href="/annotate/9d8c40cba617/foo?style=monoblue">9d8c40cba617</a>
   <a href="/annotate/43c799df6e75/foo?style=monoblue#l1"
+  <a href="/diff/43c799df6e75/foo?style=monoblue"
+  <a href="/rev/43c799df6e75?style=monoblue"
   <a href="/annotate/a7c1559b7bba/foo?style=monoblue#l2"
+  <a href="/diff/a7c1559b7bba/foo?style=monoblue"
+  <a href="/rev/a7c1559b7bba?style=monoblue"
   <div>parent: <a href="/annotate/43c799df6e75/foo?style=monoblue"
 
   $ "$TESTDIR/get-with-headers.py" 127.0.0.1:$HGPORT 'diff/xyzzy/foo?style=monoblue' | egrep $REVLINKS
@@ -1034,7 +1050,11 @@  Set up the repo
   <a href="/annotate/43c799df6e75/foo?style=spartan">
   <td><a href="/annotate/9d8c40cba617/foo?style=spartan">9d8c40cba617</a></td>
   <a href="/annotate/43c799df6e75/foo?style=spartan#l1"
+  <a href="/diff/43c799df6e75/foo?style=spartan"
+  <a href="/rev/43c799df6e75?style=spartan"
   <a href="/annotate/a7c1559b7bba/foo?style=spartan#l2"
+  <a href="/diff/a7c1559b7bba/foo?style=spartan"
+  <a href="/rev/a7c1559b7bba?style=spartan"
   <div>parent: <a href="/annotate/43c799df6e75/foo?style=spartan"
 
   $ "$TESTDIR/get-with-headers.py" 127.0.0.1:$HGPORT 'diff/xyzzy/foo?style=spartan' | egrep $REVLINKS
diff --git a/tests/test-highlight.t b/tests/test-highlight.t
--- a/tests/test-highlight.t
+++ b/tests/test-highlight.t
@@ -293,6 +293,10 @@  hgweb fileannotate, html
   <a href="/annotate/06824edf55d0/primes.py#l1"
   title="06824edf55d0: a">test@0</a>
   <div class="annotate-info">
+  <a href="/diff/06824edf55d0/primes.py"
+  title="diff primes.py@06824edf55d0">diff</a>
+  <a href="/rev/06824edf55d0"
+  title="06824edf55d0: a">rev</a>
   
   </div>
   </td>