launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #18855
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"/>' +
> - ' 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"/>' +
> + ' 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
