Merge lp:~mwhudson/loggerhead/ajaxy-revision-page into lp:loggerhead

[Michael Hudson-Doyle]

[Michael Hudson-Doyle (mwhudson)]

Hi Reviewer,

The main change this branch makes is to make the default revision page not show any diffs by default and having them loaded by the wonders of xhr.

I've put some effort in to making sure non-js users have a decent experience -- instead of xhr-ing a diff, when scripting is disabled the user is taken to a page that just contains the the diff for the file.

The code, particularly in revision_ui.py, is now much less than clear. I'll fix that in the next branch tomorrow -- basically if you think the new behaviour is good, I can then clean up the innards without changing behaviour tomorrow (perhaps it would have been better to clean up the innards then clean up the behaviour, but oh well).

[Paul Hummer (rockstar)]

This is very sexy!

review: Approve

=== modified file 'loggerhead/apps/branch.py'
--- loggerhead/apps/branch.py	2009-03-13 05:36:26 +0000
+++ loggerhead/apps/branch.py	2009-03-17 00:02:21 +0000
@@ -11,15 +11,16 @@
 from paste import httpexceptions
 
 from loggerhead.apps import static_app
+from loggerhead.controllers.annotate_ui import AnnotateUI
+from loggerhead.controllers.atom_ui import AtomUI
 from loggerhead.controllers.changelog_ui import ChangeLogUI
+from loggerhead.controllers.diff_ui import DiffUI
+from loggerhead.controllers.download_ui import DownloadUI
+from loggerhead.controllers.filediff_ui import FileDiffUI
 from loggerhead.controllers.inventory_ui import InventoryUI
-from loggerhead.controllers.annotate_ui import AnnotateUI
 from loggerhead.controllers.revision_ui import RevisionUI
 from loggerhead.controllers.revlog_ui import RevLogUI
-from loggerhead.controllers.atom_ui import AtomUI
-from loggerhead.controllers.download_ui import DownloadUI
 from loggerhead.controllers.search_ui import SearchUI
-from loggerhead.controllers.diff_ui import DiffUI
 from loggerhead.history import History
 from loggerhead import util
 
@@ -79,15 +80,16 @@
         return self._static_url_base + path
 
     controllers_dict = {
+        '+filediff': FileDiffUI,
+        '+revlog': RevLogUI,
         'annotate': AnnotateUI,
+        'atom': AtomUI,
         'changes': ChangeLogUI,
+        'diff': DiffUI,
+        'download': DownloadUI,
         'files': InventoryUI,
         'revision': RevisionUI,
-        '+revlog': RevLogUI,
-        'download': DownloadUI,
-        'atom': AtomUI,
         'search': SearchUI,
-        'diff': DiffUI,
         }
 
     def last_updated(self):
 
