Merge lp:~thumper/launchpad/edit-commit-msg-link into lp:launchpad/db-devel

[Tim Penhey]

[Tim Penhey (thumper)]

Change the UI so it doesn't show an empty commit message in the editor, but instead shows a link.

Hooks into the editor events so it knows to flash green or red when the commit message ends up empty.

[Martin Albisetti (beuno)]

This looks great, Tim! Thanks for working on it :)

What do you think about moving the link closer to the information on the MP? (ie, underneath "merge into")?

review: Approve (ui)

[Michael Hudson-Doyle (mwhudson)]

All the interesting stuff is Javascript, which I don't really feel competent to review :( It looks fine, but I don't know anything about our coding standards really.

I'm not sure that the block starting:

+ <div id="commit-message" class="yui-g">
+ <tal:no-commit-message condition="not: context/commit_message">

Can't be better factored, but it's TAL so maybe not.

review: Abstain

[Tim Penhey (thumper)]

On Fri, 27 Nov 2009 16:37:59 Michael Hudson wrote:
> Review: Abstain
> All the interesting stuff is Javascript, which I don't really feel
> competent to review :( It looks fine, but I don't know anything about our
> coding standards really.
>
> I'm not sure that the block starting:
>
> + <div id="commit-message" class="yui-g">
> + <tal:no-commit-message condition="not: context/commit_message">
>
> Can't be better factored, but it's TAL so maybe not.

No it can't really, there are several bits that follow.

  reviewer launchpad

[Māris Fogels (mars)]

Hi Tim,

As we discussed on IRC, your approach is good. However, I do have a question or two about how you chose to integrate the widget into Launchpad.

I'm marking this as Needs Information for now. If you have already considered and confirmed the answers, then feel free to land the code with r=mars.

First, I don't understand why you had to create your own wrapper for the multiline editor. Did the inline editor's activator widget not work? The Activator is used to make the current set of inline editors flash and spin.

Line by line:

• 13: I strongly suggest using a different namespace to store the static reference to the widget. The 'lp' namespace would be an acceptable place.
• 27: Is the message variable reliably an empty string, or could it possibly be null? Check the inline editor's message attribute getter. Does the inline editor have an isEmpty() method?
• 50: Do you want to use e.preventDefault(), or e.halt()?
• 56: Too bad you have to call a protected method to trigger the editor. That is a wart in the API.
• 89: See my above comment about using the 'lp' namespace to host the global variable instead of using Y itself.

var lpns = Y.namespace('lp');
if (!lpns.widgets) { lpns.widgets = {}; }
lpns.widgets['%s'] = widget;

Maris

review: Needs Information (js)

[Tim Penhey (thumper)]

On Sat, 28 Nov 2009 04:54:18 Māris Fogels wrote:
> First, I don't understand why you had to create your own wrapper for the
> multiline editor. Did the inline editor's activator widget not work? The
> Activator is used to make the current set of inline editors flash and
> spin.

I'm sorry. I don't understand this at all. What activator widget?

Do you mean this code?

> var widget = Y.lp.widgets['edit-commit-message'];
> widget.editor.on('save', function() {
> commit_message_listener(this.get('value'), true);
> });
> widget.editor.on('cancel', function() {
> commit_message_listener(this.get('value'), false);
> });

If I try to remove the ".editor" it doesn't work. So I can't just attach to
the "save" or "cancel" button of the widget - which is what I'd like to do.

> Line by line:
>
> • 13: I strongly suggest using a different namespace to store the static
> reference to the widget. The 'lp' namespace would be an acceptable place.

Done.

> • 27: Is the message variable reliably an empty string, or could it
> possibly be null? Check the inline editor's message attribute getter.
> Does the inline editor have an isEmpty() method?

The widget will never give us null, only empty strings.

Since the widget doesn't allow listening on the save or cancel events, they
had to be listened to on the ieditor - and as such the widget is not easily
available to listen to. No the inline editor does not have an isEmpty method.

> • 50: Do you want to use
> e.preventDefault(), or e.halt()?

I don't know. Halt probably, but I was copying from other code. Should we
use halt if we have handled the problem?

> • 56: Too bad you have to call a protected method to trigger the editor.
> That is a wart in the API.

Yep.

Tim

[Māris Fogels (mars)]

Hi Tim,

As discussed on IRC, the calls to commit_message_listener() actually flash the merge link, not the original trigger or message. I couldn't read that in the code, but it makes sense. Perhaps there is some way to make that visual feature a bit clearer.

Using e.halt() is the correct behaviour if you have handled the result. In the case of clicking on a link, you usually want to prevent others from handling a click, so e.halt() is correct.

Please try to clarify the reason for flashing code a bit, and use e.halt(). With those, you will be good to land: r=mars.

Maris

review: Approve (js)

[Francis J. Lacoste (flacoste)]

review: Approve (release-critical)

=== modified file 'lib/canonical/launchpad/javascript/code/codereview.js'
--- lib/canonical/launchpad/javascript/code/codereview.js	2009-11-24 09:30:01 +0000
+++ lib/canonical/launchpad/javascript/code/codereview.js	2009-11-30 23:54:16 +0000
@@ -24,9 +24,67 @@
         link.addClass('js-action');
         link.on('click', show_request_review_form);
     }
