Merge lp:~stevenk/launchpad/inline-diff-comments-ui into lp:launchpad

Proposed by Steve Kowalik
Status: Merged
Approved by: Steve Kowalik
Approved revision: no longer in the source branch.
Merged at revision: 16871
Proposed branch: lp:~stevenk/launchpad/inline-diff-comments-ui
Merge into: lp:launchpad
Diff against target: 438 lines (+332/-7)
10 files modified
lib/canonical/launchpad/icing/style.css (+3/-0)
lib/lp/app/browser/stringformatter.py (+1/-1)
lib/lp/app/javascript/client.js (+2/-1)
lib/lp/app/javascript/inlineedit/editor.js (+6/-2)
lib/lp/code/browser/branchmergeproposal.py (+4/-2)
lib/lp/code/javascript/branchmergeproposal.inlinecomments.js (+132/-0)
lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.html (+84/-0)
lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js (+91/-0)
lib/lp/code/templates/branchmergeproposal-index.pt (+3/-1)
lib/lp/services/features/flags.py (+6/-0)
To merge this branch: bzr merge lp:~stevenk/launchpad/inline-diff-comments-ui
Reviewer Review Type Date Requested Status
William Grant code Approve
Review via email: mp+184006@code.launchpad.net

Commit message

Add the fully JavaScript UI for inline comments on merge proposal diffs.

Description of the change

Add the fully JavaScript UI for inline comments for diffs. This is protected by a feature flag, so should be safe to land but will change a little, since the server side bits to populate existing comments, save draft comments, and publish them require DB changes and modeling.

I have more then enough LoC to eat the net-positive cost of this branch.

To post a comment you must log in.
Revision history for this message
William Grant (wgrant) :
review: Approve (code)
Revision history for this message
Samuel Mehrbrodt (sam92) wrote :

