Merge lp:~cjwatson/launchpad/side-by-side-diff into lp:launchpad

[Colin Watson]

Add an alternative side-by-side preview diff view to merge proposals.

Add an alternative side-by-side preview diff view to merge proposals.

A first pass at this, at least, turned out to be much easier than I expected: it's mostly a matter of adding fmt:ssdiff which mangles a unified diff into side-by-side form, and keeping careful track of line numbers. The line-no cells are retained for the sake of inline comment tracking, but hidden from display; I added new ss-line-no cells which show the line numbers in the underlying files, which feels more comprehensible in this view and is more in line with how side-by-side diff views in other web applications tend to work.

I added similar support to other places where fmt:diff is used since it was quick to do so, but for now those other views are unlinked and only accessible via URL-hacking to add ?ss=1. We can add links later if they're needed.

[William Grant (wgrant)]

review: Approve (code)

=== modified file 'lib/canonical/launchpad/icing/css/modifiers.css'
--- lib/canonical/launchpad/icing/css/modifiers.css	2014-05-19 08:03:55 +0000
+++ lib/canonical/launchpad/icing/css/modifiers.css	2015-06-29 23:03:03 +0000
@@ -166,7 +166,7 @@
     }
 
 pre.changelog,
-table.diff td.line-no,
+table.diff td.line-no, table.diff td.ss-line-no,
 table.diff td.text,
 .comment-text,
 .bug-activity {

=== modified file 'lib/canonical/launchpad/icing/style.css'
--- lib/canonical/launchpad/icing/style.css	2015-06-24 21:22:17 +0000
+++ lib/canonical/launchpad/icing/style.css	2015-06-29 23:03:03 +0000
@@ -626,10 +626,12 @@
 table.diff {
    white-space: pre-wrap;
    width: 100%;
-   table-layout: fixed;
    background-color: white;
    margin-bottom: 0.5em;
 }
+table.unidiff {
+   table-layout: fixed;
+}
 
 table.diff .nav-cursor:before {
    content: "\0025B6";
@@ -639,7 +641,7 @@
    margin-left: 1.25em;
 }
 table.diff .text { padding-left: 0.5em; }
-table.diff .line-no {
+table.diff .line-no, table.diff .ss-line-no {
   text-align: right;
   background-color: #f6f6f6;
   color: #999;

=== modified file 'lib/lp/app/browser/stringformatter.py'
--- lib/lp/app/browser/stringformatter.py	2015-04-29 14:30:26 +0000
+++ lib/lp/app/browser/stringformatter.py	2015-06-29 23:03:03 +0000
@@ -11,16 +11,21 @@
     'extract_email_addresses',
     'FormattersAPI',
     'linkify_bug_numbers',
+    'parse_diff',
     're_substitute',
     'split_paragraphs',
     ]
 
 from base64 import urlsafe_b64encode
+from itertools import izip_longest
 import re
+import sys
 
+from bzrlib.patches import hunk_from_header
 from lxml import html
 import markdown
 from zope.component import getUtility
+from zope.error.interfaces import IErrorReportingUtility
 from zope.interface import implements
 from zope.traversing.interfaces import (
     ITraversable,
@@ -241,6 +246,59 @@
     return list(set([match.group() for match in matches]))
 
 
+def parse_diff(text):
+    """Parse a string into categorised diff lines.
+
+    Yields a sequence of (CSS class, line number in diff, line number in
+    original file, line number in modified file, line).
+    """
+    max_format_lines = config.diff.max_format_lines
+    header_next = False
+    orig_row = 0
+    mod_row = 0
+    for row, line in enumerate(text.splitlines()[:max_format_lines]):
+        row += 1
+        if (line.startswith('===') or
+                line.startswith('diff') or
+                line.startswith('index')):
+            yield 'diff-file text', row, None, None, line
+            header_next = True
+        elif (header_next and
+              (line.startswith('+++') or
+              line.startswith('---'))):
+            yield 'diff-header text', row, None, None, line
+        elif line.startswith('@@'):
+            try:
+                hunk = hunk_from_header(line + '\n')
+                # The positions indicate the per-file line numbers of
+                # the next row.
+                orig_row = hunk.orig_pos
+                mod_row = hunk.mod_pos
+            except Exception:
+                getUtility(IErrorReportingUtility).raising(sys.exc_info())
+                orig_row = 1
+                mod_row = 1
+            yield 'diff-chunk text', row, None, None, line
+            header_next = False
+        elif line.startswith('+'):
+            yield 'diff-added text', row, None, mod_row, line
+            mod_row += 1
+        elif line.startswith('-'):
+            yield 'diff-removed text', row, orig_row, None, line
+            orig_row += 1
+        elif line.startswith('#'):
+            # This doesn't occur in normal unified diffs, but does
+            # appear in merge directives, which use text/x-diff or
+            # text/x-patch.
+            yield 'diff-comment text', row, None, None, line
+            header_next = False
+        else:
+            yield 'text', row, orig_row, mod_row, line
+            orig_row += 1
+            mod_row += 1
+            header_next = False
+
+
 class FormattersAPI:
     """Adapter from strings to HTML formatted text."""
 
@@ -877,40 +935,11 @@
         text = self._stringtoformat.rstrip('\n')
         if len(text) == 0:
             return text
-        result = ['<table class="diff">']
+        result = ['<table class="diff unidiff">']
 
-        max_format_lines = config.diff.max_format_lines
-        header_next = False
-        for row, line in enumerate(text.splitlines()[:max_format_lines]):
-            result.append('<tr id="diff-line-%s">' % (row + 1))
-            result.append('<td class="line-no">%s</td>' % (row + 1))
-            if (line.startswith('===') or
-                    line.startswith('diff') or
-                    line.startswith('index')):
-                css_class = 'diff-file text'
-                header_next = True
-            elif (header_next and
-                  (line.startswith('+++') or
-                  line.startswith('---'))):
-                css_class = 'diff-header text'
-            elif line.startswith('@@'):
-                css_class = 'diff-chunk text'
-                header_next = False
-            elif line.startswith('+'):
-                css_class = 'diff-added text'
-                header_next = False
-            elif line.startswith('-'):
-                css_class = 'diff-removed text'
-                header_next = False
-            elif line.startswith('#'):
-                # This doesn't occur in normal unified diffs, but does
-                # appear in merge directives, which use text/x-diff or
-                # text/x-patch.
-                css_class = 'diff-comment text'
-                header_next = False
-            else:
-                css_class = 'text'
-                header_next = False
+        for css_class, row, _, _, line in parse_diff(text):
+            result.append('<tr id="diff-line-%s">' % row)
+            result.append('<td class="line-no">%s</td>' % row)
             result.append(
                 structured(
                     '<td class="%s">%s</td>', css_class, line).escapedtext)
@@ -919,6 +948,95 @@
         result.append('</table>')
         return ''.join(result)
 
+    def _ssdiff_emit_line(self, result, row, cells):
+        result.append('<tr id="diff-line-%s">' % row)
+        # A line-no cell has to be present for the inline comments code to
+        # work, but displaying it would be confusing since there are also
+        # per-file line numbers.
+        result.append(
+            '<td class="line-no" style="display: none">%s</td>' % row)
+        result.extend(cells)
+        result.append('</tr>')
+
+    def _ssdiff_emit_queued_lines(self, result, queue_removed, queue_added):
+        for removed, added in izip_longest(queue_removed, queue_added):
+            if removed:
+                removed_diff_row, removed_row, removed_line = removed
+            else:
+                removed_diff_row, removed_row, removed_line = 0, '', ''
+            if added:
+                added_diff_row, added_row, added_line = added
+            else:
+                added_diff_row, added_row, added_line = 0, '', ''
+            cells = (
+                '<td class="ss-line-no">%s</td>' % removed_row,
+                structured(
+                    '<td class="diff-removed text">%s</td>',
+                    removed_line).escapedtext,
+                '<td class="ss-line-no">%s</td>' % added_row,
+                structured(
+                    '<td class="diff-added text">%s</td>',
+                    added_line).escapedtext,
+                )
+            # Pick a reasonable row of the unified diff to attribute inline
+            # comments to.  Whichever of the removed and added rows is later
+            # will do.
+            self._ssdiff_emit_line(
+                result, max(removed_diff_row, added_diff_row), cells)
+
+    def format_ssdiff(self):
+        """Format the string as a side-by-side diff."""
+        # Trim off trailing carriage returns.
+        text = self._stringtoformat.rstrip('\n')
+        if not text:
+            return text
+        result = ['<table class="diff ssdiff">']
+
+        queue_orig = []
+        queue_mod = []
+        for css_class, row, orig_row, mod_row, line in parse_diff(text):
+            if orig_row is not None and mod_row is None:
+                # Text line only in original version.  Queue until we find a
+                # common or non-text line.
+                queue_orig.append((row, orig_row, line[1:]))
+            elif mod_row is not None and orig_row is None:
+                # Text line only in modified version.  Queue until we find a
+                # common or non-text line.
+                queue_mod.append((row, mod_row, line[1:]))
+            else:
+                # Common or non-text line; emit any queued differences, then
+                # this line.
+                if queue_orig or queue_mod:
+                    self._ssdiff_emit_queued_lines(
+                        result, queue_orig, queue_mod)
+                    queue_orig = []
+                    queue_mod = []
+                if orig_row is None and mod_row is None:
+                    # Non-text line.
+                    cells = [
+                        structured(
+                            '<td class="%s" colspan="4">%s</td>',
+                            css_class, line).escapedtext,
+                        ]
+                else:
+                    # Line common to both versions.
+                    if line.startswith(' '):
+                        line = line[1:]
+                    cells = [
+                        '<td class="ss-line-no">%s</td>' % orig_row,
+                        structured(
+                            '<td class="text">%s</td>', line).escapedtext,
+                        '<td class="ss-line-no">%s</td>' % mod_row,
+                        structured(
+                            '<td class="text">%s</td>', line).escapedtext,
+                        ]
+                self._ssdiff_emit_line(result, row, cells)
+        if queue_orig or queue_mod:
+            self._ssdiff_emit_queued_lines(result, queue_orig, queue_mod)
+
+        result.append('</table>')
+        return ''.join(result)
+
     _css_id_strip_pattern = re.compile(r'[^a-zA-Z0-9-_]+')
     _zope_css_id_strip_pattern = re.compile(r'[^a-zA-Z0-9-_\.]+')
 
@@ -1030,6 +1148,8 @@
             return self.ellipsize(maxlength)
         elif name == 'diff':
             return self.format_diff()
+        elif name == 'ssdiff':
+            return self.format_ssdiff()
         elif name == 'css-id':
             if len(furtherPath) > 0:
                 return self.css_id(furtherPath.pop())

=== modified file 'lib/lp/app/browser/tests/test_stringformatter.py'
--- lib/lp/app/browser/tests/test_stringformatter.py	2015-04-29 14:30:26 +0000
+++ lib/lp/app/browser/tests/test_stringformatter.py	2015-06-29 23:03:03 +0000
@@ -18,6 +18,7 @@
 from lp.app.browser.stringformatter import (
     FormattersAPI,
     linkify_bug_numbers,
+    parse_diff,
     )
 from lp.services.config import config
 from lp.services.features.testing import FeatureFixture
@@ -27,7 +28,10 @@
     TestCase,
     TestCaseWithFactory,
     )
-from lp.testing.layers import DatabaseFunctionalLayer
+from lp.testing.layers import (
+    DatabaseFunctionalLayer,
+    ZopelessLayer,
+    )
 from lp.testing.pages import find_tags_by_class
 
 
@@ -299,6 +303,174 @@
                 last_paragraph_class="last"))
 
 