+    link = Y.one('.menu-link-set_commit_message');
+    if (link !== null) {
+        link.addClass('js-action');
+        link.on('click', edit_commit_message);
+    }
+    var widget = Y.lp.widgets['edit-commit-message'];
+    widget.editor.on('save', function() {
+            commit_message_listener(this.get('value'), true);
+        });
+    widget.editor.on('cancel', function() {
+            commit_message_listener(this.get('value'), false);
+        });
 };
 
 /*
+ * Hide the commit message editor if the value is empty.
+ *
+ * If the commit message is empty, we want to show the
+ * 'Set commit message' link again.  For consistency with
+ * page updates we want to flash this link so the user can
+ * see what we are doing.  If the commit message was saved
+ * and is empty, then we flash green as all is good.  If the
+ * user has cancelled the edit, and the commit message is
+ * empty, then we flash the link red.
+ */
+function commit_message_listener(message, saved)
+{
+    if (message == '') {
+        // Hide the multiline editor
+        Y.one('#edit-commit-message').addClass('unseen');
+        // Show the link again
+        var link = Y.one('.menu-link-set_commit_message');
+        link.removeClass('unseen');
+        if (saved) {
+            // Flash green.
+            Y.lazr.anim.green_flash({node:link}).run();
+        }
+        else {
+            // Flash red.
+            Y.lazr.anim.red_flash({node:link}).run();
+        }
+    }
+}
+
+/*
+ * Edit the commit message.
+ *
+ * Hide the link, show the multi-line editor, and set it to edit.
+ */
+function edit_commit_message(e) {
+    // We are handling this click event.
+    e.halt();
+    // Make the edit button unseen.
+    Y.one('#edit-commit-message').removeClass('unseen');
+    // Remove the unseen class from the commit message.
+    Y.one('.menu-link-set_commit_message').addClass('unseen');
+    // Trigger the edit on the multiline editor.
+    Y.lp.widgets['edit-commit-message']._triggerEdit(e);
+}
+
+/*
  * Show the "Request a reviewer" overlay.
  */
 function show_request_review_form(e) {
@@ -149,10 +207,15 @@
             '<input type="checkbox" checked="checked" id="show-no"/>' +
             '&nbsp;Show line numbers</label></li>');
         var ul = Y.one('#review-diff div div ul.horizontal');
-        ul.appendChild(ui);
+        if (ul) {
+            ul.appendChild(ui);
+        }
     },
-    bindUI: function(){
-        Y.one('#show-no').on('click', update_nos);
+    bindUI: function() {
+        var cb = Y.one('#show-no');
+        if (cb) {
+            cb.on('click', update_nos);
+        }
     }
 });
 
 
