Merge lp:~cprov/launchpad/ic-show-hide into lp:launchpad

[Celso Providelo]

Implements another auxiliary widget to show/hide rendered inline comments.

Fixes presentation order of draft and published inline comments. Drafts are always presented after chronologically-ordered (serve-side) published comments for the context diff line.

As a code refactoring/cleanup, removes DiffNav._showPreviewDiff() 'force' argument, since it's not used (or needed).

[William Grant (wgrant)]

The whole previous().previous().previous().previous()... business is a trainwreck, but it should work for now.

review: Approve (code)

=== modified file 'lib/lp/code/javascript/branchmergeproposal.inlinecomments.js'
--- lib/lp/code/javascript/branchmergeproposal.inlinecomments.js	2014-04-04 05:23:48 +0000
+++ lib/lp/code/javascript/branchmergeproposal.inlinecomments.js	2014-04-24 11:02:44 +0000
@@ -84,19 +84,20 @@
 namespace.create_row = function(comment) {
     var ident = comment.line_number + '-draft';
     var headerrow = Y.Node.create(
-        '<tr><td colspan="2"></td></tr>').addClass('ict-header');
+        '<tr><td class="line-no"></td><td></td></tr>').addClass('ict-header');
     var headerspan;
     var newrow;
     if (Y.Lang.isUndefined(comment.person)) {
         // Creates a draft inline comment area.
         headerspan = Y.Node.create('<span></span>').set(
-            'text', 'Draft comment.');
+            'text', 'Draft comment');
         headerrow.set('id', 'ict-' + ident + '-header');
-        newrow = Y.Node.create('<tr><td></td><td><div>' +
+        newrow = Y.Node.create('<tr><td class="line-no"></td><td><div>' +
             '<span class="yui3-editable_text-text"></span>' +
             '<div class="yui3-editable_text-trigger"></div>' +
             '</div></td></tr>').set('id', 'ict-' + ident);
         newrow.one('td>div').set('id', 'inlinecomment-' + ident);
+        newrow.addClass('draft');
         if (!Y.Lang.isUndefined(comment.text)) {
             namespace.inlinecomments[comment.line_number] = comment.text;
             newrow.one('span').set('text', comment.text);
@@ -123,19 +124,29 @@
         var fmt_date = 'on ' + Y.Date.format(
             new Date(comment.date), {format: '%Y-%m-%d'});
         headerspan.one('span').set('text', fmt_date);
-        newrow = Y.Node.create('<tr><td></td><td><span></span></td></tr>');
+        newrow = Y.Node.create(
+            '<tr><td class="line-no"></td><td><span></span></td></tr>');
         newrow.one('span').set('text', comment.text);
     }
-    headerrow.one('td').appendChild(headerspan);
+    headerrow.one('td:last-child').appendChild(headerspan);
 
     // We want to have the comments in order after the line.
-    var tr = Y.one('#diff-line-' + (parseInt(comment.line_number, 10) + 1));
-    if (tr !== null) {
-        tr.insert(headerrow, 'before');
+    var base_line = Y.one(
+        '#diff-line-' + (parseInt(comment.line_number, 10) + 1));
+    if (base_line !== null) {
+        // If there are drafts, they should remain at the bottom.
+        if (base_line.previous().hasClass('draft')) {
+            base_line = base_line.previous().previous();
+        }
+        base_line.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');
+        base_line = Y.one('.diff>tbody>tr:last-child');
+        // If there are drafts, they should remain at the bottom.
+        if (base_line.hasClass('draft')) {
+            base_line = base_line.previous().previous();
+        }
+        base_line.insert(headerrow, 'after');
     }
     headerrow.insert(newrow, 'after');
 
@@ -207,6 +218,56 @@
 };
 
 
+var InlineCommentToggle = function () {
+    InlineCommentToggle.superclass.constructor.apply(this, arguments);
+};
+
+Y.mix(InlineCommentToggle, {
+
+    NAME: 'inlinecommenttoggle',
+
+    ATTRS: {
+    }
+
+});
+
+Y.extend(InlineCommentToggle, Y.Widget, {
+
+    renderUI: function() {
+        var ui = Y.Node.create('<li><label>' +
+            '<input type="checkbox" checked="checked" id="show-ic"/>' +
+            '&nbsp;Show inline comments</label></li>');
+        var ul = Y.one('#review-diff div div ul.horizontal');
+        if (ul) {
+            ul.appendChild(ui);
+        }
+    },
+
+    bindUI: function() {
+        var cb = Y.one('#show-ic');
+        if (cb === null) {
+            return;
+        }
+        var self = this;
+        cb.on('click', function() {
+            self.showIC(cb.get('checked'));
+        });
+    },
+
+    showIC: function(display) {
+        var css_display = 'none';
+        if (display) {
+            css_display = 'table-row';
+        }
+        Y.all('.ict-header').each( function() {
+            this.setStyle('display', css_display);
+            this.next().setStyle('display', css_display);
+        });
+    }
+});
+
+namespace.InlineCommentToggle = InlineCommentToggle;
+
 
 function DiffNav(config) {
     DiffNav.superclass.constructor.apply(this, arguments);
@@ -239,10 +300,10 @@
                 container.append(navigator);
                 var self = this;
                 navigator.on('change', function() {
-                    self._showPreviewDiff(this.get('value'), false);
+                    self._showPreviewDiff(this.get('value'));
                 });
                 this._connectScrollers();
-                this._showPreviewDiff(navigator.get('value'), true);
+                this._showPreviewDiff(navigator.get('value'));
             }
         }
 