=== added file 'loggerhead/controllers/filediff_ui.py'
--- loggerhead/controllers/filediff_ui.py	1970-01-01 00:00:00 +0000
+++ loggerhead/controllers/filediff_ui.py	2009-03-17 06:43:34 +0000
@@ -0,0 +1,97 @@
+from StringIO import StringIO
+import urllib
+
+from bzrlib import diff
+from bzrlib import errors
+
+from loggerhead import util
+from loggerhead.controllers import TemplatedBranchView
+
+def _process_diff(difftext):
+    chunks = []
+    chunk = None
+    for line in difftext.splitlines():
+        if len(line) == 0:
+            continue
+        if line.startswith('+++ ') or line.startswith('--- '):
+            continue
+        if line.startswith('@@ '):
+            # new chunk
+            if chunk is not None:
+                chunks.append(chunk)
+            chunk = util.Container()
+            chunk.diff = []
+            split_lines = line.split(' ')[1:3]
+            lines = [int(x.split(',')[0][1:]) for x in split_lines]
+            old_lineno = lines[0]
+            new_lineno = lines[1]
+        elif line.startswith(' '):
+            chunk.diff.append(util.Container(old_lineno=old_lineno,
+                                             new_lineno=new_lineno,
+                                             type='context',
+                                             line=line[1:]))
+            old_lineno += 1
+            new_lineno += 1
+        elif line.startswith('+'):
+            chunk.diff.append(util.Container(old_lineno=None,
+                                             new_lineno=new_lineno,
+                                             type='insert', line=line[1:]))
+            new_lineno += 1
+        elif line.startswith('-'):
+            chunk.diff.append(util.Container(old_lineno=old_lineno,
+                                             new_lineno=None,
+                                             type='delete', line=line[1:]))
+            old_lineno += 1
+        else:
+            chunk.diff.append(util.Container(old_lineno=None,
+                                             new_lineno=None,
+                                             type='unknown',
+                                             line=repr(line)))
+    if chunk is not None:
+        chunks.append(chunk)
+    return chunks
+
+
+def diff_chunks_for_file(file_id, old_tree, new_tree):
+    try:
+        old_lines = old_tree.get_file_lines(file_id)
+    except errors.NoSuchId:
+        old_lines = []
+    try:
+        new_lines = new_tree.get_file_lines(file_id)
+    except errors.NoSuchId:
+        new_lines = []
+    buffer = StringIO()
+    if old_lines != new_lines:
+        try:
+            diff.internal_diff('', old_lines, '', new_lines, buffer)
+        except errors.BinaryFile:
+            difftext = ''
+        else:
+            difftext = buffer.getvalue()
+    else:
+        difftext = ''
+
+    return _process_diff(difftext)
+
+
+class FileDiffUI(TemplatedBranchView):
+
+    template_path = 'loggerhead.templates.filediff'
+
+    def get_values(self, path, kwargs, headers):
+        history = self._history
+
+        revid = urllib.unquote(self.args[0])
+        compare_revid = urllib.unquote(self.args[1])
+        file_id = urllib.unquote(self.args[2])
+
+        repository = self._history._branch.repository
+        old_tree, new_tree = repository.revision_trees([compare_revid, revid])
+
+        chunks = diff_chunks_for_file(file_id, old_tree, new_tree)
+
+        return {
+            'util': util,
+            'chunks': chunks,
+        }
 
=== modified file 'loggerhead/controllers/revision_ui.py'
--- loggerhead/controllers/revision_ui.py	2009-03-17 02:19:20 +0000
+++ loggerhead/controllers/revision_ui.py	2009-03-17 06:43:34 +0000
@@ -17,70 +17,26 @@
 # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
 #
 
-from StringIO import StringIO
-
-import bzrlib.diff
-from bzrlib import errors
+import simplejson
+import urllib
 
 from paste.httpexceptions import HTTPServerError
 
 from loggerhead import util
 from loggerhead.controllers import TemplatedBranchView
+from loggerhead.controllers.filediff_ui import diff_chunks_for_file
 from loggerhead.history import rich_filename
 
 
 DEFAULT_LINE_COUNT_LIMIT = 3000
 
+def dq(p):
+    return urllib.quote(urllib.quote(p, safe=''))
 
 class RevisionUI(TemplatedBranchView):
 
     template_path = 'loggerhead.templates.revision'
 
-    def _process_diff(self, diff):
-        # doesn't really need to be a method; could be static.
-        chunks = []
-        chunk = None
-        for line in diff.splitlines():
-            if len(line) == 0:
-                continue
-            if line.startswith('+++ ') or line.startswith('--- '):
-                continue
-            if line.startswith('@@ '):
-                # new chunk
-                if chunk is not None:
-                    chunks.append(chunk)
-                chunk = util.Container()
-                chunk.diff = []
-                split_lines = line.split(' ')[1:3]
-                lines = [int(x.split(',')[0][1:]) for x in split_lines]
-                old_lineno = lines[0]
-                new_lineno = lines[1]
-            elif line.startswith(' '):
-                chunk.diff.append(util.Container(old_lineno=old_lineno,
-                                                 new_lineno=new_lineno,
-                                                 type='context',
-                                                 line=line[1:]))
-                old_lineno += 1
-                new_lineno += 1
-            elif line.startswith('+'):
-                chunk.diff.append(util.Container(old_lineno=None,
-                                                 new_lineno=new_lineno,
-                                                 type='insert', line=line[1:]))
-                new_lineno += 1
-            elif line.startswith('-'):
-                chunk.diff.append(util.Container(old_lineno=old_lineno,
-                                                 new_lineno=None,
-                                                 type='delete', line=line[1:]))
-                old_lineno += 1
-            else:
-                chunk.diff.append(util.Container(old_lineno=None,
-                                                 new_lineno=None,
-                                                 type='unknown',
-                                                 line=repr(line)))
-        if chunk is not None:
-            chunks.append(chunk)
-        return chunks
-
     def _parse_diffs(self, old_tree, new_tree, delta, specific_path):
         """
         Return a list of processed diffs, in the format::
@@ -98,57 +54,36 @@
                 ),
             )
         """
