Patchwork [2,of,4] hgweb: calculate <canvas> width and height client-side

login
register
mail settings
Submitter Anton Shestakov
Date Dec. 12, 2017, 4:27 p.m.
Message ID <2319d0216460c6f3b91f.1513096067@neuro>
Download mbox | patch
Permalink /patch/26238/
State Accepted
Headers show

Comments

Anton Shestakov - Dec. 12, 2017, 4:27 p.m.
# HG changeset patch
# User Anton Shestakov <av6@dwimlabs.net>
# Date 1512892582 -28800
#      Sun Dec 10 15:56:22 2017 +0800
# Node ID 2319d0216460c6f3b91f174a5027190aa80e07b7
# Parent  e35959da063b5944369277f3b2c69c3af06e2710
hgweb: calculate <canvas> width and height client-side

hgweb determines and passes to templates some variables related to graph
appearance, like bg_height, canvaswidth and canvasheight. bg_height was and
still is used for graph.scale() call in graph.tmpl, and the two latter
variables were used in <canvas> element as width and height properties, and
they were set before JS code got to run. Setting these properties server-side
doesn't make a lot of sense, because a graph that has been scaled should
calculate things like width and height on its own when being rendered.

Let's move (re)sizing <canvas> to JavaScript (to Graph.render function) and
stop parsing HTML with regular expressions just to know new width and height.
That extra loop that only counts cols is required because <canvas> can't
be resized after or in the process of rendering (or it gets cleared).
Incidentally, SVG doesn't have this problem and I'm hoping to switch graph to
using it in future.

There also was truecanvasheight, but according to hg grep --all it was never
used, see d490edc71146.
Yuya Nishihara - Dec. 14, 2017, 12:07 p.m.
On Wed, 13 Dec 2017 00:27:47 +0800, Anton Shestakov wrote:
> # HG changeset patch
> # User Anton Shestakov <av6@dwimlabs.net>
> # Date 1512892582 -28800
> #      Sun Dec 10 15:56:22 2017 +0800
> # Node ID 2319d0216460c6f3b91f174a5027190aa80e07b7
> # Parent  e35959da063b5944369277f3b2c69c3af06e2710
> hgweb: calculate <canvas> width and height client-side
> 
> hgweb determines and passes to templates some variables related to graph
> appearance, like bg_height, canvaswidth and canvasheight. bg_height was and
> still is used for graph.scale() call in graph.tmpl, and the two latter
> variables were used in <canvas> element as width and height properties, and
> they were set before JS code got to run. Setting these properties server-side
> doesn't make a lot of sense, because a graph that has been scaled should
> calculate things like width and height on its own when being rendered.
> 
> Let's move (re)sizing <canvas> to JavaScript (to Graph.render function) and
> stop parsing HTML with regular expressions just to know new width and height.

I'm okay with this change, but it might be bad in UX as the resize occurs
very late.
Anton Shestakov - Dec. 14, 2017, 1:10 p.m.
On Thu, 14 Dec 2017 21:07:38 +0900
Yuya Nishihara <yuya@tcha.org> wrote:

> On Wed, 13 Dec 2017 00:27:47 +0800, Anton Shestakov wrote:
> > # HG changeset patch
> > # User Anton Shestakov <av6@dwimlabs.net>
> > # Date 1512892582 -28800
> > #      Sun Dec 10 15:56:22 2017 +0800
> > # Node ID 2319d0216460c6f3b91f174a5027190aa80e07b7
> > # Parent  e35959da063b5944369277f3b2c69c3af06e2710
> > hgweb: calculate <canvas> width and height client-side
> > 
> > hgweb determines and passes to templates some variables related to graph
> > appearance, like bg_height, canvaswidth and canvasheight. bg_height was and
> > still is used for graph.scale() call in graph.tmpl, and the two latter
> > variables were used in <canvas> element as width and height properties, and
> > they were set before JS code got to run. Setting these properties server-side
> > doesn't make a lot of sense, because a graph that has been scaled should
> > calculate things like width and height on its own when being rendered.
> > 
> > Let's move (re)sizing <canvas> to JavaScript (to Graph.render function) and
> > stop parsing HTML with regular expressions just to know new width and height.  
> 
> I'm okay with this change, but it might be bad in UX as the resize occurs
> very late.