@@ -288,6 +349,7 @@
     set_status_updated: function() {
         var rc = Y.lp.code.branchmergeproposal.reviewcomment;
         (new rc.NumberToggle()).render();
+        (new namespace.InlineCommentToggle()).render();
         if (this.get('previewdiff_id')) {
             namespace.setup_inline_comments(this.get('previewdiff_id'));
         }
@@ -327,7 +389,7 @@
         Y.all('.inline-comment-scroller').each( function(node) {
             rc.link_scroller(node, '#review-diff', function() {
                 self._showPreviewDiff(
-                    node.getAttribute('data-previewdiff-id'), false);
+                    node.getAttribute('data-previewdiff-id'));
             });
         });
     },
@@ -374,7 +436,7 @@
      *
      * @method _showPreviewDiff
      */
-    _showPreviewDiff: function(previewdiff_id, force) {
+    _showPreviewDiff: function(previewdiff_id) {
         var navigator = this.get('navigator');
         if (!Y.Lang.isValue(navigator)) {
            // DiffNav was not properly initialised.

=== modified file 'lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js'
--- lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js	2014-04-04 05:23:48 +0000
+++ lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js	2014-04-24 11:02:44 +0000
@@ -55,7 +55,7 @@
             Y.Assert.areSame(
                 "ws.op=getDraftInlineComments&previewdiff_id=1",
                 mockio.requests[1].config.data);
-            draft_comments = {'3': 'Zoing!'};
+            draft_comments = {'2': 'Boing!', '3': 'Zoing!'};
             mockio.success({
                 responseText: Y.JSON.stringify(draft_comments),
                 responseHeaders: {'Content-Type': 'application/json'}});
@@ -88,9 +88,16 @@
             var text = header.next();
             Y.Assert.areEqual('This is preloaded.', text.get('text'));
 
-            // Draft comment is displayed.
+            // Draft comment for line 2 is displayed after all published
+            // comments.
+            header = text.next()
+            Y.Assert.areEqual('Draft comment', header.get('text'));
+            var text = header.next();
+            Y.Assert.areEqual('Boing!', text.get('text'));
+
+            // Draft comment for line 3 is displayed.
             header = Y.one('#diff-line-3').next();
-            Y.Assert.areEqual('Draft comment.', header.get('text'));
+            Y.Assert.areEqual('Draft comment', header.get('text'));
             text = header.next();
             Y.Assert.areEqual('Zoing!', text.get('text'));
         },
@@ -117,7 +124,7 @@
             // LP was hit and a comment was created.
             Y.Assert.areEqual(1, mockio.requests.length);
             Y.Assert.areEqual(
-                'Draft comment.', Y.one('#ict-1-draft-header').get('text'));
+                'Draft comment', Y.one('#ict-1-draft-header').get('text'));
             Y.Assert.areEqual(
                 'Go!', ic_area.one('.yui3-editable_text-text').get('text'));
 
@@ -283,12 +290,11 @@
             // The local (fake) setup_inline_comments function
             // was called with the selected diff_id value.
             Y.Assert.areEqual(101, this.inline_comments_requested_id);
-            // NumberToggle widget is rendered
-            Y.Assert.isNotNull(Y.one('.diff-content').one('#show-no'));
             // Diff content is rendered/updated.
+            var comment = Y.one(
+                '.diff-content table tbody tr td').next()
             Y.Assert.areSame(
-                 "foo bar",
-                 Y.one('.diff-content table tbody tr td').next().get('text'));
+                 "foo bar", comment.get('text'));
             // The option corresponding to the current 'preview_diff'
             // is selected and contains the expected text (title and
             // formatted date_created).