+        if specific_path:
+            fid = new_tree.path2id(specific_path)
+            kind = new_tree.kind(fid)
+            chunks=diff_chunks_for_file(fid, old_tree, new_tree)
+            return [util.Container(
+                filename=rich_filename(specific_path, kind), file_id=fid,
+                chunks=chunks)]
+
         process = []
         out = []
 
-        def include_specific_path(path):
-            return specific_path == path
-        def include_all_paths(path):
-            return True
-        if specific_path:
-            include_path = include_specific_path
-        else:
-            include_path = include_all_paths
-
         for old_path, new_path, fid, \
             kind, text_modified, meta_modified in delta.renamed:
-            if text_modified and include_path(new_path):
-                process.append((old_path, new_path, fid, kind))
+            if text_modified:
+                process.append((new_path, fid, kind))
         for path, fid, kind, text_modified, meta_modified in delta.modified:
-            if include_path(path):
-                process.append((path, path, fid, kind))
+            process.append((path, fid, kind))
         for path, fid, kind in delta.added:
-            if kind == 'file' and include_path(path):
-                process.append((path, path, fid, kind))
+            if kind == 'file':
+                process.append((path, fid, kind))
         for path, fid, kind in delta.removed:
-            if kind == 'file' and include_path(path):
-                process.append((path, path, fid, kind))
-
-        process.sort(key=lambda x:x[1])
-
-        for old_path, new_path, fid, kind in process:
-            try:
-                old_lines = old_tree.get_file_lines(fid)
-            except errors.NoSuchId:
-                old_lines = []
-            try:
-                new_lines = new_tree.get_file_lines(fid)
-            except errors.NoSuchId:
-                new_lines = []
-            buffer = StringIO()
-            if old_lines != new_lines:
-                try:
-                    bzrlib.diff.internal_diff(old_path, old_lines,
-                                              new_path, new_lines, buffer)
-                except bzrlib.errors.BinaryFile:
-                    diff = ''
-                else:
-                    diff = buffer.getvalue()
-            else:
-                diff = ''
+            if kind == 'file':
+                process.append((path, fid, kind))
+
+        process.sort()
+
+        for new_path, fid, kind in process:
             out.append(util.Container(
                 filename=rich_filename(new_path, kind), file_id=fid,
-                chunks=self._process_diff(diff)))
+                chunks=[]))
 
         return out
 
@@ -200,6 +135,20 @@
         if path in ('', '/'):
             path = None
         change.changes, diffs = self.get_changes_with_diff(change, compare_revid, path)
+        link_data = {}
+        path_to_id = {}
+        if compare_revid is None:
+            if change.parents:
+                cr = change.parents[0].revid
+            else:
+                cr = 'null:'
+        else:
+            cr = compare_revid
+        for i, item in enumerate(diffs):
+            item.index = i
+            link_data['diff-' + str(i)] = '%s/%s/%s' % (
+                dq(revid), dq(cr), dq(item.file_id))
+            path_to_id[item.filename] = 'diff-' + str(i)
         # add parent & merge-point branch-nick info, in case it's useful
         h.get_branch_nicks([change])
 
@@ -215,7 +164,10 @@
             'revid': revid,
             'change': change,
             'diffs': diffs,
+            'link_data': simplejson.dumps(link_data),
             'specific_path': path,
+            'json_specific_path': simplejson.dumps(path),
+            'path_to_id': simplejson.dumps(path_to_id),
             'start_revid': start_revid,
             'filter_file_id': filter_file_id,
             'util': util,
 
=== modified file 'loggerhead/static/javascript/custom.js'
--- loggerhead/static/javascript/custom.js	2009-03-16 03:22:02 +0000
+++ loggerhead/static/javascript/custom.js	2009-03-17 04:26:28 +0000
@@ -86,6 +86,7 @@
   this.expand_icon = config.expand_icon;
   this.source = config.source;
   this.loading = config.loading;