+class TestParseDiff(TestCase):
+    """Test the parser for fmt:diff and fmt:ssdiff."""
+
+    def test_emptyString(self):
+        # An empty string yields no lines.
+        self.assertEqual([], list(parse_diff('')))
+
+    def test_almostEmptyString(self):
+        # White space yields a single line of text.
+        self.assertEqual([('text', 1, 0, 0, ' ')], list(parse_diff(' ')))
+
+    def test_unicode(self):
+        # Diffs containing Unicode work too.
+        self.assertEqual(
+            [('text', 1, 0, 0, u'Unicode \u1010')],
+            list(parse_diff(u'Unicode \u1010')))
+
+    def assertParses(self, expected, diff):
+        diff_lines = diff.splitlines()
+        self.assertEqual(
+            [(css_class, row + 1, orig_row, mod_row, diff_lines[row])
+             for row, (css_class, orig_row, mod_row) in enumerate(expected)],
+            list(parse_diff(diff)))
+
+    def test_basic_bzr(self):
+        # A basic Bazaar diff with a few different classes is parsed correctly.
+        diff = dedent('''\
+            === modified file 'tales.py'
+            --- tales.py
+            +++ tales.py
+            @@ -2435,7 +2439,8 @@
+                 def method(self):
+            -        removed this line
+            +        added this line
+            +        added another line
+                     something in between
+            -------- a sql style comment
+            ++++++++ a line of pluses
+            ########
+            # A merge directive comment.
+            ''')
+        expected = [
+            ('diff-file text', None, None),
+            ('diff-header text', None, None),
+            ('diff-header text', None, None),
+            ('diff-chunk text', None, None),
+            ('text', 2435, 2439),
+            ('diff-removed text', 2436, None),
+            ('diff-added text', None, 2440),
+            ('diff-added text', None, 2441),
+            ('text', 2437, 2442),
+            ('diff-removed text', 2438, None),
+            ('diff-added text', None, 2443),
+            ('diff-comment text', None, None),
+            ('diff-comment text', None, None),
+            ]
+        self.assertParses(expected, diff)
+
+    def test_basic_git(self):
+        # A basic Git diff with a few different classes is parsed correctly.
+        diff = dedent('''\
+            diff --git a/tales.py b/tales.py
+            index aaaaaaa..bbbbbbb 100644
+            --- a/tales.py
+            +++ b/tales.py
+            @@ -2435,7 +2439,8 @@
+                 def method(self):
+            -        removed this line
+            +        added this line
+            +        added another line
+                     something in between
+            -------- a sql style comment
+            ++++++++ a line of pluses
+            ''')
+        expected = [
+            ('diff-file text', None, None),
+            ('diff-file text', None, None),
+            ('diff-header text', None, None),
+            ('diff-header text', None, None),
+            ('diff-chunk text', None, None),
+            ('text', 2435, 2439),
+            ('diff-removed text', 2436, None),
+            ('diff-added text', None, 2440),
+            ('diff-added text', None, 2441),
+            ('text', 2437, 2442),
+            ('diff-removed text', 2438, None),
+            ('diff-added text', None, 2443),
+            ]
+        self.assertParses(expected, diff)
+
+    def test_config_value_limits_line_count(self):
+        # The config.diff.max_line_format contains the maximum number of
+        # lines to parse.
+        diff = dedent('''\
+            === modified file 'tales.py'
+            --- tales.py
+            +++ tales.py
+            @@ -2435,6 +2435,8 @@
+                 def method(self):
+            -        removed this line
+            +        added this line
+            ########
+            # A merge directive comment.
+            ''')
+        expected = [
+            ('diff-file text', None, None),
+            ('diff-header text', None, None),
+            ('diff-header text', None, None),
+            ]
+        self.pushConfig("diff", max_format_lines=3)
+        self.assertParses(expected, diff)
+
+    def test_multiple_hunks(self):
+        # Diffs containing multiple hunks are parsed reasonably, and the
+        # original and modified row numbers are adjusted for each hunk.
+        diff = dedent('''\
+            @@ -2,2 +2,3 @@
+             a
+            -b
+            +c
+            +d
+            @@ -10,3 +11,2 @@
+            -e
+             f
+            -g
+            +h
+            ''')
+        expected = [
+            ('diff-chunk text', None, None),
+            ('text', 2, 2),
+            ('diff-removed text', 3, None),
+            ('diff-added text', None, 3),
+            ('diff-added text', None, 4),
+            ('diff-chunk text', None, None),
+            ('diff-removed text', 10, None),
+            ('text', 11, 11),
+            ('diff-removed text', 12, None),
+            ('diff-added text', None, 12),
+            ]
+        self.assertParses(expected, diff)
+
+
+class TestParseDiffErrors(TestCaseWithFactory):
+
+    layer = ZopelessLayer
+
+    def assertParses(self, expected, diff):
+        diff_lines = diff.splitlines()
+        self.assertEqual(
+            [(css_class, row + 1, orig_row, mod_row, diff_lines[row])
+             for row, (css_class, orig_row, mod_row) in enumerate(expected)],
+            list(parse_diff(diff)))
+
+    def test_bad_hunk_header(self):
+        # A bad hunk header causes the parser to record an OOPS but continue
+        # anyway.
+        diff = dedent('''\
+            @@ some nonsense @@
+                 def method(self):
+            ''')
+        expected = [
+            ('diff-chunk text', None, None),
+            ('text', 1, 1),
+            ]
+        self.assertParses(expected, diff)
+        self.assertEqual('MalformedHunkHeader', self.oopses[0]['type'])
+
+
 class TestDiffFormatter(TestCase):
     """Test the string formatter fmt:diff."""
 