@@ -321,6 +327,50 @@
             Y.Assert.areEqual(202, this.inline_comments_requested_id);
         },
 
+        test_diff_lineno_show_hide: function() {
+            // NumberToggle widget is rendered as a checked checkbox.
+            // Checking/Unchecking it show or hide the diff
+            // line-numbers cells.
+            var line_no_check = Y.one('.diff-content').one('#show-no');
+            Y.Assert.isTrue(line_no_check.get('checked'));
+            Y.Assert.areEqual(
+                'table-cell', Y.one('td.line-no').getStyle('display'));
+            line_no_check.set('checked', false);
+            line_no_check.simulate('change');
+            // XXX cprov 20140414: a tiny/empty wait seems to be
+            // required for the DOM changes become accessible.
+            this.wait(function() {}, 1);
+            Y.Assert.areEqual(
+                'none', Y.one('td.line-no').getStyle('display'));
+            line_no_check.set('checked', true);
+            line_no_check.simulate('change');
+            this.wait(function() {}, 1);
+            Y.Assert.areEqual(
+                'block', Y.one('td.line-no').getStyle('display'));
+        },
+
+        test_diff_inline_show_hide: function() {
+            // InlineCommentToggle widget is rendered as a checked
+            // checkbox. Checking/Unchecking it show or hide all
+            // rendered inline comments.
+            var ic_check = Y.one('.diff-content').one('#show-ic');
+            Y.Assert.isTrue(ic_check.get('checked'));
+            // Render a draft comment for tests.
+            module.create_row({'line_number': '1', 'text': 'inline'});
+            Y.Assert.areEqual(
+                'table-row', Y.one('tr.ict-header').getStyle('display'));
+            ic_check.set('checked', false);
+            ic_check.simulate('change');
+            this.wait(function() {}, 1);
+            Y.Assert.areEqual(
+                'none', Y.one('tr.ict-header').getStyle('display'));
+            ic_check.set('checked', true);
+            ic_check.simulate('change');
+            this.wait(function() {}, 1);
+            Y.Assert.areEqual(
+                'table-row', Y.one('tr.ict-header').getStyle('display'));
+        },
+
         test_diff_nav_scrollers: function() {
             // The Diff Navigator review comment *scrollers* are connected
             // upon widget rendering and when clicked trigger the diff
@@ -380,16 +430,13 @@
                 rc.window_scroll_anim.fire('end');
             });
             var called_pd_id = null;
-            var called_force = null;
-            this.diffnav._showPreviewDiff = function (pd_id, force) {
+            this.diffnav._showPreviewDiff = function (pd_id) {
                 called_pd_id = pd_id;
-                called_force = force;
             };
             // the new scroller works, it has requested and diff update
             // to the encoded data-previewdiff-id.
             new_scroller.simulate('click');
             Y.Assert.areEqual(202, called_pd_id);
-            Y.Assert.isFalse(called_force);
         },
 
         test_diff_nav_failed_diff_content: function() {