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

[Steve Kowalik]

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

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.

[William Grant (wgrant)]

review: Approve (code)

[Samuel Mehrbrodt (sam92)]

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

=== 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 @@
   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	2013-10-04 05:49:30 +0000
+++ lib/lp/app/browser/stringformatter.py	2013-12-12 06:04:37 +0000
@@ -882,7 +882,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/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 @@
                 return result;
             } else {
               if (this.get('use_html')) {
-                return result.getHTML(attribute).get('innerHTML');
+                return Y.Node.create(
+                    result.getHTML(attribute).get('innerHTML'));
               } else {
                 return result.get(attribute);
               }

=== 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 @@
             var text = this.get(TEXT),
                 val  = this.editor.get(VALUE);
             text.setData(ORIGINAL_ELLIPSIS_TEXT, val);
-            text.set('innerHTML', '');
+            text.set('text', '');
             if (this.editor.get(MULTILINE)) {
-                text.set('innerHTML', val);
+                if (Y.Lang.isString(val)) {
+                    text.set('text', val);
+                } else {
+                    text.appendChild(val);
+                }
             } else {
                 text.appendChild(document.createTextNode(val));
             }

=== 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 @@
                     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-12-12 06:04:37 +0000
@@ -0,0 +1,132 @@
+/* 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, null, null, null);
+        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 middle = rownumber + suffix;
+    var draft = true;
+    if (person !== null) {
+        draft = false;
+    }
+    var headerrow = null;
+    if (draft === true) {
+        headerrow = Y.one('#ict-' + middle + '-header');
+        if (headerrow !== null) {
+            return [headerrow, headerrow.next()];
+        }
+    }
+    headerrow = Y.Node.create(
+        '<tr><td colspan="2"></td></tr>').addClass('ict-header');
+    var headerspan;
+    if (person !== null && date !== null) {
+        headerspan = Y.Node.create(
+            '<span>Comment by <a></a> on <span></span></span>');
+        headerspan.one('a').set('href', person.web_link).set(
+            'text', person.display_name + ' (' + person.name + ')');
+        headerspan.one('span').set('text', date);
+    } else {
+        headerspan = Y.Node.create('<span></span>').set(
+            'text', 'Draft comment.');
+    }
+    headerrow.one('td').appendChild(headerspan);
+    if (draft === true) {
+        headerrow.set('id', 'ict-' + middle + '-header');
+        newrow = Y.Node.create('<tr><td></td><td><div>' +
+            '<span class="yui3-editable_text-text"></span>' +
+            '<div class="yui3-editable_text-trigger"></div>' +
+            '</div></td></tr>').set('id', 'ict-' + middle);
+        newrow.one('td>div').set('id', 'inlinecomment-' + middle);
+    } else {
+        newrow = Y.Node.create('<tr><td></td><td><span></span></td></tr>');
+        newrow.one('span').set('text', comment);
+    }
+    // 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');
+    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-12-12 06:04:37 +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-12-12 06:04:37 +0000
@@ -0,0 +1,91 @@
+/* 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': 'name.12', '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();
+            var row = Y.one('#diff-line-2').next().next();
+            Y.Assert.areEqual('This is preloaded.', row.get('text'));
+        },
+
+        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-12-12 06:04:37 +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-08-08 08:49:04 +0000
+++ lib/lp/services/features/flags.py	2013-12-12 06:04:37 +0000
@@ -221,6 +221,12 @@
      'disabled',
      '',
      ''),
+    ('code.inline_diff_comments.enabled',
+     'space delimited',
+     'Names of projects have inline diff comments enabled.',
+     '',
+     '',
+     ''),
     ])
 
 # The set of all flag names that are documented.