Merge lp:~gmb/launchpad/speed-up-subscription-overlay-bug-719249 into lp:launchpad

[Graham Binns]

[r=allenap][bug=719249] The advanced bug subscription overlay is now pre-populated in order to speed it up a bit.

This branch tweaks the new advanced bug subscription overlay to make it
faster by pre-loading the contents of the overlay when it is initally
created (this is done asynchronously and doesn't affect page render
time).

The overlay is also repopulated after the user makes a change to their
subscription, subscribes or unsubscribes, so that the overlay will
always appear quickly but will also always have the right stuff in it.

[Gavin Panella (allenap)]

Nice improvement.

[1]

+ subscription.enable_spinner();
...
+ subscription.disable_spinner();

This pair runs in the same function; the spinner is not disabled after
some asynchronous activity via a callback or event. I doubt that the
spinner will appear for anything more than a fraction of a second,
possibly not at all.

[2]

+ subscription.disable_spinner();
+
     return subscription_overlay

Not your change, but there's no semi-colon here.

review: Approve

[Graham Binns (gmb)]

> [1]
>
> +    subscription.enable_spinner();
> ...
> +    subscription.disable_spinner();
>
> This pair runs in the same function; the spinner is not disabled after
> some asynchronous activity via a callback or event. I doubt that the
> spinner will appear for anything more than a fraction of a second,
> possibly not at all.

This is a compromise to take care of Rob and Gary's concerns about
this change increasing the time it takes for the JS setup on
BugTask:+index to complete. It will make more of a difference on
production where loading the ++form++ for +subscribe is a bit on the
slow side (that's where I found this bug).

> [2]
>
> +    subscription.disable_spinner();
> +
>     return subscription_overlay
>
> Not your change, but there's no semi-colon here.

Good catch (it was my change, just not in this branch).

[Gavin Panella (allenap)]

> This is a compromise to take care of Rob and Gary's concerns about
> this change increasing the time it takes for the JS setup on
> BugTask:+index to complete. It will make more of a difference on
> production where loading the ++form++ for +subscribe is a bit on the
> slow side (that's where I found this bug).

My point is that that the spinner won't be seen because this function
will complete very quickly, so it's superfluous and doesn't do what
you expect it to.

Of course, I might be misunderstanding it :)

[Graham Binns (gmb)]

On 17 February 2011 14:14, Gavin Panella <email address hidden> wrote:
> My point is that that the spinner won't be seen because this function
> will complete very quickly, so it's superfluous and doesn't do what
> you expect it to.

OIC, right. We need to wait for the async stuff to return. Oops. I'll fix that.

=== modified file 'lib/lp/bugs/javascript/bugtask_index_portlets.js'
--- lib/lp/bugs/javascript/bugtask_index_portlets.js	2011-02-15 18:16:28 +0000
+++ lib/lp/bugs/javascript/bugtask_index_portlets.js	2011-02-18 09:00:00 +0000
@@ -273,6 +273,12 @@
     subscription.set('is_direct', is_direct);
     subscription.set('has_dupes', has_dupes);
 
+    var subscription_overlay;
+    if (namespace.use_advanced_subscriptions) {
+        subscription_overlay = setup_advanced_subscription_overlay(
+            subscription);
+    }
+
     if (subscription.is_node()) {
         subscription.get('link').on('click', function(e) {
             e.halt();
@@ -281,8 +287,6 @@
             subscription.set('is_team', false);
             var parent = e.target.get('parentNode');
             if (namespace.use_advanced_subscriptions) {
-                var subscription_overlay =
-                    setup_advanced_subscription_overlay(subscription);
                 subscription_overlay.show();
             } else {
                 // Look for the false conditions of subscription, which
@@ -491,6 +495,7 @@
  * @param subscription {Object} A Y.lp.bugs.subscriber.Subscription object.
  */
 function setup_advanced_subscription_overlay(subscription) {
+    subscription.enable_spinner("Loading...");
     var subscription_overlay = new Y.lazr.FormOverlay({
         headerContent: '<h2>Subscribe to bug</h2>',
         form_submit_button:
@@ -500,18 +505,41 @@
         centered: true,
         visible: false
     });
+
+    var subscription_link_url = subscription.get(
+        'link').get('href') + '/++form++';
     subscription_overlay.set(
         'form_submit_callback', function(form_data) {
         handle_advanced_subscription_overlay(subscription, form_data);
         subscription_overlay.hide();
+        subscription_overlay.loadFormContentAndRender(
+            subscription_link_url);
     });
 
-    var subscription_link_url = subscription.get(
-        'link').get('href') + '/++form++';
-    subscription_overlay.loadFormContentAndRender(
-        subscription_link_url);
+    // Normally we'd just call loadFormContentAndRender() here, but we
+    // want to be able to have our own callback for when loading the
+    // content is complete.
+    function on_success(id, response) {
+        subscription_overlay.set(
+            'form_content', response.responseText);
+        subscription_overlay.renderUI();
+        subscription_overlay.bindUI();
+        subscription.disable_spinner();
+    }
+    function on_failure(id, response, subscription_overlay) {
+        subscription_overlay.set(
+            'form_content',
+            "Sorry, an error occurred while loading the form.");
+        subscription_overlay.renderUI();
+        subscription.disable_spinner();
+    }
+    var config = {
+        on: {success: on_success, failure: on_failure},
+    }
+    Y.io(subscription_link_url, config);
     subscription_overlay.render('#privacy-form-container');
-    return subscription_overlay
+
+    return subscription_overlay;
 }
 
 /*