launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #15883
[Merge] lp:~stevenk/launchpad/inline-diff-comments-ui into lp:launchpad
Steve Kowalik has proposed merging lp:~stevenk/launchpad/inline-diff-comments-ui into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~stevenk/launchpad/inline-diff-comments-ui/+merge/184006
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.
--
https://code.launchpad.net/~stevenk/launchpad/inline-diff-comments-ui/+merge/184006
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/inline-diff-comments-ui into lp:launchpad.
=== 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-09-05 04:21:34 +0000
@@ -628,6 +628,9 @@
margin: 0 0 0 0;
}
+tr.ict-header {
+ background-color: #EEEEEE;
+}
/* === Bugs === */
=== modified file 'lib/lp/app/browser/stringformatter.py'
--- lib/lp/app/browser/stringformatter.py 2012-12-11 05:41:38 +0000
+++ lib/lp/app/browser/stringformatter.py 2013-09-05 04:21:34 +0000
@@ -878,7 +878,7 @@
max_format_lines = config.diff.max_format_lines
header_next = False
for row, line in enumerate(text.splitlines()[:max_format_lines]):
- result.append('<tr>')
+ result.append('<tr id="diff-line-%s">' % (row + 1))
result.append('<td class="line-no">%s</td>' % (row + 1))
if line.startswith('==='):
css_class = 'diff-file text'
=== 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-09-05 04:21:34 +0000
@@ -623,8 +623,10 @@
self.context.source_branch.unique_name),
})
if getFeatureFlag("longpoll.merge_proposals.enabled"):
- cache.objects['merge_proposal_event_key'] = (
- subscribe(self.context).event_key)
+ cache.objects['merge_proposal_event_key'] = subscribe(
+ self.context).event_key
+ if getFeatureFlag("code.inline_diff_comments.enabled"):
+ cache.objects['inline_diff_comments'] = True
@action('Claim', name='claim')
def claim_action(self, action, data):
=== 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-09-05 04:21:34 +0000
@@ -0,0 +1,131 @@
+/* Copyright 2013 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.
+ *
+ * @module lp.code.branchmergeproposal.inlinecomments
+ * @requires node
+ */
+
+YUI.add('lp.code.branchmergeproposal.inlinecomments', function(Y) {
+
+// Grab the namespace in order to be able to expose the connect methods.
+var namespace = Y.namespace('lp.code.branchmergeproposal.inlinecomments');
+
+namespace.add_doubleclick_handler = function() {
+ var inlinecomments = {};
+ var rows = Y.one('.diff').all('tr');
+ var handling_request = false;
+ handler = function(e) {
+ if (handling_request === true) {
+ return;
+ }
+ handling_request = true;
+ var linenumberdata = e.currentTarget.one('.line-no');
+ var rownumber = linenumberdata.get('text');
+ var rows = namespace.create_or_return_row(rownumber, '', '', '');
+ var headerrow = rows[0];
+ var newrow = rows[1];
+ var widget = new Y.EditableText({
+ contentBox: newrow.one('#inlinecomment-' + rownumber + '-draft'),
+ initial_value_override: inlinecomments[rownumber],
+ accept_empty: true,
+ multiline: true,
+ buttons: 'top'
+ });
+ widget.render();
+ handle_widget_button = function(saved, comment) {
+ if (saved === true) {
+ inlinecomments[rownumber] = comment;
+ }
+ if (comment === '') {
+ headerrow.remove(true);
+ newrow.remove(true);
+ }
+ handling_request = false;
+ };
+ widget.editor.on('save', function() {
+ handle_widget_button(true, this.get('value'));
+ });
+ widget.editor.on('cancel', function(e) {
+ handle_widget_button(false, this.get('value'));
+ });
+ widget._triggerEdit(e);
+ };
+ rows.on('dblclick', handler);
+};
+
+namespace.create_or_return_row = function(rownumber, person, comment, date) {
+ var suffix = '-draft';
+ var draft = true;
+ if (person !== '') {
+ suffix = '-' + person.name;
+ draft = false;
+ }
+ var middle = rownumber + suffix;
+ var headerrow = Y.one('#ict-' + middle + '-header');
+ var newrow;
+ if (headerrow === null) {
+ var header = "Draft comment.";
+ if (person !== '' && date !== '') {
+ var personlink = (
+ '<a href="' + person.web_link + '">' + person.display_name +
+ ' (' + person.name + ')</a>');
+ header = "Comment by " + personlink + " on " + date + ".";
+ }
+ headerrow = Y.Node.create(
+ '<tr id="ict-' + middle + '-header" class="ict-header">' +
+ '<td colspan="2">' +
+ '<span class="inlinecomment-' + middle + '-header">' + header +
+ '</span></td></tr>');
+ if (draft === true) {
+ newrow = Y.Node.create(
+ '<tr id="ict-' + middle + '"><td></td><td>' +
+ '<div id="inlinecomment-' + middle + '">' +
+ '<span class="yui3-editable_text-text"></span>' +
+ '<div class="yui3-editable_text-trigger"></div>' +
+ '</div></td></tr>');
+ } else {
+ newrow = Y.Node.create(
+ '<tr id="ict-' + middle + '"><td></td><td>' +
+ '<span>' + comment + '</span></td></tr>');
+ }
+ // We want to have the comments in order after the line, so grab the
+ // next row.
+ var tr = Y.one('#diff-line-' + (parseInt(rownumber, 10) + 1));
+ if (tr !== null) {
+ tr.insert(headerrow, 'before');
+ } else {
+ // If this is the last row, grab the last child.
+ tr = Y.one('.diff>tbody>tr:last-child');
+ tr.insert(headerrow, 'after');
+ }
+ // The header row is the tricky one to place, the comment just goes
+ // after it.
+ headerrow.insert(newrow, 'after');
+ } else {
+ newrow = headerrow.next();
+ }
+ return [headerrow, newrow];
+};
+
+namespace.populate_existing_comments = function() {
+ var i;
+ for (i = 0; i < LP.cache.published_inline_comments.length; i++) {
+ var row = LP.cache.published_inline_comments[i];
+ namespace.create_or_return_row(
+ row.line, row.person, row.comment, row.date);
+ }
+};
+
+namespace.setup_inline_comments = function() {
+ if (LP.cache.inline_diff_comments === true) {
+ // Add the double-click handler for each row in the diff. This needs
+ // to be done first since populating existing published and draft
+ // comments may add more rows.
+ namespace.add_doubleclick_handler();
+ namespace.populate_existing_comments();
+ }
+};
+
+ }, '0.1', {requires: ['event', 'io', 'node', 'widget', 'lp.ui.editor']});
=== 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-09-05 04:21:34 +0000
@@ -0,0 +1,84 @@
+<!DOCTYPE html>
+<!--
+Copyright 2013 Canonical Ltd. This software is licensed under the
+GNU Affero General Public License version 3 (see the file LICENSE).
+-->
+
+<html>
+ <head>
+ <title>code.branchmergeproposal.inlinecomments Tests</title>
+
+ <!-- YUI and test setup -->
+ <script type="text/javascript"
+ src="../../../../../build/js/yui/yui/yui.js">
+ </script>
+ <link rel="stylesheet"
+ href="../../../../../build/js/yui/console/assets/console-core.css" />
+ <link rel="stylesheet"
+ href="../../../../../build/js/yui/test-console/assets/skins/sam/test-console.css" />
+ <link rel="stylesheet"
+ href="../../../../../build/js/yui/test/assets/skins/sam/test.css" />
+
+ <script type="text/javascript"
+ src="../../../../../build/js/lp/app/testing/testrunner.js"></script>
+ <script type="text/javascript"
+ src="../../../../../build/js/lp/app/testing/helpers.js"></script>
+
+ <link rel="stylesheet" href="../../../app/javascript/testing/test.css" />
+
+ <!-- Dependencies -->
+ <script type="text/javascript"
+ src="../../../../../build/js/lp/app/ui/ui.js"></script>
+ <script type="text/javascript"
+ src="../../../../../build/js/lp/app/extras/extras.js"></script>
+ <script type="text/javascript"
+ src="../../../../../build/js/lp/app/anim/anim.js"></script>
+ <script type="text/javascript"
+ src="../../../../../build/js/lp/app/errors.js"></script>
+ <script type="text/javascript"
+ src="../../../../../build/js/lp/app/overlay/overlay.js"></script>
+ <script type="text/javascript"
+ src="../../../../../build/js/lp/app/formoverlay/formoverlay.js"></script>
+ <script type="text/javascript"
+ src="../../../../../build/js/lp/app/formwidgets/resizing_textarea.js"></script>
+ <script type="text/javascript"
+ src="../../../../../build/js/lp/app/ellipsis.js"></script>
+ <script type="text/javascript"
+ src="../../../../../build/js/lp/app/inlineedit/editor.js"></script>
+
+ <!-- The module under test. -->
+ <script type="text/javascript" src="../branchmergeproposal.inlinecomments.js"></script>
+
+ <!-- The test suite -->
+ <script type="text/javascript" src="test_branchmergeproposal.inlinecomments.js"></script>
+
+ <!-- expected variable -->
+ <script type="text/javascript">
+ var LP = {
+ cache: {}
+ };
+ </script>
+
+ </head>
+ <body class="yui3-skin-sam">
+ <ul id="suites">
+ <li>lp.code.branchmergeproposal.inlinecomments.test</li>
+ </ul>
+ <table class="diff">
+ <tbody>
+ <tr id="diff-line-1">
+ <td class="line-no">1</td>
+ <td class="diff-header text">foo bar</td>
+ </tr>
+ <tr id="diff-line-2">
+ <td class="line-no">2</td>
+ <td class="text">baz</td>
+ </tr>
+ <tr id="diff-line-3">
+ <td class="line-no">3</td>
+ <td class="text">quux</td>
+ </tr>
+ </tbody>
+ </table>
+ </body>
+</html>
=== 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-09-05 04:21:34 +0000
@@ -0,0 +1,90 @@
+/* Copyright 2013 Canonical Ltd. This software is licensed under the
+ * GNU Affero General Public License version 3 (see the file LICENSE). */
+
+YUI.add('lp.code.branchmergeproposal.inlinecomments.test', function (Y) {
+
+ var module = Y.lp.code.branchmergeproposal.inlinecomments;
+ var tests = Y.namespace('lp.code.branchmergeproposal.inlinecomments.test');
+ tests.suite = new Y.Test.Suite('branchmergeproposal.inlinecomments Tests');
+
+ tests.suite.add(new Y.Test.Case({
+ name: 'code.branchmergeproposal.inlinecomments_tests',
+
+ setUp: function () {},
+ tearDown: function () {},
+
+ test_library_exists: function () {
+ Y.Assert.isObject(
+ module, "Could not locate the " +
+ "lp.code.branchmergeproposal.inlinecomments module");
+ },
+
+ test_populatation: function () {
+ var published_inline_comments = [
+ {'line': 2, 'person': {'display_name': 'Sample Person',
+ 'name': 'name12', 'web_link': 'http://launchpad.dev/~name12'},
+ 'comment': 'This is preloaded.', 'date': '2012-08-12 17:45'}];
+ LP.cache.published_inline_comments = published_inline_comments;
+ module.populate_existing_comments();
+ Y.Assert.isObject(Y.one('#ict-2-name12-header'));
+ },
+
+ test_handler_normal: function() {
+ module.add_doubleclick_handler();
+ var line_2 = Y.one('#diff-line-2');
+ line_2.simulate('dblclick');
+ var comment = 'This is a comment.';
+ Y.one('.yui3-ieditor-input>textarea').set('value', comment);
+ Y.one('.lazr-pos').simulate('click');
+ Y.Assert.areEqual(
+ comment, Y.one('.yui3-editable_text-text').get('text'));
+ },
+
+ test_handler_cancel: function() {
+ var line_2 = Y.one('#diff-line-2');
+ line_2.simulate('dblclick');
+ var comment = 'Cancelling test.';
+ Y.one('.yui3-ieditor-input>textarea').set('value', comment);
+ Y.one('.lazr-pos').simulate('click');
+ line_2.simulate('dblclick');
+ Y.one('.yui3-ieditor-input>textarea').set('value', 'Foobar!');
+ Y.one('.lazr-neg').simulate('click');
+ Y.Assert.areEqual(
+ comment, Y.one('.yui3-editable_text-text').get('text'));
+ },
+
+ test_handler_cancel_immediately: function() {
+ var line_1 = Y.one('#diff-line-1');
+ line_1.simulate('dblclick');
+ Y.one('.lazr-neg').simulate('click');
+ Y.Assert.isNull(Y.one('#ict-1-draft-header'));
+ },
+
+ test_feature_flag_off: function() {
+ var called = false;
+ add_doubleclick_handler = function() {
+ called = true;
+ };
+ module.add_doubleclick_handler = add_doubleclick_handler;
+ module.setup_inline_comments();
+ Y.Assert.isFalse(called);
+ },
+
+ test_feature_flag: function() {
+ LP.cache.published_inline_comments = [];
+ var called = false;
+ add_doubleclick_handler = function() {
+ called = true;
+ };
+ module.add_doubleclick_handler = add_doubleclick_handler;
+ window.LP.cache.inline_diff_comments = true;
+ module.setup_inline_comments();
+ Y.Assert.isTrue(called);
+ }
+
+ }));
+
+}, '0.1', {
+ requires: ['node-event-simulate', 'test', 'lp.testing.helpers', 'console',
+ 'lp.code.branchmergeproposal.inlinecomments']
+});
=== 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-09-05 04:21:34 +0000
@@ -197,7 +197,8 @@
LPJS.use('io-base', 'lp.code.branchmergeproposal.reviewcomment',
'lp.code.branchmergeproposal.status', 'lp.app.comment',
'lp.code.branchmergeproposal.updater', 'lp.app.widgets.expander',
- 'lp.code.branch.revisionexpander', function(Y) {
+ 'lp.code.branch.revisionexpander',
+ 'lp.code.branchmergeproposal.inlinecomments', function(Y) {
Y.on('load', function() {
var logged_in = LP.links['me'] !== undefined;
@@ -237,6 +238,7 @@
'.expander-content',
false,
Y.lp.code.branch.revisionexpander.bmp_diff_loader);
+ Y.lp.code.branchmergeproposal.inlinecomments.setup_inline_comments();
});
});
-->
=== modified file 'lib/lp/services/features/flags.py'
--- lib/lp/services/features/flags.py 2013-05-22 09:48:07 +0000
+++ lib/lp/services/features/flags.py 2013-09-05 04:21:34 +0000
@@ -209,6 +209,12 @@
'',
'',
''),
+ ('code.inline_diff_comments.enabled',
+ 'space delimited',
+ 'Names of projects have inline diff comments enabled.',
+ '',
+ '',
+ ''),
])
# The set of all flag names that are documented.
Follow ups