@@ -308,16 +480,16 @@
             '', FormattersAPI('').format_diff())
 
     def test_almostEmptyString(self):
-        # White space doesn't count as empty, and is formtted.
+        # White space doesn't count as empty, and is formatted.
         self.assertEqual(
-            '<table class="diff"><tr id="diff-line-1">'
+            '<table class="diff unidiff"><tr id="diff-line-1">'
             '<td class="line-no">1</td><td class="text"> </td></tr></table>',
             FormattersAPI(' ').format_diff())
 
     def test_format_unicode(self):
         # Sometimes the strings contain unicode, those should work too.
         self.assertEqual(
-            u'<table class="diff"><tr id="diff-line-1">'
+            u'<table class="diff unidiff"><tr id="diff-line-1">'
             u'<td class="line-no">1</td><td class="text">'
             u'Unicode \u1010</td></tr></table>',
             FormattersAPI(u'Unicode \u1010').format_diff())
@@ -391,24 +563,131 @@
              'diff-added text'],
             [str(tag['class']) for tag in text])
 
-    def test_config_value_limits_line_count(self):
-        # The config.diff.max_line_format contains the maximum number of lines
-        # to format.
+
+class TestSideBySideDiffFormatter(TestCase):
+    """Test the string formatter fmt:ssdiff."""
+
+    def test_emptyString(self):
+        # An empty string gives an empty string.
+        self.assertEqual(
+            '', FormattersAPI('').format_ssdiff())
+
+    def test_almostEmptyString(self):
+        # White space doesn't count as empty, and is formatted.
+        self.assertEqual(
+            '<table class="diff ssdiff"><tr id="diff-line-1">'
+            '<td class="line-no" style="display: none">1</td>'
+            '<td class="ss-line-no">0</td>'
+            '<td class="text"></td>'
+            '<td class="ss-line-no">0</td>'
+            '<td class="text"></td>'
+            '</tr></table>',
+            FormattersAPI(' ').format_ssdiff())
+
+    def test_format_unicode(self):
+        # Sometimes the strings contain unicode, those should work too.
+        self.assertEqual(
+            u'<table class="diff ssdiff"><tr id="diff-line-1">'
+            u'<td class="line-no" style="display: none">1</td>'
+            u'<td class="ss-line-no">0</td>'
+            u'<td class="text">Unicode \u1010</td>'
+            u'<td class="ss-line-no">0</td>'
+            u'<td class="text">Unicode \u1010</td>'
+            u'</tr></table>',
+            FormattersAPI(u'Unicode \u1010').format_ssdiff())
+
+    def test_cssClasses(self):
+        # Different parts of the diff have different css classes.
         diff = dedent('''\
             === modified file 'tales.py'
             --- tales.py
             +++ tales.py
-            @@ -2435,6 +2435,8 @@
-                 def format_diff(self):
+            @@ -2435,7 +2439,8 @@
+                 def format_ssdiff(self):
             -        removed this line
             +        added this line
+            +        added another line
+                     something in between
+            -------- a sql style comment
+            ++++++++ a line of pluses
             ########
             # A merge directive comment.
             ''')