=== modified file 'lib/canonical/widgets/lazrjs.py'
--- lib/canonical/widgets/lazrjs.py	2009-10-28 08:03:36 +0000
+++ lib/canonical/widgets/lazrjs.py	2009-11-30 23:54:16 +0000
@@ -61,7 +61,7 @@
     # Template for the activation script.
     ACTIVATION_TEMPLATE = dedent(u"""\
         <script>
-        YUI().use('lazr.editor', 'lp.client.plugins', function (Y) {
+        LPS.use('lazr.editor', 'lp.client.plugins', function (Y) {
             var widget = new Y.EditableText({
                 contentBox: '#%(id)s',
                 accept_empty: %(accept_empty)s,
@@ -198,7 +198,7 @@
 
     ACTIVATION_TEMPLATE = dedent(u"""\
         <script>
-        YUI().use('lazr.editor', 'lp.client.plugins', function (Y) {
+        LPS.use('lazr.editor', 'lp.client.plugins', function (Y) {
             var widget = new Y.EditableText({
                 contentBox: '#%(id)s',
                 accept_empty: %(accept_empty)s,
@@ -215,6 +215,11 @@
             if (!Y.UA.opera) {
                 widget.render();
             }
+            var lpns = Y.namespace('lp');
+            if (!lpns.widgets) {
+                lpns.widgets = {};
+            }
+            lpns.widgets['%(id)s'] = widget;
         });
         </script>
         """)
 
=== modified file 'lib/lp/app/templates/base-layout-macros.pt'
--- lib/lp/app/templates/base-layout-macros.pt	2009-11-24 09:30:01 +0000
+++ lib/lp/app/templates/base-layout-macros.pt	2009-11-30 23:54:16 +0000
@@ -222,7 +222,9 @@
   <metal:load-lavascript use-macro="context/@@+base-layout-macros/load-javascript" />
 
   <script type="text/javascript">
-    YUI().use('node', 'lp', function(Y) {
+    // Define a global YUI sandbox that should be used by everyone.
+    var LPS = YUI();
+    LPS.use('node', 'lp', function(Y) {
         Y.on('load', function(e) {
             sortables_init();
             initInlineHelp();
 
=== modified file 'lib/lp/code/browser/branchmergeproposal.py'
--- lib/lp/code/browser/branchmergeproposal.py	2009-11-18 04:23:29 +0000
+++ lib/lp/code/browser/branchmergeproposal.py	2009-11-30 23:54:16 +0000
@@ -195,10 +195,10 @@
         return Link('+edit', text, icon='edit', enabled=enabled)
 
     @enabled_with_permission('launchpad.Edit')
-    def edit_commit_message(self):
-        text = 'Edit commit message'
+    def set_commit_message(self):
+        text = 'Set commit message'
         enabled = self.context.isMergable()
-        return Link('+edit-commit-message', text, icon='edit',
+        return Link('+edit-commit-message', text, icon='add',
                     enabled=enabled)
 
     @enabled_with_permission('launchpad.Edit')
@@ -292,7 +292,7 @@
     links = [
         'add_comment',
         'dequeue',
-        'edit_commit_message',
+        'set_commit_message',
         'edit_status',
         'enqueue',
         'merge',
@@ -600,7 +600,7 @@
             self.context,
             'commit_message',
             canonical_url(self.context, view_name='+edit-commit-message'),
-            id="edit-description",
+            id="edit-commit-message",
             title="Commit Message",
             value=commit_message,
             accept_empty=True)
 
=== modified file 'lib/lp/code/stories/branches/xx-branch-merge-proposals.txt'
--- lib/lp/code/stories/branches/xx-branch-merge-proposals.txt	2009-11-23 03:41:39 +0000
+++ lib/lp/code/stories/branches/xx-branch-merge-proposals.txt	2009-11-30 23:54:16 +0000
@@ -100,7 +100,7 @@
     ...     'Add more <b>mojo</b>')
     >>> nopriv_browser.getControl('Update').click()
 
-    >>> print_tag_with_id(nopriv_browser.contents, 'edit-description')
+    >>> print_tag_with_id(nopriv_browser.contents, 'edit-commit-message')
     Commit Message
     Add more &lt;b&gt;mojo&lt;/b&gt;
 
@@ -111,8 +111,9 @@
     ...     '')
     >>> nopriv_browser.getControl('Update').click()
 
-    >>> print_tag_with_id(nopriv_browser.contents, 'edit-description')
-    Commit Message
+    >>> print_tag_with_id(nopriv_browser.contents, 'commit-message')
+    Set commit message
+    ...
 
 
 Deleting merge proposals
 
=== modified file 'lib/lp/code/templates/branchmergeproposal-index.pt'
--- lib/lp/code/templates/branchmergeproposal-index.pt	2009-11-13 03:21:35 +0000
+++ lib/lp/code/templates/branchmergeproposal-index.pt	2009-11-30 23:54:16 +0000
@@ -12,14 +12,17 @@
 
 <metal:block fill-slot="head_epilogue">
   <style type="text/css">
+    .menu-link-set_commit_message {
+    margin-top: 1em;
+    }
     #code-review-votes {
-      margin: 2em 0;
+      margin: 1em 0;
     }
-    #edit-description {
+    #commit-message, #edit-commit-message {
       margin: 1em 0 0 0;
     }
     /* A page-specific fix for inline text are editing to line up box. */
-    #edit-description .yui-ieditor-input { top: 0; }
+    #edit-commit-message .yui-ieditor-input { top: 0; }
   </style>
 </metal:block>
 
@@ -38,12 +41,34 @@
 </metal:side>
 
 
-<div metal:fill-slot="main">
+<div metal:fill-slot="main"
+     tal:define="menu context/menu:context">
 
   <div class="yui-g first">
     <tal:summary replace="structure context/@@+pagelet-summary" />
   </div>
 
+  <div id="commit-message" class="yui-g">
+  <tal:no-commit-message condition="not: context/commit_message">
+    <div tal:define="link menu/set_commit_message"
+         tal:condition="link/enabled"
+         tal:content="structure link/render">
+      Set commit message
+    </div>
+    <div id="edit-commit-message" class="lazr-multiline-edit unseen"
+         tal:content="structure view/commit_message_html"/>
+  </tal:no-commit-message>
+  <tal:has-commit-message condition="context/commit_message">
+    <div tal:define="link menu/set_commit_message"
+         tal:condition="link/enabled"
+         tal:content="structure link/render" class="unseen">
+      Set commit message
+    </div>
+    <div id="edit-commit-message" class="lazr-multiline-edit"
+         tal:content="structure view/commit_message_html"/>
+  </tal:has-commit-message>
+  </div>
+
   <div class="yui-g">
     <div id="votes-target"
          tal:content="structure context/@@+votes" />
@@ -67,15 +92,12 @@
   </div>
 
   <div class="yui-g">
-    <div tal:define="link context/menu:context/add_comment"
+    <div tal:define="link menu/add_comment"
          tal:condition="link/enabled"
          tal:content="structure link/render">
          Add comment
     </div>
 
-    <div id="edit-description" class="lazr-multiline-edit"
-         tal:content="structure view/commit_message_html"/>
-
     <div id="conversation"
          tal:content="structure view/conversation/@@+render"/>
   </div>
@@ -88,7 +110,7 @@
   </div>
 
   <script type="text/javascript">
-    YUI().use('lp.comment', function(Y) {
+    LPS.use('lp.comment', function(Y) {
     var comment = new Y.lp.CodeReviewComment();
     comment.render();
     })
@@ -137,7 +159,7 @@
   string:&lt;script id='codereview-script' type='text/javascript'&gt;" />
   conf = <tal:status-config replace="view/status_config" />
   <!--
-  YUI().use('io-base', 'code.codereview', 'code.branchmergeproposal',
+  LPS.use('io-base', 'code.codereview', 'code.branchmergeproposal',
             function(Y) {
 
 
 
=== modified file 'lib/lp/code/windmill/tests/test_branch_merge_proposal.py'
--- lib/lp/code/windmill/tests/test_branch_merge_proposal.py	2009-11-10 18:10:12 +0000
+++ lib/lp/code/windmill/tests/test_branch_merge_proposal.py	2009-11-30 23:54:16 +0000
@@ -22,12 +22,12 @@
 # There seem to be two textareas rendered for the yui-ieditor-input for some
 # reason.
 EDIT_COMMENT_TEXTBOX = (
-    u'//div[@id="edit-description"]//textarea[@class="yui-ieditor-input"][1]')
+    u'//div[@id="edit-commit-message"]//textarea[@class="yui-ieditor-input"][1]')
 EDIT_COMMENT_SUBMIT = (
-    u'//div[@id="edit-description"]//'
+    u'//div[@id="edit-commit-message"]//'
     'button[contains(@class, "yui-ieditor-submit_button")]')
 COMMIT_MESSAGE_TEXT = (
-    u'//div[@id="edit-description"]//div[@class="yui-editable_text-text"]')
+    u'//div[@id="edit-commit-message"]//div[@class="yui-editable_text-text"]')
 
 
 class TestCommitMessage(TestCaseWithFactory):