+  this.node_process = config.node_process;
 }
 
 function get_height(node) {
@@ -99,17 +100,19 @@
   return height;
 }
 
-Collapsable.prototype._load_finished = function(tid, res)
+Collapsable.prototype._load_finished = function(tid, res, args)
 {
   var newNode = Y.Node.create(res.responseText.split('\n').splice(1).join(''));
+  if (this.node_process)
+    this.node_process(newNode);
   this.source_target.ancestor().replaceChild(newNode, this.source_target);
   this.source_target = null;
   this.source = null;
   this.loading.setStyle('display', 'none');
-  this.open();
+  this.open(args[0]);
 };
 
-Collapsable.prototype.open = function()
+Collapsable.prototype.open = function(callback)
 {
   if (this.source) {
     this.loading.setStyle('display', 'block');
@@ -117,6 +120,7 @@
       this.source,
       {
         on: {complete: this._load_finished},
+        arguments: [callback],
         context: this
       });
     return;
@@ -146,7 +150,7 @@
       duration: 0.2
     });
 
-  anim.on('end', this.openComplete, this);
+  anim.on('end', this.openComplete, this, callback);
   container.setStyle('marginBottom', close_height - open_height);
   if (this.close_node) {
     this.close_node.setStyle('display', 'none');
@@ -156,8 +160,9 @@
   anim.run();
 };
 
-Collapsable.prototype.openComplete = function()
+Collapsable.prototype.openComplete = function(evt, callback)
 {
+  if (callback) callback();
   this.is_open = true;
 };
 
 