-        self.pushConfig("diff", max_format_lines=3)
-        html = FormattersAPI(diff).format_diff()
-        line_count = html.count('<td class="line-no">')
-        self.assertEqual(3, line_count)
+        html = FormattersAPI(diff).format_ssdiff()
+        line_numbers = find_tags_by_class(html, 'line-no')
+        self.assertEqual(
+            ['1', '2', '3', '4', '5', '7', '8', '9', '11', '12', '13'],
+            [tag.renderContents() for tag in line_numbers])
+        ss_line_numbers = find_tags_by_class(html, 'ss-line-no')
+        self.assertEqual(
+            ['2435', '2439', '2436', '2440', '', '2441', '2437', '2442',
+             '2438', '2443'],
+            [tag.renderContents() for tag in ss_line_numbers])
+        text = find_tags_by_class(html, 'text')
+        self.assertEqual(
+            ['diff-file text',
+             'diff-header text',
+             'diff-header text',
+             'diff-chunk text',
+             'text',
+             'text',
+             'diff-removed text',
+             'diff-added text',
+             'diff-removed text',
+             'diff-added text',
+             'text',
+             'text',
+             'diff-removed text',
+             'diff-added text',
+             'diff-comment text',
+             'diff-comment text'],
+            [str(tag['class']) for tag in text])
+
+    def test_cssClasses_git(self):
+        # Git diffs look slightly different, so check that they also end up
+        # with the correct CSS classes.
+        diff = dedent('''\
+            diff --git a/tales.py b/tales.py
+            index aaaaaaa..bbbbbbb 100644
+            --- a/tales.py
+            +++ b/tales.py
+            @@ -2435,7 +2439,8 @@
+                 def format_ssdiff(self):
+            -        removed this line
+            +        added this line
+            +        added another line
+                     something in between
+            -------- a sql style comment
+            ++++++++ a line of pluses
+            ''')
+        html = FormattersAPI(diff).format_ssdiff()
+        line_numbers = find_tags_by_class(html, 'line-no')
+        self.assertEqual(
+            ['1', '2', '3', '4', '5', '6', '8', '9', '10', '12'],
+            [tag.renderContents() for tag in line_numbers])
+        ss_line_numbers = find_tags_by_class(html, 'ss-line-no')
+        self.assertEqual(
+            ['2435', '2439', '2436', '2440', '', '2441', '2437', '2442',
+             '2438', '2443'],
+            [tag.renderContents() for tag in ss_line_numbers])
+        text = find_tags_by_class(html, 'text')
+        self.assertEqual(
+            ['diff-file text',
+             'diff-file text',
+             'diff-header text',
+             'diff-header text',
+             'diff-chunk text',
+             'text',
+             'text',
+             'diff-removed text',
+             'diff-added text',
+             'diff-removed text',
+             'diff-added text',
+             'text',
+             'text',
+             'diff-removed text',
+             'diff-added text'],
+            [str(tag['class']) for tag in text])
 
 
 class TestOOPSFormatter(TestCase):

