← Back to team overview

launchpad-reviewers team mailing list archive

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

 

Review: Approve code



Diff comments:

> === 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-25 11:16:49 +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-25 11:16:49 +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-25 11:16:49 +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, diff row, per-file row in original
> +    file, per-file row in modified file, line).

"per-file row" means "line number"?

> +    """
> +    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-25 11:16:49 +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-25 11:16:49 +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-25 11:16:49 +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-25 11:16:49 +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-25 11:16:49 +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-25 11:16:49 +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-25 11:16:49 +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-25 11:16:49 +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>
> 


-- 
https://code.launchpad.net/~cjwatson/launchpad/side-by-side-diff/+merge/262768
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.


References