launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04247
[Merge] lp:~spiv/launchpad/bmp-inline-diffs into lp:launchpad
Andrew Bennetts has proposed merging lp:~spiv/launchpad/bmp-inline-diffs into lp:launchpad with lp:~danilo/launchpad/expander-anim as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~spiv/launchpad/bmp-inline-diffs/+merge/66634
This still needs some polish, but it's close enough to be worth a review. It's guarded by a feature flag so it ought to be safe to land as is.
It adds inline dynamic diffs to branch index and merge proposal pages. It fetches them from loggerhead (proxied via the lpnet webapp).
It's guarded by a feature flag, the jslint is clean, and it has some tests (although it could use more). I don't think this does anything outrageously wrong, but many of the tools used here are new to me so I could be wrong! So let me know what you think!
--
https://code.launchpad.net/~spiv/launchpad/bmp-inline-diffs/+merge/66634
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~spiv/launchpad/bmp-inline-diffs into lp:launchpad.
=== modified file 'lib/lp/code/browser/branchmergeproposal.py'
--- lib/lp/code/browser/branchmergeproposal.py 2011-07-04 15:48:18 +0000
+++ lib/lp/code/browser/branchmergeproposal.py 2011-07-12 06:38:10 +0000
@@ -38,6 +38,7 @@
from lazr.lifecycle.event import ObjectModifiedEvent
from lazr.restful.interface import copy_field
from lazr.restful.interfaces import (
+ IJSONRequestCache,
IWebServiceClientRequest,
)
import simplejson
@@ -596,6 +597,16 @@
label = "Proposal to merge branch"
schema = ClaimButton
+ def initialize(self):
+ super(BranchMergeProposalView, self).initialize()
+ cache = IJSONRequestCache(self.request)
+ cache.objects.update({
+ 'branch_diff_link':
+ 'https://%s/+loggerhead/%s/diff/' %
+ (config.launchpad.code_domain,
+ self.context.source_branch.unique_name)
+ })
+
@action('Claim', name='claim')
def claim_action(self, action, data):
"""Claim this proposal."""
=== added file 'lib/lp/code/javascript/branch.revisionexpander.js'
--- lib/lp/code/javascript/branch.revisionexpander.js 1970-01-01 00:00:00 +0000
+++ lib/lp/code/javascript/branch.revisionexpander.js 2011-07-12 06:38:10 +0000
@@ -0,0 +1,113 @@
+/* Copyright 2011 Canonical Ltd. This software is licensed under the
+ * GNU Affero General Public License version 3 (see the file LICENSE).
+ *
+ * Expandable branch revisions.
+ *
+ * @module lp.code.branch.revisionexpander
+ * @requires node, lp.client.plugins
+ */
+
+YUI.add('lp.code.branch.revisionexpander', function(Y) {
+
+var namespace = Y.namespace('lp.code.branch.revisionexpander');
+
+function getDiffUrl(rev_number) {
+ // Get the URL of the form
+ // https://code.launchpad.dev/+loggerhead/branch/diff/revno
+ var branch_name = LP.cache.context.unique_name;
+ var branch_url = LP.cache.context.web_link;
+
+ // To construct the URL, we take the branch web link, and insert
+ // +loggerhead/ before the branch name, and /diff/revno
+ // at the end of the URL.
+ return branch_url.replace(branch_name, '+loggerhead/' + branch_name) +
+ '/diff/' + rev_number;
+}
+
+function bmpGetDiffUrl(start_revno, end_revno) {
+ var diff_url = LP.cache.branch_diff_link + end_revno;
+ if (start_revno !== 0) {
+ diff_url += '/' + start_revno;
+ }
+ return diff_url;
+}
+
+function difftext_to_node(difftext) {
+ var node = Y.Node.create('<table class="diff"></table>');
+ var difflines = difftext.split('\n');
+ var i;
+ /* Remove the empty final row caused by a trailing newline
+ * (if it is empty) */
+ if (difflines.length > 0 && difflines[difflines.length-1] === '') {
+ difflines.pop();
+ }
+
+ for (i=0; i < difflines.length; i++) {
+ var line = difflines[i];
+ var line_node = Y.Node.create('<td/>');
+ line_node.set('text', line + '\n');
+ /* Colour the unified diff */
+ var header_pat = /^(===|---|\+\+\+) /;
+ var chunk_pat = /^@@ /;
+ if (line.match(header_pat)) {
+ line_node.addClass('diff-header');
+ } else if (line.match(chunk_pat)) {
+ line_node.addClass('diff-chunk');
+ } else {
+ switch (line[0]) {
+ case '+':
+ line_node.addClass('diff-added');
+ break;
+ case '-':
+ line_node.addClass('diff-removed');
+ break;
+ }
+ }
+ line_node.addClass('text');
+ var row = Y.Node.create('<tr></tr>');
+ row.appendChild(line_node);
+ node.appendChild(row);
+ }
+ return node;
+}
+
+function revision_expander_config(expander){
+ return {
+ on: {
+ success: function nodify_result(diff) {
+ expander.receive(difftext_to_node(diff));
+ },
+ failure: function(trid, response, args) {
+ expander.receive(Y.Node.create('<pre><i>Error</i></pre>'));
+ }
+ }
+ };
+}
+
+function bmp_diff_loader(expander, lp_client) {
+ if (lp_client === undefined) {
+ lp_client = new Y.lp.client.Launchpad();
+ }
+ var rev_no_range = expander.icon_node.get(
+ 'id').replace('expandable-', '').split('-');
+ var start_revno = rev_no_range[0]-1;
+ var end_revno = rev_no_range[1];
+
+ lp_client.get(bmpGetDiffUrl(start_revno, end_revno),
+ revision_expander_config(expander));
+}
+
+function diff_loader(expander, lp_client) {
+ if (lp_client === undefined) {
+ lp_client = new Y.lp.client.Launchpad();
+ }
+ var rev_no = expander.icon_node.get('id').replace('expandable-', '');
+
+ lp_client.get(getDiffUrl(rev_no), revision_expander_config(expander));
+}
+
+namespace.diff_loader = diff_loader;
+namespace.bmp_diff_loader = bmp_diff_loader;
+namespace.difftext_to_node = difftext_to_node;
+
+}, "0.1", {"requires": ["node", "lp.app.widgets.expander"]});
=== added file 'lib/lp/code/javascript/tests/test_branchrevisionexpander.html'
--- lib/lp/code/javascript/tests/test_branchrevisionexpander.html 1970-01-01 00:00:00 +0000
+++ lib/lp/code/javascript/tests/test_branchrevisionexpander.html 2011-07-12 06:38:10 +0000
@@ -0,0 +1,35 @@
+<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd";>
+
+<html>
+ <head>
+ <title>Test page for branch revisionexpander JavaScript unit tests</title>
+
+ <!-- YUI 3.0 Setup -->
+ <script type="text/javascript" src="../../../../canonical/launchpad/icing/yui/yui/yui.js"></script>
+ <script type="text/javascript" src="../../../../canonical/launchpad/icing/lazr/build/lazr.js"></script>
+ <script type="text/javascript" src="../../../app/javascript/client.js"></script>
+ <link rel="stylesheet" href="../../../../canonical/launchpad/icing/yui/cssreset/reset.css"/>
+ <link rel="stylesheet" href="../../../../canonical/launchpad/icing/yui/cssfonts/fonts.css"/>
+ <link rel="stylesheet" href="../../../../canonical/launchpad/icing/yui/cssbase/base.css"/>
+ <link rel="stylesheet" href="../../../../canonical/launchpad/javascript/test.css" />
+ <link rel="stylesheet" href="../../../../canonical/launchpad/icing/lazr/build/lazr-sam.css" />
+
+ <!-- expected variable -->
+ <script type="text/javascript">
+ var LP = {
+ cache: {},
+ links: {}
+ };
+ </script>
+
+ <!-- The module under test -->
+ <script type="text/javascript" src="../branch.revisionexpander.js"></script>
+
+ <!-- The test suite -->
+ <script type="text/javascript" src="test_branchrevisionexpander.js"></script>
+ </head>
+
+<body class="yui3-skin-sam">
+</body>
+</html>
+
=== added file 'lib/lp/code/javascript/tests/test_branchrevisionexpander.js'
--- lib/lp/code/javascript/tests/test_branchrevisionexpander.js 1970-01-01 00:00:00 +0000
+++ lib/lp/code/javascript/tests/test_branchrevisionexpander.js 2011-07-12 06:38:10 +0000
@@ -0,0 +1,106 @@
+/* Copyright 2010 Canonical Ltd. This software is licensed under the
+ * GNU Affero General Public License version 3 (see the file LICENSE).
+ *
+ * Tests for lp.code.branch.revisionexpander.
+ *
+ */
+
+YUI({
+ base: '../../../../canonical/launchpad/icing/yui/',
+ filter: 'raw', combine: false
+ }).use('test', 'console', 'node-event-simulate', 'lp.client',
+ 'lp.code.branch.revisionexpander', function(Y) {
+
+ var module = Y.lp.code.branch.revisionexpander;
+ var suite = new Y.Test.Suite("branch.revisionexpander Tests");
+
+ var MockClient = function() {};
+ MockClient.prototype = {
+ 'calls': [],
+ 'get': function(uri, config) {
+ this.calls.push({'uri': uri});
+ config.on.success(samplediff);
+ }
+ };
+
+ var samplediff = (
+ "=== modified file 'README'\n" +
+ "--- README 2011-01-20 23:05:06 +0000\n" +
+ "+++ README 2011-06-30 10:47:28 +0000\n" +
+ "@@ -1,3 +1,4 @@\n" +
+ "+Green sheep!\n" +
+ " =========\n" +
+ " testtools\n" +
+ " =========\n" +
+ " \n");
+
+ suite.add(new Y.Test.Case({
+ name: 'Test difftext_to_node',
+
+ /*
+ * Unified diffs are rendered to a table, one row per line
+ */
+ test_difftext_to_node_outputs_table: function() {
+ var node = module.difftext_to_node(samplediff);
+ Y.Assert.areEqual('TABLE', node.get('tagName'));
+ Y.Assert.isTrue(node.hasClass('diff'));
+ /* samplediff has 9 lines, so the table will have 9 rows
+ * (trailing newlines don't result in a final row containing an
+ * empty string) */
+ Y.Assert.areEqual(9, node.get('children').size());
+ },
+
+ /*
+ * Diffs are not interpreted as HTML.
+ */
+ test_difftext_to_node_escaping: function() {
+ var node = module.difftext_to_node("<p>hello</p>");
+ var td = node.one('td');
+ Y.Assert.isNull(td.one('p'));
+ }
+ }));
+
+ suite.add(new Y.Test.Case({
+ name: 'Tests for bmp_diff_loader',
+
+ /*
+ * bmp_diff_loader fetches from the URI specified by the div id and
+ * renders a diff.
+ */
+ test_bmp_diff_loader_fetches_from_diff_uri: function() {
+ var FakeExpander = function() {};
+ FakeExpander.prototype = {
+ 'icon_node':
+ Y.Node.create('<div id="expandable-23-45"></div>'),
+ 'receive': function (node) {
+ this.received_node = node;
+ }
+ };
+ var mock_client = new MockClient();
+ var fake_expander = new FakeExpander();
+ LP.cache.branch_diff_link = 'fake-link-base/';
+ module.bmp_diff_loader(fake_expander, mock_client);
+ LP.cache.branch_diff_link = undefined;
+ Y.Assert.areEqual(
+ 'fake-link-base/45/22', mock_client.calls[0].uri);
+ Y.Assert.areEqual(
+ 'TABLE', fake_expander.received_node.get('tagName'));
+ }
+ }));
+
+ var handle_complete = function(data) {
+ window.status = '::::' + JSON.stringify(data);
+ };
+ Y.Test.Runner.on('complete', handle_complete);
+ Y.Test.Runner.add(suite);
+
+ var console = new Y.Console({newestOnTop: false});
+ console.render('#log');
+
+ // Start the test runner on Y.after to ensure all setup has had a
+ // chance to complete.
+ Y.after('domready', function() {
+ Y.Test.Runner.run();
+ });
+});
+
=== modified file 'lib/lp/code/templates/branch-macros.pt'
--- lib/lp/code/templates/branch-macros.pt 2011-06-30 15:34:08 +0000
+++ lib/lp/code/templates/branch-macros.pt 2011-07-12 06:38:10 +0000
@@ -202,6 +202,22 @@
condition="revisions | nothing">
<metal:landing-target use-macro="branch/@@+macros/revision-text"/>
</tal:revision>
+ <tal:ajax-revision-diffs
+ condition="request/features/code.ajax_revision_diffs.enabled">
+ <tal:diff-expander condition="show_diff_expander | nothing">
+ <div class="revision-group-diff" tal:condition="revisions | nothing">
+ <a class="unseen expander-icon"
+ tal:define="start_revno python:revisions[0].sequence;
+ last_revno python:revisions[-1].sequence"
+ tal:attributes="id string:expandable-${start_revno}-${last_revno}"
+ >Changes added by revision <tal:revno replace="start_revno">1</tal:revno>
+ <tal:plural condition="python: start_revno!=last_revno">
+ to revision <tal:revno replace="last_revno">2</tal:revno>
+ </tal:plural></a>
+ <div class="unseen expander-content">Loading diff</div>
+ </div>
+ </tal:diff-expander>
+ </tal:ajax-revision-diffs>
</dl>
</metal:branch-revisions>
@@ -255,6 +271,17 @@
The revision commit message.
</tal:commit-message>
</dd>
+ <div
+ class="revision-expander"
+ tal:condition="request/features/code.ajax_revision_diffs.enabled">
+ <a class="unseen expander-icon"
+ tal:attributes="id string:expandable-${rev_no/sequence}"
+ >Changes</a>
+ <div class="unseen expander-content">
+ <img src="/@@/spinner" alt="Please wait" />
+ Loading diff
+ </div>
+ </div>
<div tal:condition="rev_info | nothing">
<div tal:define="merge_proposal python:rev_info['merge_proposal']"
=== modified file 'lib/lp/code/templates/branchmergeproposal-index.pt'
--- lib/lp/code/templates/branchmergeproposal-index.pt 2011-02-23 22:32:15 +0000
+++ lib/lp/code/templates/branchmergeproposal-index.pt 2011-07-12 06:38:10 +0000
@@ -200,7 +200,8 @@
conf = <tal:status-config replace="view/status_config" />
<!--
LPS.use('io-base', 'lp.code.branchmergeproposal.reviewcomment',
- 'lp.code.branchmergeproposal.status', 'lp.app.comment', function(Y) {
+ 'lp.code.branchmergeproposal.status', 'lp.app.comment',
+ 'lp.app.widgets.expander', 'lp.code.branch.revisionexpander', function(Y) {
Y.on('load', function() {
var logged_in = LP.links['me'] !== undefined;
@@ -219,6 +220,14 @@
(new Y.lp.code.branchmergeproposal.reviewcomment.NumberToggle()
).render();
}, window);
+
+ Y.on('domready', function() {
+ Y.lp.app.widgets.expander.createByCSS(
+ '.revision-group-diff',
+ '.expander-icon',
+ '.expander-content',
+ Y.lp.code.branch.revisionexpander.bmp_diff_loader);
+ });
});
-->
<tal:script replace="structure string:</script>" />
=== modified file 'lib/lp/code/templates/codereviewnewrevisions-footer.pt'
--- lib/lp/code/templates/codereviewnewrevisions-footer.pt 2010-10-06 17:07:11 +0000
+++ lib/lp/code/templates/codereviewnewrevisions-footer.pt 2011-07-12 06:38:10 +0000
@@ -4,7 +4,8 @@
omit-tag="">
<tal:revisions define="branch context/branch;
- revisions context/revisions">
+ revisions context/revisions;
+ 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" />
=== modified file 'lib/lp/services/features/flags.py'
--- lib/lp/services/features/flags.py 2011-06-16 13:50:58 +0000
+++ lib/lp/services/features/flags.py 2011-07-12 06:38:10 +0000
@@ -42,6 +42,10 @@
'boolean',
('Enables the display of bugtracker components.'),
''),
+ ('code.ajax_revision_diffs.enabled',
+ 'boolean',
+ ("Offer expandable inline diffs for branch revisions."),
+ ''),
('code.branchmergequeue',
'boolean',
'Enables merge queue pages and lists them on branch pages.',