I don't think the way it's resized now would cause any problems:

- <canvas> is an absolutely-positioned element that is simply stacked on
  top of the list of commits, which means the rest of the page doesn't
  really depend on its size,

- it is also fully transparent initially and after any resizing, so its
  size is not something that can be easily seen without devtools,

- and most importantly, while the way JS code updates the page has been
  changed from

    resize <canvas>, copy new HTML, render on <canvas>

  to
 
    copy new HTML, resize <canvas>, render on <canvas>

  DOM updates still happen only when scripting has finished the
  execution (if it's synchronous, like in our case), so the result can
  only be seen after all three "functions" complete anyway,

I think even if we switched to SVG and did the resizing part at the
very end, it would still be only one "visible" update to the page
(I'll do some experiments when I actually try to do graph in SVG).
Yuya Nishihara - Dec. 14, 2017, 2:01 p.m.
On Thu, 14 Dec 2017 21:10:06 +0800, Anton Shestakov wrote:
> On Thu, 14 Dec 2017 21:07:38 +0900
> Yuya Nishihara <yuya@tcha.org> wrote:
> > I'm okay with this change, but it might be bad in UX as the resize occurs
> > very late.
> 
> I don't think the way it's resized now would cause any problems:
> 
> - <canvas> is an absolutely-positioned element that is simply stacked on
>   top of the list of commits, which means the rest of the page doesn't
>   really depend on its size,
> 
> - it is also fully transparent initially and after any resizing, so its
>   size is not something that can be easily seen without devtools,
> 
> - and most importantly, while the way JS code updates the page has been
>   changed from
> 
>     resize <canvas>, copy new HTML, render on <canvas>
> 
>   to
>  
>     copy new HTML, resize <canvas>, render on <canvas>
> 
>   DOM updates still happen only when scripting has finished the
>   execution (if it's synchronous, like in our case), so the result can
>   only be seen after all three "functions" complete anyway,
> 
> I think even if we switched to SVG and did the resizing part at the
> very end, it would still be only one "visible" update to the page
> (I'll do some experiments when I actually try to do graph in SVG).

Well, I see difference on Firefox 57. Perhaps the initial padding-left of
data-node had a reasonable value before.
Anton Shestakov - Dec. 14, 2017, 2:58 p.m.
On Thu, 14 Dec 2017 23:01:18 +0900
Yuya Nishihara <yuya@tcha.org> wrote:

> On Thu, 14 Dec 2017 21:10:06 +0800, Anton Shestakov wrote:
> > On Thu, 14 Dec 2017 21:07:38 +0900
> > Yuya Nishihara <yuya@tcha.org> wrote:  
> > > I'm okay with this change, but it might be bad in UX as the resize occurs
> > > very late.  
> > 
> > I don't think the way it's resized now would cause any problems:
> > 
> > - <canvas> is an absolutely-positioned element that is simply stacked on
> >   top of the list of commits, which means the rest of the page doesn't
> >   really depend on its size,
> > 
> > - it is also fully transparent initially and after any resizing, so its
> >   size is not something that can be easily seen without devtools,
> > 
> > - and most importantly, while the way JS code updates the page has been
> >   changed from
> > 
> >     resize <canvas>, copy new HTML, render on <canvas>
> > 
> >   to
> >  
> >     copy new HTML, resize <canvas>, render on <canvas>
> > 
> >   DOM updates still happen only when scripting has finished the
> >   execution (if it's synchronous, like in our case), so the result can
> >   only be seen after all three "functions" complete anyway,
> > 
> > I think even if we switched to SVG and did the resizing part at the
> > very end, it would still be only one "visible" update to the page
> > (I'll do some experiments when I actually try to do graph in SVG).  
> 
> Well, I see difference on Firefox 57. Perhaps the initial padding-left of
> data-node had a reasonable value before.

Looks like browsers can sometimes manage to display a page after the
changesets HTML is downloaded, but before graph code gets to run, which
means that for a moment people can see /graph like if they had JS
disabled. At least there's an easy way to reproduce it.

I think the best way to deal with this is to add initial padding-left
(there wasn't before) and render backgrounds too. But looks like graph
code is already in the best spot to run as soon as possible. There's
always an option to just put display:hidden on everything initially and
remove it in graph.render(), which would emulate the old "do everything
in JS"-style graph, but I'm not going to resort to it yet.

Patch

diff --git a/mercurial/hgweb/webcommands.py b/mercurial/hgweb/webcommands.py
--- a/mercurial/hgweb/webcommands.py
+++ b/mercurial/hgweb/webcommands.py
@@ -1231,13 +1231,6 @@  def graph(web, req, tmpl):
         tree = list(item for item in graphmod.colored(dag, web.repo)
                     if item[1] == graphmod.CHANGESET)
 
-    def getcolumns(tree):
-        cols = 0
-        for (id, type, ctx, vtx, edges) in tree:
-            cols = max(cols, max([edge[0] for edge in edges] or [0]),
-                             max([edge[1] for edge in edges] or [0]))
-        return cols
-
     def graphdata(usetuples):
         data = []
 
@@ -1266,17 +1259,14 @@  def graph(web, req, tmpl):
 
         return data
 
-    cols = getcolumns(tree)
     rows = len(tree)
-    canvasheight = (rows + 1) * bg_height - 27
 
     return tmpl('graph', rev=rev, symrev=symrev, revcount=revcount,
                 uprev=uprev,
                 lessvars=lessvars, morevars=morevars, downrev=downrev,
-                cols=cols, rows=rows, changesets=count,
-                canvaswidth=(cols + 1) * bg_height,
-                truecanvasheight=rows * bg_height,
-                canvasheight=canvasheight, bg_height=bg_height,
+                rows=rows,
+                bg_height=bg_height,
+                changesets=count,
                 jsdata=lambda **x: graphdata(True),
                 nodes=lambda **x: graphdata(False),
                 node=ctx.hex(), changenav=changenav)
diff --git a/mercurial/templates/gitweb/graph.tmpl b/mercurial/templates/gitweb/graph.tmpl
--- a/mercurial/templates/gitweb/graph.tmpl
+++ b/mercurial/templates/gitweb/graph.tmpl
@@ -38,7 +38,7 @@  graph |
 
 <div id="wrapper">
 <ul id="nodebgs"></ul>
-<canvas id="graph" width="{canvaswidth}" height="{canvasheight}"></canvas>
+<canvas id="graph"></canvas>
 <ul id="graphnodes">{nodes%graphentry}</ul>
 </div>
 
diff --git a/mercurial/templates/monoblue/graph.tmpl b/mercurial/templates/monoblue/graph.tmpl
--- a/mercurial/templates/monoblue/graph.tmpl
+++ b/mercurial/templates/monoblue/graph.tmpl
@@ -30,7 +30,7 @@ 
     <div id="noscript">The revision graph only works with JavaScript-enabled browsers.</div>
     <div id="wrapper">
         <ul id="nodebgs"></ul>
-        <canvas id="graph" width="{canvaswidth}" height="{canvasheight}"></canvas>
+        <canvas id="graph"></canvas>
         <ul id="graphnodes">{nodes%graphentry}</ul>
     </div>
 
diff --git a/mercurial/templates/paper/graph.tmpl b/mercurial/templates/paper/graph.tmpl
--- a/mercurial/templates/paper/graph.tmpl
+++ b/mercurial/templates/paper/graph.tmpl
@@ -51,7 +51,7 @@ 
 
 <div id="wrapper">
 <ul id="nodebgs" class="stripes2"></ul>
-<canvas id="graph" width="{canvaswidth}" height="{canvasheight}"></canvas>
+<canvas id="graph"></canvas>
 <ul id="graphnodes">{nodes%graphentry}</ul>
 </div>
 
diff --git a/mercurial/templates/spartan/graph.tmpl b/mercurial/templates/spartan/graph.tmpl
--- a/mercurial/templates/spartan/graph.tmpl
+++ b/mercurial/templates/spartan/graph.tmpl
@@ -32,7 +32,7 @@  navigate: <small class="navigate">{chang
 
 <div id="wrapper">
 <ul id="nodebgs"></ul>
-<canvas id="graph" width="{canvaswidth}" height="{canvasheight}"></canvas>
+<canvas id="graph"></canvas>
 <ul id="graphnodes">{nodes%graphentry}</ul>
 </div>
 
diff --git a/mercurial/templates/static/mercurial.js b/mercurial/templates/static/mercurial.js
--- a/mercurial/templates/static/mercurial.js
+++ b/mercurial/templates/static/mercurial.js
@@ -111,19 +111,30 @@  Graph.prototype = {
 
 		var backgrounds = '';
 		var nodedata = '';
-		var line, start, end, color, x, y, x0, y0, x1, y1, column, radius;
+		var i, j, cur, line, start, end, color, x, y, x0, y0, x1, y1, column, radius;
 
-		for (var i = 0; i < data.length; i++) {
+		var cols = 0;
+		for (i = 0; i < data.length; i++) {
+			cur = data[i];
+			for (j = 0; j < cur.edges.length; j++) {
+				line = cur.edges[j];
+				cols = Math.max(cols, line[0], line[1]);
+			}
+		}
+		this.canvas.width = (cols + 1) * this.bg_height;
+		this.canvas.height = (data.length + 1) * this.bg_height - 27;
+
+		for (i = 0; i < data.length; i++) {
 
 			var parity = i % 2;
 			this.cell[1] += this.bg_height;
 			this.bg[1] += this.bg_height;
 
-			var cur = data[i];
+			cur = data[i];
 			var fold = false;
 
 			var prevWidth = this.ctx.lineWidth;
-			for (var j = 0; j < cur.edges.length; j++) {
+			for (j = 0; j < cur.edges.length; j++) {
 
 				line = cur.edges[j];
 				start = line[0];
@@ -413,14 +424,6 @@  function ajaxScrollInit(urlFormat,
 
                     if (mode === 'graph') {
                         var graph = window.graph;
-                        var sizes = htmlText.match(/^\s*<canvas id="graph" width="(\d+)" height="(\d+)"><\/canvas>$/m);
-                        var addWidth = sizes[1];
-                        var addHeight = sizes[2];
-                        addWidth = parseInt(addWidth);
-                        addHeight = parseInt(addHeight);
-                        graph.canvas.width = addWidth;
-                        graph.canvas.height = addHeight;
-
                         var dataStr = htmlText.match(/^\s*var data = (.*);$/m)[1];
                         var data = JSON.parse(dataStr);
                         if (data.length < nextPageVar) {
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
@@ -1781,7 +1781,7 @@  Overviews
   
   <div id="wrapper">
   <ul id="nodebgs"></ul>
-  <canvas id="graph" width="39" height="168"></canvas>
+  <canvas id="graph"></canvas>
   <ul id="graphnodes"><li data-node="cad8025a2e87">
    <span class="desc">
     <a class="list" href="/rev/cad8025a2e87?style=gitweb"><b>branch commit with null character: </b></a>
diff --git a/tests/test-hgweb-empty.t b/tests/test-hgweb-empty.t
--- a/tests/test-hgweb-empty.t
+++ b/tests/test-hgweb-empty.t
@@ -295,7 +295,7 @@  Some tests for hgweb in an empty reposit
   
   <div id="wrapper">
   <ul id="nodebgs" class="stripes2"></ul>
-  <canvas id="graph" width="39" height="12"></canvas>
+  <canvas id="graph"></canvas>
   <ul id="graphnodes"></ul>
   </div>