=== modified file 'loggerhead/static/javascript/diff.js'
--- loggerhead/static/javascript/diff.js	2009-03-16 21:53:23 +0000
+++ loggerhead/static/javascript/diff.js	2009-03-17 06:51:29 +0000
@@ -87,13 +87,14 @@
 
 function toggle_unified_sbs(event) {
   event.preventDefault();
+  var pts = Y.all(".pseudotable");
   if (unified) {
-    Y.all(".pseudotable").each(make_sbs);
+    pts && pts.each(make_sbs);
     unified = false;
     Y.get("#toggle_unified_sbs").set('innerHTML', "Show unified diffs");
   }
   else {
-    Y.all(".pseudotable").each(make_unified);
+    pts && pts.each(make_unified);
     unified = true;
     Y.get("#toggle_unified_sbs").set('innerHTML', "Show diffs side-by-side");
   }
@@ -103,7 +104,7 @@
 
 function toggle_expand_all_revisionview(action)
 {
-  var diffs = Y.all('.diffBox');
+  var diffs = Y.all('.diff');
   if (diffs == null) return;
   diffs.each(
     function(item, i)
@@ -142,24 +143,61 @@
   '#collapse_all a'
 );
 
+function node_process(node) {
+  if (!unified) {
+    node.get('children').filter('.pseudotable').each(make_sbs);
+  }
+}
+
+function zoom_to_diff (path) {
+  var collapsable = Y.get('#' + path_to_id[path]).collapsable;
+  if (!collapsable.is_open) {
+    collapsable.open(
+      function () {
+        window.location.hash = '#' + path;
+      });
+  }
+}
+
 Y.on(
   "domready", function () {
     Y.all(".show_if_js").removeClass("show_if_js");
-    var diffs = Y.all('.diffBox');
+    Y.all("#list-files a").on(
+      'click',
+      function (e) {
+        e.preventDefault();
+        var path = decodeURIComponent(e.target.get('href').split('#')[1]);
+        window.location.hash = '#' + path;
+        zoom_to_diff(path);
+      });
+    var diffs = Y.all('.diff');
     if (diffs == null) return;
     diffs.each(
       function(item, i)
       {
-        item.query('.expand_diff').on('click', function() { collapsable.toggle(); });
+        var source_url = null;
+        if (!specific_path)
+          source_url = global_path + '+filediff/' + link_data[item.get('id')];
+        item.query('.the-link').on(
+          'click',
+          function(e) {
+            e.preventDefault();
+            collapsable.toggle();
+          });
         var collapsable = new Collapsable(
           {
             expand_icon: item.query('.expand_diff'),
-            open_node: item.ancestor().query('.diffinfo'),
+            open_node: item.query('.diffinfo'),
             close_node: null,
-            source: null,
-            source_target: null,
-            is_open: true
+            source: source_url,
+            source_target: item.query('.source_target'),
+            is_open: specific_path != null,
+            loading: item.query('.loading'),
+            node_process: node_process
           });
        item.collapsable=collapsable;
        });
+    if (window.location.hash && !specific_path) {
+      zoom_to_diff(window.location.hash.substring(1));
+    }
   });
 
=== modified file 'loggerhead/templatefunctions.py'
--- loggerhead/templatefunctions.py	2009-03-07 07:29:26 +0000
+++ loggerhead/templatefunctions.py	2009-03-17 06:32:25 +0000
@@ -38,20 +38,20 @@
 
 
 @templatefunc
-def file_change_summary(url, entry, link_style='normal', currently_showing=None):
-    if link_style == 'fragment':
+def file_change_summary(url, entry, style='normal', currently_showing=None):
+    if style == 'fragment':
         def file_link(filename):
-            if currently_showing:
-                if filename == currently_showing:
-                    return '<b><a href="#%s">%s</a></b>' % (
-                        cgi.escape(filename), cgi.escape(filename))
-                else:
-                    return revision_link(url, entry.revno, filename)
+            if currently_showing and filename == currently_showing:
+                return '<b><a href="#%s">%s</a></b>' % (
+                    cgi.escape(filename), cgi.escape(filename))
             else:
-                return '<a href="#%s">%s</a>' % (
-                    cgi.escape(filename), cgi.escape(filename))
+                return revision_link(
+                    url, entry.revno, filename, '#' + filename)
     else:
-        file_link = lambda filename: revision_link(url, entry.revno, filename)
+        def file_link(filename):
+            return '<a href="%s%s" title="View changes to %s in revision %s">%s</a>'%(
+                url(['/revision', entry.revno]), '#' + filename, cgi.escape(filename),
+                cgi.escape(entry.revno), cgi.escape(filename))
     return _pt('revisionfilechanges').expand(
         url=url, entry=entry, file_link=file_link,
         currently_showing=currently_showing, **templatefunctions)
@@ -120,7 +120,7 @@
         url(['/annotate', revno, path]), cgi.escape(path), cgi.escape(path))
 
 @templatefunc
-def revision_link(url, revno, path):
-    return '<a href="%s" title="View changes to %s in revision %s">%s</a>'%(
-        url(['/revision', revno, path]), cgi.escape(path), cgi.escape(revno),
-        cgi.escape(path))
+def revision_link(url, revno, path, frag=''):
+    return '<a href="%s%s" title="View changes to %s in revision %s">%s</a>'%(
+        url(['/revision', revno, path]), frag, cgi.escape(path),
+        cgi.escape(revno), cgi.escape(path))
 
=== added file 'loggerhead/templates/filediff.pt'
--- loggerhead/templates/filediff.pt	1970-01-01 00:00:00 +0000
+++ loggerhead/templates/filediff.pt	2009-03-17 00:02:21 +0000
@@ -0,0 +1,25 @@
+<div class="diffinfo">
+  <div class="pseudotable unified"
+       tal:repeat="chunk chunks">
+
+    <tal:block condition="not:repeat/chunk/start">
+      <div class="pseudorow context-row">
+        <div class="lineNumber separate"></div>
+        <div class="lineNumber second separate"></div>
+        <div class="code separate"></div>
+        <div class="clear"><!-- --></div>
+      </div>
+    </tal:block>
+
+    <div tal:repeat="line chunk/diff"
+         tal:attributes="class string:pseudorow ${line/type}-row">
+      <div class="lineNumber first"
+           tal:content="structure python:util.fill_div(line.old_lineno)"></div>
+      <div class="lineNumber second"
+           tal:content="structure python:util.fill_div(line.new_lineno)"></div>
+      <div tal:attributes="class string:code ${line/type}"
+           tal:content="structure python:util.fill_div(util.html_clean(line.line))"></div>
+      <div class="clear"><!-- --></div>
+    </div>
+  </div>
+</div>
 
=== modified file 'loggerhead/templates/revision.pt'
--- loggerhead/templates/revision.pt	2009-03-16 06:44:50 +0000
+++ loggerhead/templates/revision.pt	2009-03-17 03:49:33 +0000
@@ -11,6 +11,11 @@
               tal:attributes="href python:branch.static_url('/static/css/diff.css')"/>
         <script type="text/javascript"
                 tal:attributes="src python:branch.static_url('/static/javascript/diff.js')"></script>
+        <script type="text/javascript">
+          var link_data = <tal:b content="link_data" />;
+          var specific_path = <tal:b content="json_specific_path" />;
+          var path_to_id = <tal:b content="path_to_id" />;
+        </script>
       </metal:block>
     </head>
 
@@ -92,14 +97,16 @@
         </tal:we-are-comparing>
 
         <tal:revision-info replace="structure python:revisioninfo(url, branch, change, 'fragment', specific_path)" />
-        <p class="expand" id="expand_all" style="display:none;"><a href="#">
-            <img tal:attributes="src python:branch.static_url('/static/images/treeCollapsed.png')"
-                 alt="expand all" /> expand all</a>
-        </p>
-        <p class="expand show_if_js" id="collapse_all"><a href="#">
-            <img tal:attributes="src python:branch.static_url('/static/images/treeExpanded.png')"
-                 alt="collapse all" /> collapse all</a>
-        </p>
+        <tal:specific-path condition="not:specific_path">
+          <p class="expand show_if_js" id="expand_all"><a href="#">
+              <img tal:attributes="src python:branch.static_url('/static/images/treeCollapsed.png')"
+                   alt="expand all" /> expand all</a>
+          </p>
+          <p class="expand" id="collapse_all" style="display:none;"><a href="#">
+              <img tal:attributes="src python:branch.static_url('/static/images/treeExpanded.png')"
+                   alt="collapse all" /> collapse all</a>
+          </p>
+        </tal:specific-path>
         <!-- Table -->
         <p class="expand show_if_js"><a id="toggle_unified_sbs" href="#">Show diffs side-by-side</a></p>
         <p class="codin"><img tal:attributes="src python:branch.static_url('/static/images/newCode.gif')" alt="added" /> added</p>
@@ -110,22 +117,34 @@
 
       <div metal:fill-slot="content">
         <div class="diff"
-             tal:repeat="item diffs">
+             tal:repeat="item diffs" tal:attributes="id string:diff-${item/index}">
 
-          <div class="diffBox">
-            <img tal:attributes="src python:branch.static_url('/static/images/treeExpanded.png');
-                                 title python:branch.static_url('/static/images/treeCollapsed.png');
-                                 alt python:branch.static_url('/static/images/treeExpanded.png')"
-                 class="expand_diff" />
+          <div  class="diffBox">
             <a tal:attributes="href python:url(['/revision', change.revno, item.filename], clear=1);
                                id string:${item/filename};
                                title string:View changes to ${item/filename} only"
-               tal:content="item/filename">
+               class="the-link">
+              <img tal:attributes="src python:branch.static_url('/static/images/treeCollapsed.png');
+                                   title python:branch.static_url('/static/images/treeCollapsed.png');
+                                   alt python:branch.static_url('/static/images/treeExpanded.png')"
+                   class="expand_diff" tal:condition="not:specific_path" />
+              <img tal:attributes="src python:branch.static_url('/static/images/treeExpanded.png');
+                                   title python:branch.static_url('/static/images/treeCollapsed.png');
+                                   alt python:branch.static_url('/static/images/treeExpanded.png')"
+                   class="expand_diff" tal:condition="specific_path" />
+              <tal:b content="item/filename" />
             </a>
           </div>
           <div style="overflow: hidden">
             <div class="container">
-              <div class="diffinfo">
+              <div class="loading" style="display:none">
+                <img tal:attributes="src python:branch.static_url('/static/images/spinner.gif')" />
+              </div>
+              <div class="diffinfo" tal:condition="not:specific_path">
+                <div class="source_target" >
+                </div>
+              </div>
+              <div class="diffinfo" tal:condition="specific_path">
                 <div class="pseudotable unified"
                      tal:repeat="chunk item/chunks">