Merge lp:~mkanat/loggerhead/view-default into lp:loggerhead

[Max Kanat-Alexander]

Create a new "view" controller, and have that be the default view of a file.

This adds a new controller, "view", and makes it the default for viewing a file, instead of "annotate". The "view" controller is just like "annotate", except that it doesn't show the revision for each line. This should significantly improve the performance of loggerhead, particularly on high-level installations (like launchpad) or on large branches (such as launchpad itself) because most of the time, people don't need the annotated information for a branch--they just want to see its content.

In actual tests, the "view" controller is between 3x and 10x faster than the "annotate" controller. (To see similar gains to these, try viewing a large file on the launchpad devel branch without any existing caches in loggerhead.)

The "view" and "annotate" controller both use the same templates, and I've renamed all of the templates and CSS to "view" instead of "annotate" (since we now think of "annotate" as a special case of "view"). I've moved annotate_ui.py to view_ui.py and created a brand-new annotate_ui.py over the old one.

[Michael Hudson-Doyle (mwhudson)]

Yay for progress on this!

I think it's mostly fine. The template is ugly though. Why do you need to put a None at the end of the annotation iterator? It would be better to pad out the results until you've reached the length of the text lines, or something.

If you're going to rename the annotation css file, you should probably rename the rules therein too.

Everything else looks ok. Loading the annotation information via ajax would be nice, but can wait :-)

Cheers,
mwh

review: Approve

[Max Kanat-Alexander (mkanat)]

> I think it's mostly fine. The template is ugly though. Why do you need to
> put a None at the end of the annotation iterator? It would be better to pad
> out the results until you've reached the length of the text lines, or
> something.

  That's true, I could do that, but that would also then involve code for keeping track of when there were extra lines in 'contents' that were beyond how far along we were with annotating. Would simplify the template, though, which I think is a valid goal. I think the problem is that 'contents' contains an extra final line when there's a "\n" at the end of the file, and annotate_iter doesn't consider that to be a line.

> If you're going to rename the annotation css file, you should probably rename
> the rules therein too.

  Yeah, I suppose you're right. I was trying to keep the changes at least *somewhat* focused though, so that they could be reviewed sensibly. But now that it's passed review I'll just do that change.

> Everything else looks ok. Loading the annotation information via ajax would
> be nice, but can wait :-)

  Yeah, I had the same thought.

[Martin Pool (mbp)]

That looks plausible to me, and reasonably clean.

As we discussed elsewhere, it would be good to get this onto the Launchpad deployment branch separately from changes in loggerhead trunk that are possibly more risky, such as historydb.

I wish there was an automatic test included here, but if there is no framework in place I won't ask you to write one as part of this proposal.

review: Approve

[lp:~mkanat/loggerhead/view-default]

433. By Max Kanat-Alexander on 2010-12-01

Instead of yielding None for when there are more lines than annotate_iter
has, just fix the off-by-one bug that made us have too many lines in
"contents" (caused by annotate_iter using split_lines and us using
split("\n") in highlight()).

434. By Max Kanat-Alexander on 2010-12-01

The colors of rows in the annotated view wasn't getting set properly. There
would be a revision number, and then the *next* line would change color.

435. By Max Kanat-Alexander on 2010-12-01

Rename the anno* CSS classes to view*.

=== modified file 'loggerhead/apps/branch.py'
--- loggerhead/apps/branch.py	2010-05-21 14:07:06 +0000
+++ loggerhead/apps/branch.py	2010-12-01 08:13:14 +0000
@@ -29,6 +29,7 @@
 
 from loggerhead.apps import static_app
 from loggerhead.controllers.annotate_ui import AnnotateUI
+from loggerhead.controllers.view_ui import ViewUI
 from loggerhead.controllers.atom_ui import AtomUI
 from loggerhead.controllers.changelog_ui import ChangeLogUI
 from loggerhead.controllers.diff_ui import DiffUI
@@ -114,6 +115,7 @@
         'files': InventoryUI,
         'revision': RevisionUI,
         'search': SearchUI,
+        'view': ViewUI,
         }
 
     def last_updated(self):
 