Hi,
are there any plans to enable this feature on launchpad.net?
Would be really helpful.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/canonical/launchpad/icing/style.css'
--- lib/canonical/launchpad/icing/style.css 2012-08-09 04:56:41 +0000
+++ lib/canonical/launchpad/icing/style.css 2013-12-12 06:04:37 +0000
@@ -628,6 +628,9 @@
628 margin: 0 0 0 0;628 margin: 0 0 0 0;
629}629}
630630
631tr.ict-header {
632 background-color: #EEEEEE;
633}
631634
632/* === Bugs === */635/* === Bugs === */
633636
634637
=== modified file 'lib/lp/app/browser/stringformatter.py'
--- lib/lp/app/browser/stringformatter.py 2013-10-04 05:49:30 +0000
+++ lib/lp/app/browser/stringformatter.py 2013-12-12 06:04:37 +0000
@@ -882,7 +882,7 @@
882 max_format_lines = config.diff.max_format_lines882 max_format_lines = config.diff.max_format_lines
883 header_next = False883 header_next = False
884 for row, line in enumerate(text.splitlines()[:max_format_lines]):884 for row, line in enumerate(text.splitlines()[:max_format_lines]):
885 result.append('<tr>')885 result.append('<tr id="diff-line-%s">' % (row + 1))
886 result.append('<td class="line-no">%s</td>' % (row + 1))886 result.append('<td class="line-no">%s</td>' % (row + 1))
887 if line.startswith('==='):887 if line.startswith('==='):
888 css_class = 'diff-file text'888 css_class = 'diff-file text'
889889
=== modified file 'lib/lp/app/javascript/client.js'
--- lib/lp/app/javascript/client.js 2012-09-07 15:13:40 +0000
+++ lib/lp/app/javascript/client.js 2013-12-12 06:04:37 +0000
@@ -1179,7 +1179,8 @@
1179 return result;1179 return result;
1180 } else {1180 } else {
1181 if (this.get('use_html')) {1181 if (this.get('use_html')) {
1182 return result.getHTML(attribute).get('innerHTML');1182 return Y.Node.create(
1183 result.getHTML(attribute).get('innerHTML'));
1183 } else {1184 } else {
1184 return result.get(attribute);1185 return result.get(attribute);
1185 }1186 }
11861187
=== modified file 'lib/lp/app/javascript/inlineedit/editor.js'
--- lib/lp/app/javascript/inlineedit/editor.js 2013-03-20 03:41:40 +0000
+++ lib/lp/app/javascript/inlineedit/editor.js 2013-12-12 06:04:37 +0000
@@ -1339,9 +1339,13 @@
1339 var text = this.get(TEXT),1339 var text = this.get(TEXT),
1340 val = this.editor.get(VALUE);1340 val = this.editor.get(VALUE);
1341 text.setData(ORIGINAL_ELLIPSIS_TEXT, val);1341 text.setData(ORIGINAL_ELLIPSIS_TEXT, val);
1342 text.set('innerHTML', '');1342 text.set('text', '');
1343 if (this.editor.get(MULTILINE)) {1343 if (this.editor.get(MULTILINE)) {
1344 text.set('innerHTML', val);1344 if (Y.Lang.isString(val)) {
1345 text.set('text', val);
1346 } else {
1347 text.appendChild(val);
1348 }
1345 } else {1349 } else {
1346 text.appendChild(document.createTextNode(val));1350 text.appendChild(document.createTextNode(val));
1347 }1351 }
13481352
=== modified file 'lib/lp/code/browser/branchmergeproposal.py'
--- lib/lp/code/browser/branchmergeproposal.py 2013-04-10 08:09:05 +0000
+++ lib/lp/code/browser/branchmergeproposal.py 2013-12-12 06:04:37 +0000
@@ -623,8 +623,10 @@
623 self.context.source_branch.unique_name),623 self.context.source_branch.unique_name),
624 })624 })
625 if getFeatureFlag("longpoll.merge_proposals.enabled"):625 if getFeatureFlag("longpoll.merge_proposals.enabled"):
626 cache.objects['merge_proposal_event_key'] = (626 cache.objects['merge_proposal_event_key'] = subscribe(
627 subscribe(self.context).event_key)627 self.context).event_key
628 if getFeatureFlag("code.inline_diff_comments.enabled"):
629 cache.objects['inline_diff_comments'] = True
628630
629 @action('Claim', name='claim')631 @action('Claim', name='claim')
630 def claim_action(self, action, data):632 def claim_action(self, action, data):
631633
=== added file 'lib/lp/code/javascript/branchmergeproposal.inlinecomments.js'
--- lib/lp/code/javascript/branchmergeproposal.inlinecomments.js 1970-01-01 00:00:00 +0000
+++ lib/lp/code/javascript/branchmergeproposal.inlinecomments.js 2013-12-12 06:04:37 +0000
@@ -0,0 +1,132 @@
1/* Copyright 2013 Canonical Ltd. This software is licensed under the
2 * GNU Affero General Public License version 3 (see the file LICENSE).
3 *
4 * Code for handling inline comments in diffs.
5 *
6 * @module lp.code.branchmergeproposal.inlinecomments
7 * @requires node
8 */
9
10YUI.add('lp.code.branchmergeproposal.inlinecomments', function(Y) {
11
12// Grab the namespace in order to be able to expose the connect methods.
13var namespace = Y.namespace('lp.code.branchmergeproposal.inlinecomments');
14
15namespace.add_doubleclick_handler = function() {
16 var inlinecomments = {};
17 var rows = Y.one('.diff').all('tr');
18 var handling_request = false;
19 handler = function(e) {
20 if (handling_request === true) {
21 return;
22 }
23 handling_request = true;
24 var linenumberdata = e.currentTarget.one('.line-no');
25 var rownumber = linenumberdata.get('text');
26 var rows = namespace.create_or_return_row(rownumber, null, null, null);
27 var headerrow = rows[0];
28 var newrow = rows[1];
29 var widget = new Y.EditableText({
30 contentBox: newrow.one('#inlinecomment-' + rownumber + '-draft'),
31 initial_value_override: inlinecomments[rownumber],
32 accept_empty: true,
33 multiline: true,
34 buttons: 'top'
35 });
36 widget.render();
37 handle_widget_button = function(saved, comment) {
38 if (saved === true) {
39 inlinecomments[rownumber] = comment;
40 }
41 if (comment === '') {
42 headerrow.remove(true);
43 newrow.remove(true);
44 }
45 handling_request = false;
46 };
47 widget.editor.on('save', function() {
48 handle_widget_button(true, this.get('value'));
49 });
50 widget.editor.on('cancel', function(e) {
51 handle_widget_button(false, this.get('value'));
52 });
53 widget._triggerEdit(e);
54 };
55 rows.on('dblclick', handler);
56};
57
58namespace.create_or_return_row = function(rownumber, person, comment, date) {
59 var suffix = '-draft';
60 var middle = rownumber + suffix;
61 var draft = true;
62 if (person !== null) {
63 draft = false;
64 }
65 var headerrow = null;
66 if (draft === true) {
67 headerrow = Y.one('#ict-' + middle + '-header');
68 if (headerrow !== null) {
69 return [headerrow, headerrow.next()];
70 }
71 }
72 headerrow = Y.Node.create(
73 '<tr><td colspan="2"></td></tr>').addClass('ict-header');
74 var headerspan;
75 if (person !== null && date !== null) {
76 headerspan = Y.Node.create(
77 '<span>Comment by <a></a> on <span></span></span>');
78 headerspan.one('a').set('href', person.web_link).set(
79 'text', person.display_name + ' (' + person.name + ')');
80 headerspan.one('span').set('text', date);
81 } else {
82 headerspan = Y.Node.create('<span></span>').set(
83 'text', 'Draft comment.');
84 }
85 headerrow.one('td').appendChild(headerspan);
86 if (draft === true) {
87 headerrow.set('id', 'ict-' + middle + '-header');
88 newrow = Y.Node.create('<tr><td></td><td><div>' +
89 '<span class="yui3-editable_text-text"></span>' +
90 '<div class="yui3-editable_text-trigger"></div>' +
91 '</div></td></tr>').set('id', 'ict-' + middle);
92 newrow.one('td>div').set('id', 'inlinecomment-' + middle);
93 } else {
94 newrow = Y.Node.create('<tr><td></td><td><span></span></td></tr>');
95 newrow.one('span').set('text', comment);
96 }
97 // We want to have the comments in order after the line, so grab the
98 // next row.
99 var tr = Y.one('#diff-line-' + (parseInt(rownumber, 10) + 1));
100 if (tr !== null) {
101 tr.insert(headerrow, 'before');
102 } else {
103 // If this is the last row, grab the last child.
104 tr = Y.one('.diff>tbody>tr:last-child');
105 tr.insert(headerrow, 'after');
106 }
107 // The header row is the tricky one to place, the comment just goes
108 // after it.
109 headerrow.insert(newrow, 'after');
110 return [headerrow, newrow];
111};
112
113namespace.populate_existing_comments = function() {
114 var i;
115 for (i = 0; i < LP.cache.published_inline_comments.length; i++) {
116 var row = LP.cache.published_inline_comments[i];
117 namespace.create_or_return_row(
118 row.line, row.person, row.comment, row.date);
119 }
120};
121
122namespace.setup_inline_comments = function() {
123 if (LP.cache.inline_diff_comments === true) {
124 // Add the double-click handler for each row in the diff. This needs
125 // to be done first since populating existing published and draft
126 // comments may add more rows.
127 namespace.add_doubleclick_handler();
128 namespace.populate_existing_comments();
129 }
130};
131
132 }, '0.1', {requires: ['event', 'io', 'node', 'widget', 'lp.ui.editor']});
0133
=== added file 'lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.html'
--- lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.html 1970-01-01 00:00:00 +0000
+++ lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.html 2013-12-12 06:04:37 +0000
@@ -0,0 +1,84 @@
1<!DOCTYPE html>
2<!--
3Copyright 2013 Canonical Ltd. This software is licensed under the
4GNU Affero General Public License version 3 (see the file LICENSE).
5-->
6
7<html>
8 <head>
9 <title>code.branchmergeproposal.inlinecomments Tests</title>
10
11 <!-- YUI and test setup -->
12 <script type="text/javascript"
13 src="../../../../../build/js/yui/yui/yui.js">
14 </script>
15 <link rel="stylesheet"
16 href="../../../../../build/js/yui/console/assets/console-core.css" />
17 <link rel="stylesheet"
18 href="../../../../../build/js/yui/test-console/assets/skins/sam/test-console.css" />
19 <link rel="stylesheet"
20 href="../../../../../build/js/yui/test/assets/skins/sam/test.css" />
21
22 <script type="text/javascript"
23 src="../../../../../build/js/lp/app/testing/testrunner.js"></script>
24 <script type="text/javascript"
25 src="../../../../../build/js/lp/app/testing/helpers.js"></script>
26
27 <link rel="stylesheet" href="../../../app/javascript/testing/test.css" />
28
29 <!-- Dependencies -->
30 <script type="text/javascript"
31 src="../../../../../build/js/lp/app/ui/ui.js"></script>
32 <script type="text/javascript"
33 src="../../../../../build/js/lp/app/extras/extras.js"></script>
34 <script type="text/javascript"
35 src="../../../../../build/js/lp/app/anim/anim.js"></script>
36 <script type="text/javascript"
37 src="../../../../../build/js/lp/app/errors.js"></script>
38 <script type="text/javascript"
39 src="../../../../../build/js/lp/app/overlay/overlay.js"></script>
40 <script type="text/javascript"
41 src="../../../../../build/js/lp/app/formoverlay/formoverlay.js"></script>
42 <script type="text/javascript"
43 src="../../../../../build/js/lp/app/formwidgets/resizing_textarea.js"></script>
44 <script type="text/javascript"
45 src="../../../../../build/js/lp/app/ellipsis.js"></script>
46 <script type="text/javascript"
47 src="../../../../../build/js/lp/app/inlineedit/editor.js"></script>
48
49 <!-- The module under test. -->
50 <script type="text/javascript" src="../branchmergeproposal.inlinecomments.js"></script>
51
52 <!-- The test suite -->
53 <script type="text/javascript" src="test_branchmergeproposal.inlinecomments.js"></script>
54
55 <!-- expected variable -->
56 <script type="text/javascript">
57 var LP = {
58 cache: {}
59 };
60 </script>
61
62 </head>
63 <body class="yui3-skin-sam">
64 <ul id="suites">
65 <li>lp.code.branchmergeproposal.inlinecomments.test</li>
66 </ul>
67 <table class="diff">
68 <tbody>
69 <tr id="diff-line-1">
70 <td class="line-no">1</td>
71 <td class="diff-header text">foo bar</td>
72 </tr>
73 <tr id="diff-line-2">
74 <td class="line-no">2</td>
75 <td class="text">baz</td>
76 </tr>
77 <tr id="diff-line-3">
78 <td class="line-no">3</td>
79 <td class="text">quux</td>
80 </tr>
81 </tbody>
82 </table>
83 </body>
84</html>
085
=== added file 'lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js'
--- lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js 1970-01-01 00:00:00 +0000
+++ lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js 2013-12-12 06:04:37 +0000
@@ -0,0 +1,91 @@
1/* Copyright 2013 Canonical Ltd. This software is licensed under the
2 * GNU Affero General Public License version 3 (see the file LICENSE). */
3
4YUI.add('lp.code.branchmergeproposal.inlinecomments.test', function (Y) {
5
6 var module = Y.lp.code.branchmergeproposal.inlinecomments;
7 var tests = Y.namespace('lp.code.branchmergeproposal.inlinecomments.test');
8 tests.suite = new Y.Test.Suite('branchmergeproposal.inlinecomments Tests');
9
10 tests.suite.add(new Y.Test.Case({
11 name: 'code.branchmergeproposal.inlinecomments_tests',
12
13 setUp: function () {},
14 tearDown: function () {},
15
16 test_library_exists: function () {
17 Y.Assert.isObject(
18 module, "Could not locate the " +
19 "lp.code.branchmergeproposal.inlinecomments module");
20 },
21
22 test_populatation: function () {
23 var published_inline_comments = [
24 {'line': 2, 'person': {'display_name': 'Sample Person',
25 'name': 'name.12', 'web_link': 'http://launchpad.dev/~name12'},
26 'comment': 'This is preloaded.', 'date': '2012-08-12 17:45'}];
27 LP.cache.published_inline_comments = published_inline_comments;
28 module.populate_existing_comments();
29 var row = Y.one('#diff-line-2').next().next();
30 Y.Assert.areEqual('This is preloaded.', row.get('text'));
31 },
32
33 test_handler_normal: function() {
34 module.add_doubleclick_handler();
35 var line_2 = Y.one('#diff-line-2');
36 line_2.simulate('dblclick');
37 var comment = 'This is a comment.';
38 Y.one('.yui3-ieditor-input>textarea').set('value', comment);
39 Y.one('.lazr-pos').simulate('click');
40 Y.Assert.areEqual(
41 comment, Y.one('.yui3-editable_text-text').get('text'));
42 },
43
44 test_handler_cancel: function() {
45 var line_2 = Y.one('#diff-line-2');
46 line_2.simulate('dblclick');
47 var comment = 'Cancelling test.';
48 Y.one('.yui3-ieditor-input>textarea').set('value', comment);
49 Y.one('.lazr-pos').simulate('click');
50 line_2.simulate('dblclick');
51 Y.one('.yui3-ieditor-input>textarea').set('value', 'Foobar!');
52 Y.one('.lazr-neg').simulate('click');
53 Y.Assert.areEqual(
54 comment, Y.one('.yui3-editable_text-text').get('text'));
55 },
56
57 test_handler_cancel_immediately: function() {
58 var line_1 = Y.one('#diff-line-1');
59 line_1.simulate('dblclick');
60 Y.one('.lazr-neg').simulate('click');
61 Y.Assert.isNull(Y.one('#ict-1-draft-header'));
62 },
63
64 test_feature_flag_off: function() {
65 var called = false;
66 add_doubleclick_handler = function() {
67 called = true;
68 };
69 module.add_doubleclick_handler = add_doubleclick_handler;
70 module.setup_inline_comments();
71 Y.Assert.isFalse(called);
72 },
73
74 test_feature_flag: function() {
75 LP.cache.published_inline_comments = [];
76 var called = false;
77 add_doubleclick_handler = function() {
78 called = true;
79 };
80 module.add_doubleclick_handler = add_doubleclick_handler;
81 window.LP.cache.inline_diff_comments = true;
82 module.setup_inline_comments();
83 Y.Assert.isTrue(called);
84 }
85
86 }));
87
88}, '0.1', {
89 requires: ['node-event-simulate', 'test', 'lp.testing.helpers', 'console',
90 'lp.code.branchmergeproposal.inlinecomments']
91});
092
=== modified file 'lib/lp/code/templates/branchmergeproposal-index.pt'
--- lib/lp/code/templates/branchmergeproposal-index.pt 2012-06-28 14:19:28 +0000
+++ lib/lp/code/templates/branchmergeproposal-index.pt 2013-12-12 06:04:37 +0000
@@ -197,7 +197,8 @@
197 LPJS.use('io-base', 'lp.code.branchmergeproposal.reviewcomment',197 LPJS.use('io-base', 'lp.code.branchmergeproposal.reviewcomment',
198 'lp.code.branchmergeproposal.status', 'lp.app.comment',198 'lp.code.branchmergeproposal.status', 'lp.app.comment',
199 'lp.code.branchmergeproposal.updater', 'lp.app.widgets.expander',199 'lp.code.branchmergeproposal.updater', 'lp.app.widgets.expander',
200 'lp.code.branch.revisionexpander', function(Y) {200 'lp.code.branch.revisionexpander',
201 'lp.code.branchmergeproposal.inlinecomments', function(Y) {
201202
202 Y.on('load', function() {203 Y.on('load', function() {
203 var logged_in = LP.links['me'] !== undefined;204 var logged_in = LP.links['me'] !== undefined;
@@ -237,6 +238,7 @@
237 '.expander-content',238 '.expander-content',
238 false,239 false,
239 Y.lp.code.branch.revisionexpander.bmp_diff_loader);240 Y.lp.code.branch.revisionexpander.bmp_diff_loader);
241 Y.lp.code.branchmergeproposal.inlinecomments.setup_inline_comments();
240 });242 });
241 });243 });
242 -->244 -->
243245
=== modified file 'lib/lp/services/features/flags.py'
--- lib/lp/services/features/flags.py 2013-08-08 08:49:04 +0000
+++ lib/lp/services/features/flags.py 2013-12-12 06:04:37 +0000
@@ -221,6 +221,12 @@
221 'disabled',221 'disabled',
222 '',222 '',
223 ''),223 ''),
224 ('code.inline_diff_comments.enabled',
225 'space delimited',
226 'Names of projects have inline diff comments enabled.',
227 '',
228 '',
229 ''),
224 ])230 ])
225231
226# The set of all flag names that are documented.232# The set of all flag names that are documented.