launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #18822
[Merge] lp:~cjwatson/launchpad/side-by-side-diff into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/side-by-side-diff into lp:launchpad.
Commit message:
Add an alternative side-by-side preview diff view to merge proposals.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #917502 in Launchpad itself: ""Preview Diff" on code review pages lacks the "Show diffs side-by-side" option"
https://bugs.launchpad.net/launchpad/+bug/917502
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/side-by-side-diff/+merge/262768
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.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/side-by-side-diff into lp:launchpad.
=== 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-23 17:38:36 +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-04-07 23:43:43 +0000
+++ lib/canonical/launchpad/icing/style.css 2015-06-23 17:38:36 +0000
@@ -593,10 +593,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";
@@ -606,7 +608,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-23 17:38:36 +0000
@@ -16,11 +16,15 @@
]
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,
@@ -877,7 +881,7 @@
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
@@ -919,6 +923,127 @@
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">']
+
+ max_format_lines = config.diff.max_format_lines
+ header_next = False
+ queue_removed = []
+ queue_added = []
+ removed_row = 0
+ added_row = 0
+ for row, line in enumerate(text.splitlines()[:max_format_lines]):
+ row += 1
+ css_class = None
+ 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('@@'):
+ try:
+ hunk = hunk_from_header(line + '\n')
+ # The positions indicate the per-file line numbers of
+ # the next row.
+ removed_row = hunk.orig_pos
+ added_row = hunk.mod_pos
+ except Exception:
+ getUtility(IErrorReportingUtility).raising(sys.exc_info())
+ removed_row = 1
+ added_row = 1
+ css_class = 'diff-chunk text'
+ header_next = False
+ elif line.startswith('+'):
+ queue_added.append((row, added_row, line[1:]))
+ added_row += 1
+ continue
+ elif line.startswith('-'):
+ queue_removed.append((row, removed_row, line[1:]))
+ removed_row += 1
+ continue
+ 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
+
+ if css_class is not None:
+ cells = [
+ structured(
+ '<td class="%s" colspan="4">%s</td>',
+ css_class, line).escapedtext,
+ ]
+ else:
+ if line.startswith(' '):
+ line = line[1:]
+ cells = [
+ '<td class="ss-line-no">%s</td>' % removed_row,
+ structured('<td class="text">%s</td>', line).escapedtext,
+ '<td class="ss-line-no">%s</td>' % added_row,
+ structured('<td class="text">%s</td>', line).escapedtext,
+ ]
+ removed_row += 1
+ added_row += 1
+ header_next = False
+
+ if queue_removed or queue_added:
+ self._ssdiff_emit_queued_lines(
+ result, queue_removed, queue_added)
+ queue_removed = []
+ queue_added = []
+ self._ssdiff_emit_line(result, row, cells)
+ if queue_removed or queue_added:
+ self._ssdiff_emit_queued_lines(result, queue_removed, queue_added)
+
+ 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 +1155,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-23 17:38:36 +0000
@@ -310,14 +310,14 @@
def test_almostEmptyString(self):
# White space doesn't count as empty, and is formtted.
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())
@@ -411,6 +411,151 @@
self.assertEqual(3, line_count)
+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 formtted.
+ 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,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.
+ ''')
+ 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])
+
+ def test_config_value_limits_line_count(self):
+ # The config.diff.max_line_format contains the maximum number of lines
+ # to format.
+ diff = dedent('''\
+ === modified file 'tales.py'
+ --- tales.py
+ +++ tales.py
+ @@ -2435,6 +2435,8 @@
+ def format_ssdiff(self):
+ - removed this line
+ + added this line
+ ########
+ # A merge directive comment.
+ ''')
+ self.pushConfig("diff", max_format_lines=3)
+ html = FormattersAPI(diff).format_ssdiff()
+ line_count = html.count('<td class="line-no" style="display: none">')
+ self.assertEqual(3, line_count)
+
+
class TestOOPSFormatter(TestCase):
"""A test case for the oops_id() string formatter."""
=== 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-23 17:38:36 +0000
@@ -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 2014-04-03 20:33:08 +0000
+++ lib/lp/code/javascript/branchmergeproposal.reviewcomment.js 2015-06-23 17:38:36 +0000
@@ -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-23 17:38:36 +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-23 17:38:36 +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-23 17:38:36 +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-23 17:38:36 +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-23 17:38:36 +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>
Follow ups