=== added file 'loggerhead/controllers/annotate_ui.py'
--- loggerhead/controllers/annotate_ui.py	1970-01-01 00:00:00 +0000
+++ loggerhead/controllers/annotate_ui.py	2010-12-01 08:13:14 +0000
@@ -0,0 +1,54 @@
+#
+# Copyright (C) 2010 Canonical Ltd.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+#
+
+from loggerhead.controllers.view_ui import ViewUI
+from loggerhead import util
+
+class AnnotateUI(ViewUI):
+
+    def annotate_file(self, info):
+        file_id = info['file_id']
+        revid = info['change'].revid
+        
+        tree = self.tree_for(file_id, revid)
+        
+        change_cache = {}
+        last_line_revid = None
+        parity = 1
+        for line_revid, text in tree.annotate_iter(file_id):
+            if line_revid == last_line_revid:
+                # remember which lines have a new revno and which don't
+                new_rev = False
+            else:
+                new_rev = True
+                parity ^= 1
+                last_line_revid = line_revid
+                if line_revid in change_cache:
+                    change = change_cache[line_revid]
+                else:
+                    change = self._history.get_changes([line_revid])[0]
+                    change_cache[line_revid] = change
+
+            yield util.Container(
+                parity=parity, new_rev=new_rev, change=change)
+            
+    def get_values(self, path, kwargs, headers):
+        values = super(AnnotateUI, self).get_values(path, kwargs, headers)
+        values['annotated'] = self.annotate_file(values)
+        
+        return values
 
=== renamed file 'loggerhead/controllers/annotate_ui.py' => 'loggerhead/controllers/view_ui.py'
--- loggerhead/controllers/annotate_ui.py	2009-10-17 06:35:33 +0000
+++ loggerhead/controllers/view_ui.py	2010-12-01 08:13:14 +0000
@@ -35,20 +35,18 @@
 from loggerhead import util
 
 
-class AnnotateUI(TemplatedBranchView):
-
-    template_path = 'loggerhead.templates.annotate'
-
-    def annotate_file(self, file_id, revid):
-        z = time.time()
-        lineno = 1
-        parity = 0
-
+class ViewUI(TemplatedBranchView):
+
+    template_path = 'loggerhead.templates.view'
+    
+    def tree_for(self, file_id, revid):
         file_revid = self._history.get_inventory(revid)[file_id].revision
-        tree = self._history._branch.repository.revision_tree(file_revid)
+        return self._history._branch.repository.revision_tree(file_revid)
 
+    def text_lines(self, file_id, revid):
         file_name = os.path.basename(self._history.get_path(revid, file_id))
-
+        
+        tree = self.tree_for(file_id, revid)
         file_text = tree.get_file_text(file_id)
         encoding = 'utf-8'
         try:
@@ -58,44 +56,27 @@
             file_text = file_text.decode(encoding)
 
         file_lines = bzrlib.osutils.split_lines(file_text)
+        # This can throw bzrlib.errors.BinaryFile (which our caller catches).
+        bzrlib.textfile.check_text_lines(file_lines)
+        
+        if highlight is not None:
+            hl_lines = highlight(file_name, file_text, encoding)
+            # highlight strips off extra newlines at the end of the file.
+            extra_lines = len(file_lines) - len(hl_lines)
+            hl_lines.extend([u''] * extra_lines)
+        else:
+            hl_lines = map(cgi.escape, file_lines)
+        
+        return hl_lines;
 
+    def file_contents(self, file_id, revid):
         try:
-            bzrlib.textfile.check_text_lines(file_lines)
+            file_lines = self.text_lines(file_id, revid)
         except bzrlib.errors.BinaryFile:
             # bail out; this isn't displayable text
-            yield util.Container(parity=0, lineno=1, status='same',
-                                 text='(This is a binary file.)',
-                                 change=util.Container())
-        else:
-            if highlight is not None:
-                hl_lines = highlight(file_name, file_text, encoding)
-                hl_lines.extend([u''] * (len(file_lines) - len(hl_lines)))
-            else:
-                hl_lines = map(cgi.escape, file_lines)
-
-            change_cache = {}
-
-            last_line_revid = None
-            for line_revid, text in tree.annotate_iter(file_id):
-                if line_revid == last_line_revid:
-                    # remember which lines have a new revno and which don't
-                    status = 'same'
-                else:
-                    status = 'changed'
-                    parity ^= 1
-                    last_line_revid = line_revid
-                    if line_revid in change_cache:
-                        change = change_cache[line_revid]
-                    else:
-                        change = self._history.get_changes([line_revid])[0]
-                        change_cache[line_revid] = change
-
-                yield util.Container(
-                    parity=parity, lineno=lineno, status=status,
-                    change=change, text=hl_lines[lineno - 1])
-                lineno += 1
-
-        self.log.debug('annotate: %r secs' % (time.time() - z,))
+            return ['(This is a binary file.)']
+
+        return file_lines
 
     def get_values(self, path, kwargs, headers):
         history = self._history