=== modified file 'lib/lp/code/javascript/branchmergeproposal.inlinecomments.js'
--- lib/lp/code/javascript/branchmergeproposal.inlinecomments.js	2015-06-02 01:39:48 +0000
+++ lib/lp/code/javascript/branchmergeproposal.inlinecomments.js	2015-06-29 23:03:03 +0000
@@ -1,4 +1,4 @@
-/* Copyright 2014 Canonical Ltd.  This software is licensed under the
+/* Copyright 2014-2015 Canonical Ltd.  This software is licensed under the
  * GNU Affero General Public License version 3 (see the file LICENSE).
  *
  * Code for handling inline comments in diffs.
@@ -161,9 +161,15 @@
         comment_date;
 
     if (comments_tr === null) {
+        if (Y.all('table.ssdiff').size() > 0) {
+            colspan = 4;
+        } else {
+            colspan = 2;
+        }
         comments_tr = Y.Node.create(
             '<tr class="inline-comments">' +
-            '<td class="inline-comment" colspan="2"><div></div></td></tr>')
+            '<td class="inline-comment" colspan="' + colspan + '">' +
+            '<div></div></td></tr>')
             .set('id', 'comments-diff-line-' + comment.line_number);
         Y.one('#diff-line-' + comment.line_number)
             .insert(comments_tr, 'after');
@@ -649,6 +655,11 @@
         var preview_diff_uri = (
             LP.cache.context.web_link +
             '/+preview-diff/' + previewdiff_id + '/+diff');
+        var qs = window.location.search;
+        var query = Y.QueryString.parse(qs.replace(/^[?]/, ''));
+        if (query.ss) {
+            preview_diff_uri += '?ss=1';
+        }
         this._updateDiff(preview_diff_uri);
     },
 
@@ -872,5 +883,6 @@
 
 namespace.DiffNav = DiffNav;
 
-}, '0.1', {requires: ['datatype-date', 'event', 'io', 'node', 'widget',
+}, '0.1', {requires: ['datatype-date', 'event', 'io', 'node',
+                      'querystring-parse', 'widget',
                       'lp.client', 'lp.ui.editor', 'lp.app.date']});

=== modified file 'lib/lp/code/javascript/branchmergeproposal.reviewcomment.js'
--- lib/lp/code/javascript/branchmergeproposal.reviewcomment.js	2015-06-23 12:33:26 +0000
+++ lib/lp/code/javascript/branchmergeproposal.reviewcomment.js	2015-06-29 23:03:03 +0000
@@ -1,4 +1,4 @@
-/* Copyright 2009 Canonical Ltd.  This software is licensed under the
+/* Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
  * GNU Affero General Public License version 3 (see the file LICENSE).
  *
  * Library for code review javascript.
@@ -270,12 +270,16 @@
 
 Y.extend(NumberToggle, Y.Widget, {
     renderUI: function() {
-        var ui = Y.Node.create('<li><label>' +
-            '<input type="checkbox" checked="checked" id="show-no"/>' +
-            '&nbsp;Show line numbers</label></li>');
-        var ul = Y.one('#review-diff div div ul.horizontal');
-        if (ul) {
-            ul.appendChild(ui);
+        var qs = window.location.search;
+        var query = Y.QueryString.parse(qs.replace(/^[?]/, ''));
+        if (!query.ss) {
+            var ui = Y.Node.create('<li><label>' +
+                '<input type="checkbox" checked="checked" id="show-no"/>' +
+                '&nbsp;Show line numbers</label></li>');
+            var ul = Y.one('#review-diff div div ul.horizontal');
+            if (ul) {
+                ul.appendChild(ui);
+            }
         }
     },
     bindUI: function() {
@@ -288,5 +292,5 @@
 
 namespace.NumberToggle = NumberToggle;
 
-}, "0.1", {"requires": ["base", "widget", "lp.anim",
+}, "0.1", {"requires": ["base", "querystring-parse", "widget", "lp.anim",
                         "lp.ui.formoverlay", "lp.app.picker", "lp.client"]});

=== modified file 'lib/lp/code/stories/branches/xx-branchmergeproposals.txt'
--- lib/lp/code/stories/branches/xx-branchmergeproposals.txt	2015-06-05 17:49:22 +0000
+++ lib/lp/code/stories/branches/xx-branchmergeproposals.txt	2015-06-29 23:03:03 +0000
@@ -560,7 +560,7 @@
 The text of the review diff is in the page.
 
     >>> print repr(extract_text(get_review_diff()))
-    u'Preview Diff\n[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk\nDownload diff\n1\n---\n2\n+++\n3\n@@ -0,0 +1 @@\n4\n+Fake Diff\u1010'
+    u'Preview Diff\n[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk\nDownload diff\nSide-by-side diff\n1\n---\n2\n+++\n3\n@@ -0,0 +1 @@\n4\n+Fake Diff\u1010'
 
 There is also a link to the diff URL, which is the preview diff URL plus
 "+files/preview.diff". It redirects logged in users to the file in the

=== modified file 'lib/lp/code/templates/branchmergeproposal-diff.pt'
--- lib/lp/code/templates/branchmergeproposal-diff.pt	2015-05-28 08:35:20 +0000
+++ lib/lp/code/templates/branchmergeproposal-diff.pt	2015-06-29 23:03:03 +0000
@@ -14,10 +14,19 @@
               Download diff
             </a>
           </li>
+          <li tal:condition="not: request/ss|nothing">
+            <a tal:attributes="href string:?ss=1">Side-by-side diff</a>
+          </li>
+          <li tal:condition="request/ss|nothing">
+            <a tal:attributes="href string:?">Unified diff</a>
+          </li>
         </ul>
       </div>
       <div class="attachmentBody">
-        <tal:diff replace="structure view/preview_diff_text/fmt:diff" />
+        <tal:diff condition="not: request/ss|nothing"
+                  replace="structure view/preview_diff_text/fmt:diff" />
+        <tal:ssdiff condition="request/ss|nothing"
+                    replace="structure view/preview_diff_text/fmt:ssdiff" />
         <tal:diff-not-available condition="not: view/diff_available">
             The diff is not available at this time. You can reload the
             page or download it.

=== modified file 'lib/lp/code/templates/codereviewcomment-body.pt'
--- lib/lp/code/templates/codereviewcomment-body.pt	2014-05-28 21:17:40 +0000
+++ lib/lp/code/templates/codereviewcomment-body.pt	2015-06-29 23:03:03 +0000
@@ -7,7 +7,12 @@
   <tal:good-attachments repeat="attachment view/comment/display_attachments">
     <div class="boardComment attachment">
       <div class="boardCommentDetails filename"><a tal:content="attachment/filename" tal:attributes="href attachment/getURL"/></div>
-      <div class="boardCommentBody attachmentBody" tal:content="structure attachment/diff_text/fmt:diff"/>
+      <div class="boardCommentBody attachmentBody"
+           tal:condition="not: request/ss|nothing"
+           tal:content="structure attachment/diff_text/fmt:diff"/>
+      <div class="boardCommentBody attachmentBody"
+           tal:condition="request/ss|nothing"
+           tal:content="structure attachment/diff_text/fmt:ssdiff"/>
     </div>
   </tal:good-attachments>
 

=== modified file 'lib/lp/code/templates/codereviewnewrevisions-footer.pt'
--- lib/lp/code/templates/codereviewnewrevisions-footer.pt	2011-06-30 13:22:07 +0000
+++ lib/lp/code/templates/codereviewnewrevisions-footer.pt	2015-06-29 23:03:03 +0000
@@ -8,6 +8,11 @@
                          show_diff_expander python:True;">
     <metal:landing-target use-macro="branch/@@+macros/branch-revisions"/>
   </tal:revisions>
-  <tal:diff condition="context/diff" replace="structure context/diff/text/fmt:diff" />
+  <tal:has-diff condition="context/diff">
+    <tal:diff condition="not: request/ss|nothing"
+              replace="structure context/diff/text/fmt:diff" />
+    <tal:ssdiff condition="request/ss|nothing"
+                replace="structure context/diff/text/fmt:ssdiff" />
+  </tal:has-diff>
 
 </tal:root>

=== modified file 'lib/lp/services/features/templates/feature-changelog.pt'
--- lib/lp/services/features/templates/feature-changelog.pt	2011-02-22 17:07:35 +0000
+++ lib/lp/services/features/templates/feature-changelog.pt	2015-06-29 23:03:03 +0000
@@ -35,7 +35,10 @@
             <dd class="subordinate">
               <p tal:content="change/comment">comment</p>
 
-              <div tal:content="structure change/diff/fmt:diff" />
+              <div tal:condition="not: request/ss|nothing"
+                   tal:content="structure change/diff/fmt:diff" />
+              <div tal:condition="request/ss|nothing"
+                   tal:content="structure change/diff/fmt:ssdiff" />
             </dd>
           </tal:item>
         </dl>