@@ -105,7 +86,7 @@
         file_id = kwargs.get('file_id', None)
         if (file_id is None) and (path is None):
             raise HTTPBadRequest('No file_id or filename '
-                                 'provided to annotate')
+                                 'provided to view')
 
         if file_id is None:
             file_id = history.get_file_id(revid, path)
@@ -140,13 +121,17 @@
         branch_breadcrumbs = util.branch_breadcrumbs(path, inv, 'files')
 
         return {
+            # In AnnotateUI, "annotated" is a generator giving revision
+            # numbers per lines, but the template checks if "annotated" is
+            # true or not before using it, so we have to define it here also.
+            'annotated': False,
             'revno_url': revno_url,
             'file_id': file_id,
-            'path': path,
+            'file_path': path,
             'filename': filename,
             'navigation': navigation,
             'change': change,
-            'contents': list(self.annotate_file(file_id, revid)),
+            'contents':  self.file_contents(file_id, revid),
             'fileview_active': True,
             'directory_breadcrumbs': directory_breadcrumbs,
             'branch_breadcrumbs': branch_breadcrumbs,
 
=== modified file 'loggerhead/highlight.py'
--- loggerhead/highlight.py	2010-04-22 03:23:55 +0000
+++ loggerhead/highlight.py	2010-12-01 08:13:14 +0000
@@ -16,6 +16,7 @@
 # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
 #
 
+import bzrlib.osutils
 import cgi
 
 from pygments import highlight as _highlight_func
@@ -36,7 +37,7 @@
     """
 
     if len(text) > MAX_HIGHLIGHT_SIZE:
-        return map(cgi.escape, text.split('\n'))
+        return map(cgi.escape,  bzrlib.osutils.split_lines(text))
 
     formatter = HtmlFormatter(style=style, nowrap=True, classprefix='pyg-')
 
@@ -48,6 +49,7 @@
         except (ClassNotFound, ValueError):
             lexer = TextLexer(encoding=encoding)
 
-    hl_lines = _highlight_func(text, lexer, formatter).split('\n')
+    hl_lines = _highlight_func(text, lexer, formatter)
+    hl_lines = bzrlib.osutils.split_lines(hl_lines)
 
     return hl_lines
 
=== renamed file 'loggerhead/static/css/annotate.css' => 'loggerhead/static/css/view.css'
--- loggerhead/static/css/annotate.css	2009-04-30 10:39:05 +0000
+++ loggerhead/static/css/view.css	2010-12-01 08:13:14 +0000
@@ -1,29 +1,31 @@
 /*table*/
-.annoLineTit, .annoLine, .annoRevTit, .annoRev, .annoComm, .annoCommTit, .annoContTit, .annoCont {
-	width:45px;
+.viewLineTit, .viewLine, .viewRevTit, .viewRev,
+.viewContTit, .viewCont
+{
 	border:1px solid #d2d2d2;
 }
-.annoLine, .annoRev, .annoComm, .annoCont {
+.viewLine, .viewRev, .viewComm, .viewCont {
 	border:none;
 }
-.annoLine {
+.viewLine, .viewLineTit {
+	padding-right: .2em;
+}
+.viewLine {
 	width:37px;
 }
-.annoContTit, .annoCont {
+.viewContTit, .viewCont {
 	width:auto;
+	padding-left: .3em;
 }
-.annoRevTit, .annoRev {
+.viewRevTit, .viewRev {
 	width:70px;
 	text-align:center;
 }
-.annoComm, .annoCommTit {
-	width:200px;
-}
-.annoLine, .annoCont {
+.viewLine, .viewCont {
 	font:normal 12px/normal monospace;
         whitespace: pre;
 }
-.annoCont pre { margin: 0; }
-.annoLine {
+.viewCont pre { margin: 0; }
+.viewLine {
 	text-align:right;
 }
 
=== modified file 'loggerhead/templatefunctions.py'
--- loggerhead/templatefunctions.py	2009-10-17 08:47:38 +0000
+++ loggerhead/templatefunctions.py	2010-12-01 08:13:14 +0000
@@ -121,9 +121,9 @@
 
 
 @templatefunc
-def annotate_link(url, revno, path):
+def view_link(url, revno, path):
     return '<a href="%s" title="Annotate %s">%s</a>' % (
-        url(['/annotate', revno, path]), cgi.escape(path), cgi.escape(path))
+        url(['/view', revno, path]), cgi.escape(path), cgi.escape(path))
 
 @templatefunc
 def revision_link(url, revno, path, frag=''):
 
=== modified file 'loggerhead/templates/inventory.pt'
--- loggerhead/templates/inventory.pt	2009-10-17 06:55:25 +0000
+++ loggerhead/templates/inventory.pt	2010-12-01 08:13:14 +0000
@@ -89,11 +89,11 @@
 
             <!-- Show this if it's a symlink -->
             <tr tal:attributes="class string:blueRow${repeat/file/even}" tal:condition="python:file.kind=='symlink'">
-                <td class="autcell"><a tal:attributes="href python:url(['/annotate', change.revno, file.absolutepath])">
+                <td class="autcell"><a tal:attributes="href python:url(['/view', change.revno, file.absolutepath])">
                         <img tal:attributes="src python:branch.static_url('/static/images/ico_file_flecha.gif')" alt="Symlink" />
                     </a>
 
-                 <a tal:attributes="href python:url(['/annotate', revno_url, file.absolutepath])"
+                 <a tal:attributes="href python:url(['/view', revno_url, file.absolutepath])"
                                         tal:content="file/filename" class="link"></a>
                  </td>
                  <td class="date"><a tal:attributes="href python:url(['/revision', file.change.revno]);
@@ -113,17 +113,17 @@
 
             <!-- Show this if it's a regular file -->
             <tr tal:attributes="class string:blueRow${repeat/file/even}" tal:condition="python:file.kind=='file'">
-                <td class="autcell"><a tal:attributes="href python:url(['/annotate', revno_url, file.absolutepath])">
+                <td class="autcell"><a tal:attributes="href python:url(['/view', revno_url, file.absolutepath])">
                         <img tal:attributes="src python:branch.static_url('/static/images/ico_file.gif');
-                                             title string:Annotate ${file/filename}"
+                                             title string:View ${file/filename}"
                              tal:condition="python:file.executable is False" />
                         <!-- Show a different icon id the file executable -->
                         <img tal:attributes="src python:branch.static_url('/static/images/ico_file_modify.gif');
-                                             title string:Annotate ${file/filename}"
+                                             title string:View ${file/filename}"
                              tal:condition="python:file.executable is True" alt="File" />
                     </a>
 
-              <a tal:attributes="href python:url(['/annotate', revno_url, file.absolutepath])"
+              <a tal:attributes="href python:url(['/view', revno_url, file.absolutepath])"
                                      tal:content="file/filename" class="link"></a></td>
               <td class="date"><a tal:attributes="href python:url(['/revision', file.change.revno]);
                                                   title string:Show revision ${file/change/revno}"
@@ -131,8 +131,8 @@
               </td>
               <td class="date" tal:content="python:util.date_time(file.change.date)"></td>
               <td class="timedate2" tal:content="python:util.human_size(file.size)"></td>
-              <td class="expcell"><a tal:attributes="href python:url(['/annotate', revno_url, file.absolutepath]);
-                                                     title string:Annotate ${file/filename}">
+              <td class="expcell"><a tal:attributes="href python:url(['/view', revno_url, file.absolutepath]);
+                                                     title string:View ${file/filename}">
                                       <img tal:attributes="src python:branch.static_url('/static/images/ico_planilla.gif')" alt="Diff" />
                                      </a>
               </td>
 
=== modified file 'loggerhead/templates/revision.pt'
--- loggerhead/templates/revision.pt	2009-04-28 03:01:54 +0000
+++ loggerhead/templates/revision.pt	2010-12-01 08:13:14 +0000
@@ -42,7 +42,7 @@
             </tal:compare-to>
           </span>
           <span class="breadcrumb" tal:condition="specific_path">
-            : <tal:annotate content="structure python:annotate_link(url, change.revno, specific_path)" />
+            : <tal:annotate content="structure python:view_link(url, change.revno, specific_path)" />
           </span>
         </h1>
         <tal:branch-info replace="structure python:branchinfo(branch)" />
@@ -55,7 +55,7 @@
           </a>
         </p>
         <p tal:condition="specific_path">
-          Viewing changes to <tal:annotate content="structure python:annotate_link(url, change.revno, specific_path)" />
+          Viewing changes to <tal:annotate content="structure python:view_link(url, change.revno, specific_path)" />
         </p>
         <ul id="submenuTabs">
           <li id="first"><a tal:attributes="href python:url(['/files', change.revno]);
 
=== renamed file 'loggerhead/templates/annotate.pt' => 'loggerhead/templates/view.pt'
--- loggerhead/templates/annotate.pt	2009-04-30 10:39:05 +0000
+++ loggerhead/templates/view.pt	2010-12-01 08:13:14 +0000
@@ -3,12 +3,12 @@
 <html xmlns="http://www.w3.org/1999/xhtml" metal:use-macro="macros/main">
   <head>
     <title metal:fill-slot="title"
-           tal:content="string:${branch/friendly_name} : contents of ${path}
+           tal:content="string:${branch/friendly_name} : contents of ${file_path}
                         at revision ${change/revno}">
     </title>
     <metal:block fill-slot="header_extras">
         <link rel="stylesheet" type="text/css" media="all"
-              tal:attributes="href python:branch.static_url('/static/css/annotate.css')"/>
+              tal:attributes="href python:branch.static_url('/static/css/view.css')"/>
         <link rel="stylesheet" type="text/css" media="all"
               tal:attributes="href python:branch.static_url('/static/css/highlight.css')"/>
     </metal:block>
@@ -36,6 +36,14 @@
             <li id="first">
                 <a tal:attributes="href python:url(['/files', change.revno], clear=1)">browse files</a>
             </li>
+            <li tal:condition="not:annotated">
+              <a tal:attributes="href python:url(['/annotate', revno_url, file_path], clear=1)"
+              >view revision numbers per line</a>
+            </li>
+            <li tal:condition="annotated">
+              <a tal:attributes="href python:url(['/view', revno_url, file_path], clear=1)"
+              >view without revision numbers</a>
+            </li>
             <li>
                 <a tal:attributes="href python:url(['/revision', change.revno], clear=1)">view revision</a>
             </li>
@@ -50,25 +58,30 @@
             </li>
         </ul>
 
-      <div class="annotate">
+      <div class="view">
         <table id="logentries">
           <tr class="logheader">
-            <td class="annoLineTit">Line</td>
-            <td class="annoRevTit">Revision</td>
-            <td class="annoContTit">Contents</td>
+            <td class="viewLineTit">Line</td>
+            <td class="viewRevTit" tal:condition="annotated">Revision</td>
+            <td class="viewContTit">Contents</td>
           </tr>
 
-          <tr tal:repeat="line contents"
-              tal:attributes="class string:blueRow${line/parity}">
-            <td class="annoLine"><a tal:attributes="id string:L${line/lineno}; href string:#L${line/lineno}" tal:content="line/lineno">1</a></td>
-            <td class="annoRev">
-              <a tal:condition="python:line.status=='changed'"
-                 tal:content="python:util.trunc(line.change.revno)"
-                 tal:attributes="href python:url(['/revision', line.change.revno], clear=1);
-                                 title python:'%s by %s, on %s (%s)'%(line.change.revno, ', '.join(util.hide_emails(line.change.authors)), line.change.date.strftime('%d %b %Y %H:%M'), util.date_time(line.change.date))"></a>
-            </td>
-            <td class="annoCont"><pre tal:content="structure line/text"></pre></td>
-          </tr>
+          <tal:rep tal:repeat="line contents">
+            <tr tal:define="anno python:annotated and annotated.next()"
+                tal:attributes="class python:anno and 'blueRow' + str(anno.parity) or None">
+              <td class="viewLine">
+                <a tal:attributes="id string:L${repeat/line/number}; href string:#L${repeat/line/number}"
+                   tal:content="repeat/line/number">1</a>
+              </td>
+              <td class="viewRev" tal:condition="annotated">
+                <a tal:condition="python:anno.new_rev"
+                   tal:content="python:util.trunc(anno.change.revno)"
+                   tal:attributes="href python:url(['/revision', anno.change.revno], clear=1);
+                                   title python:'%s by %s, on %s (%s)' % (anno.change.revno, ', '.join(util.hide_emails(anno.change.authors)), anno.change.date.strftime('%d %b %Y %H:%M'), util.date_time(anno.change.date))"></a>
+              </td>
+              <td class="viewCont"><pre tal:content="structure line"></pre></td>
+            </tr>
+          </tal:rep>
         </table>
       </div>