Last Comment Bug 738299 - Need notification bar when Filelink upload begins
: Need notification bar when Filelink upload begins
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Message Compose Window (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: Thunderbird 14.0
Assigned To: Mike Conley (:mconley) - (needinfo me!)
:
Mentors:
Depends on: 740956
Blocks: BigFiles 742459 742524 742725
  Show dependency treegraph
 
Reported: 2012-03-22 09:40 PDT by Mike Conley (:mconley) - (needinfo me!)
Modified: 2012-04-16 11:56 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed


Attachments
Patch v1 - with tests! (18.30 KB, patch)
2012-03-22 12:41 PDT, Mike Conley (:mconley) - (needinfo me!)
squibblyflabbetydoo: feedback+
Details | Diff | Review
Ubuntu - single file uploaded (55.83 KB, image/png)
2012-03-30 12:12 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details
Ubuntu - multiple file (84.28 KB, image/png)
2012-03-30 12:18 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details
Patch v2 (18.85 KB, patch)
2012-03-30 13:00 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details | Diff | Review
Win 7 - single file uploaded (7.18 KB, image/png)
2012-03-30 13:14 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details
Win 7 - multiple file upload (6.35 KB, image/png)
2012-03-30 13:15 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details
Win XP - single file uploaded (4.79 KB, image/png)
2012-03-30 13:15 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details
Win XP - multiple file upload (6.26 KB, image/png)
2012-03-30 13:15 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details
Patch v3 (18.45 KB, patch)
2012-04-03 13:34 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details | Diff | Review
Patch v4 (28.96 KB, patch)
2012-04-04 13:41 PDT, Mike Conley (:mconley) - (needinfo me!)
bwinton: review+
bwinton: ui‑review+
squibblyflabbetydoo: feedback+
Details | Diff | Review
Patch v5 (ui-r+/r+ from bwinton, feedback+ from squib) (30.26 KB, patch)
2012-04-16 08:38 PDT, Mike Conley (:mconley) - (needinfo me!)
mozilla: approval‑comm‑aurora+
Details | Diff | Review

Description Mike Conley (:mconley) - (needinfo me!) 2012-03-22 09:40:24 PDT
Since Filelink uploads in their current form do not have a progress bar to indicate how far along we are, we'd like to inform the user that the uploads have indeed begun, and that when completed, the link will be inserted into the body of their email.
Comment 1 Mike Conley (:mconley) - (needinfo me!) 2012-03-22 12:41:24 PDT
Created attachment 608428 [details] [diff] [review]
Patch v1 - with tests!

My first run at it.

Jim - in order to determine whether or not we were uploading one, or more than one file, I had to undo some of your cleverness in the event handling for bigFileObserver.  Let me know if there was another way for me to do that.

I also noticed that there was an attachments-converted event that wasn't being fired in some cases...and we still have an mail:attachmentsConverted nsIObserver notification being sent out...

I didn't remove the nsIObserver notification in this patch, due to the off-chance that someone has already developed something for that notification, and we're going to be (hopefully) landing this in Aurora.
Comment 2 Jim Porter (:squib) 2012-03-26 19:40:21 PDT
Comment on attachment 608428 [details] [diff] [review]
Patch v1 - with tests!

Review of attachment 608428 [details] [diff] [review]:
-----------------------------------------------------------------

This seems reasonable to me. As an alternative, what do you think of implementing this as a doorhanger?
Comment 3 Mike Conley (:mconley) - (needinfo me!) 2012-03-27 08:52:45 PDT
(In reply to Jim Porter (:squib) from comment #2)
> Comment on attachment 608428 [details] [diff] [review]
> Patch v1 - with tests!
> 
> Review of attachment 608428 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This seems reasonable to me. As an alternative, what do you think of
> implementing this as a doorhanger?

So I looked into a doorhanger, and what's holding us back is that PopupNotification.show requires us to pass the browser that contains the selected anchor in as the first argument, and the compose window doesn't have much in the way of a <browser>.

We were, however, able to get away with using doorhangers for TestPilot notifications...unsure how though.

I'll CC Standard8, and see if doorhangers in the compose window is feasible...
Comment 4 Mike Conley (:mconley) - (needinfo me!) 2012-03-27 08:53:34 PDT
Mark:

You used doorhangers in TestPilot in the 3pane...any suggestions on how we might use doorhangers in the compose window?  Is that even possible?

-Mike
Comment 5 David :Bienvenu 2012-03-27 08:55:09 PDT
(In reply to Mike Conley (:mconley) from comment #4)
> Mark:
> 
> You used doorhangers in TestPilot in the 3pane...any suggestions on how we
> might use doorhangers in the compose window?  Is that even possible?
> 
> -Mike

Isn't the attachment warning in the compose window a door hanger?
Comment 6 Mike Conley (:mconley) - (needinfo me!) 2012-03-27 08:57:13 PDT
(In reply to David :Bienvenu from comment #5)
> (In reply to Mike Conley (:mconley) from comment #4)
> > Mark:
> > 
> > You used doorhangers in TestPilot in the 3pane...any suggestions on how we
> > might use doorhangers in the compose window?  Is that even possible?
> > 
> > -Mike
> 
> Isn't the attachment warning in the compose window a door hanger?

Perhaps I'm using the wrong terminology here.

squib:  Did you mean one of these? https://developer.mozilla.org/en/JavaScript_code_modules/PopupNotifications.jsm

That's what I assumed squib meant.  If you meant "doorhanger" like the attachment reminder bar, then this is what my patch already does.
Comment 7 David :Bienvenu 2012-03-27 09:13:42 PDT
(In reply to Mike Conley (:mconley) from comment #6)
> (In reply to David :Bienvenu from comment #5)
> > (In reply to Mike Conley (:mconley) from comment #4)
> > > Mark:
> > > 
> > > You used doorhangers in TestPilot in the 3pane...any suggestions on how we
> > > might use doorhangers in the compose window?  Is that even possible?
> > > 
> > > -Mike
> > 
> > Isn't the attachment warning in the compose window a door hanger?
> 
> Perhaps I'm using the wrong terminology here.
Much more likely that I'm wrong...I guess the attachment stuff is using a notification bar, and doorhangers are a different kind of notification?
Comment 8 Mike Conley (:mconley) - (needinfo me!) 2012-03-27 09:26:01 PDT
(In reply to David :Bienvenu from comment #7)
> ...I guess the attachment stuff is using a
> notification bar, and doorhangers are a different kind of notification?

From my understanding, doorhangers look like this:

https://developer.mozilla.org/@api/deki/files/4905/=popupnotification.png
Comment 9 Mike Conley (:mconley) - (needinfo me!) 2012-03-29 14:08:57 PDT
Mark:

Any ideas for https://bugzilla.mozilla.org/show_bug.cgi?id=738299#c4 ?

-Mike
Comment 10 Mark Banner (:standard8) 2012-03-30 06:15:46 PDT
Actually, test pilot doesn't use doorhangers when within Thunderbird - it uses a custom popup window, the doorhangers are only used for FF 4+ so, we're using the old mechanism.
Comment 11 Mike Conley (:mconley) - (needinfo me!) 2012-03-30 07:17:30 PDT
(In reply to Mark Banner (:standard8) from comment #10)
> Actually, test pilot doesn't use doorhangers when within Thunderbird - it
> uses a custom popup window, the doorhangers are only used for FF 4+ so,
> we're using the old mechanism.

Yeah, I think doorhangers are pretty browser-centric.  I don't think they're an option at this point.
Comment 12 Mike Conley (:mconley) - (needinfo me!) 2012-03-30 11:58:31 PDT
Comment on attachment 608428 [details] [diff] [review]
Patch v1 - with tests!

So are we still OK with a notification bar then?
Comment 13 Mike Conley (:mconley) - (needinfo me!) 2012-03-30 12:00:40 PDT
Comment on attachment 608428 [details] [diff] [review]
Patch v1 - with tests!

Actually, going to cancel the r? until I know for sure this is the UI we want to go with.
Comment 14 Mike Conley (:mconley) - (needinfo me!) 2012-03-30 12:12:43 PDT
Created attachment 610960 [details]
Ubuntu - single file uploaded
Comment 15 Mike Conley (:mconley) - (needinfo me!) 2012-03-30 12:18:28 PDT
Created attachment 610967 [details]
Ubuntu - multiple file
Comment 16 Mike Conley (:mconley) - (needinfo me!) 2012-03-30 13:00:06 PDT
Created attachment 610980 [details] [diff] [review]
Patch v2

Un-bitrotted.
Comment 17 Mike Conley (:mconley) - (needinfo me!) 2012-03-30 13:14:54 PDT
Created attachment 610984 [details]
Win 7 - single file uploaded
Comment 18 Mike Conley (:mconley) - (needinfo me!) 2012-03-30 13:15:15 PDT
Created attachment 610986 [details]
Win 7 - multiple file upload
Comment 19 Mike Conley (:mconley) - (needinfo me!) 2012-03-30 13:15:36 PDT
Created attachment 610987 [details]
Win XP - single file uploaded
Comment 20 Mike Conley (:mconley) - (needinfo me!) 2012-03-30 13:15:59 PDT
Created attachment 610989 [details]
Win XP - multiple file upload
Comment 21 Blake Winton (:bwinton) (:☕️) 2012-04-02 17:44:36 PDT
I think this is mostly the UI we wanted.  Well, that's putting it a little too strongly.  It's mostly the UI we compromised on.

I would like the bar to automatically disappear once the files are uploaded, though.
Comment 22 Mike Conley (:mconley) - (needinfo me!) 2012-04-03 13:34:01 PDT
Created attachment 611956 [details] [diff] [review]
Patch v3

This patch will be bitrotted by bug 740956, which is almost certain to land first.
Comment 23 Mike Conley (:mconley) - (needinfo me!) 2012-04-04 13:41:57 PDT
Created attachment 612320 [details] [diff] [review]
Patch v4

This patch does the following:

1)  We display the notification bar every time an upload or a conversion is initiated (unless mail.compose.big_attachments.insertNotification is set to false.  I'll file a follow-up bug to expose that pref in the Preferences dialog.)
2)  After a file has successfully uploaded, we check to see if there are any other files currently uploading.  If not, we check to ensure that the notification has been displayed for at least 2.5 seconds.  If so, we close it.  If not, we close it once the remaining time of the 2.5 seconds has elapsed.

I've also added a new event, "attachment-upload-failed" that fires on an attachmentitem when a failure occurs.  This way, the notification bar can go away even if failures have happened.
Comment 24 Blake Winton (:bwinton) (:☕️) 2012-04-11 13:30:06 PDT
Comment on attachment 612320 [details] [diff] [review]
Patch v4

So, if I convert attachments from Dropbox back to regular attachments, I see the "These are large files" text again.  Is that a different bug?

Aside from that, it seems pretty good to me, so ui-r=me, with that fixed or a new bug logged.

>+++ b/mail/components/compose/content/MsgComposeCommands.js
>@@ -1223,16 +1235,21 @@ function convertListItemsToCloudAttachme
>+  let bucket = document.getElementById("attachmentBucket");
>+  let event = document.createEvent("CustomEvent");
>+  event.initCustomEvent("attachments-converted", true, true, convertedAttachments);
>+  bucket.dispatchEvent(event);

This is the second or third time I've seen this bit of code.  I wonder if it would make sense to have a utility method…

>@@ -1303,17 +1320,17 @@ function convertListItemsToRegularAttach
>   }
>   let bucket = document.getElementById("attachmentBucket");
>   let event = document.createEvent("CustomEvent");
>   event.initCustomEvent("attachments-converted", true, true, convertedAttachments);
>   bucket.dispatchEvent(event);

Hey, here it is again!  ;)

Other than that, the code seems okay, as far as I can tell.  You might want to get a second opinion from squib or bienvenu, but for what it's worth, r=me.

Thanks,
Blake.
Comment 25 Mike Conley (:mconley) - (needinfo me!) 2012-04-11 13:33:02 PDT
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #24)
> Comment on attachment 612320 [details] [diff] [review]
> Patch v4
> 
> So, if I convert attachments from Dropbox back to regular attachments, I see
> the "These are large files" text again.  Is that a different bug?
> 

Yeah, that's bug 742725 - which relies on this patch, I believe.

> Aside from that, it seems pretty good to me, so ui-r=me, with that fixed or
> a new bug logged.
> 
> >+++ b/mail/components/compose/content/MsgComposeCommands.js
> >@@ -1223,16 +1235,21 @@ function convertListItemsToCloudAttachme
> >+  let bucket = document.getElementById("attachmentBucket");
> >+  let event = document.createEvent("CustomEvent");
> >+  event.initCustomEvent("attachments-converted", true, true, convertedAttachments);
> >+  bucket.dispatchEvent(event);
> 
> This is the second or third time I've seen this bit of code.  I wonder if it
> would make sense to have a utility method…
> 
> >@@ -1303,17 +1320,17 @@ function convertListItemsToRegularAttach
> >   }
> >   let bucket = document.getElementById("attachmentBucket");
> >   let event = document.createEvent("CustomEvent");
> >   event.initCustomEvent("attachments-converted", true, true, convertedAttachments);
> >   bucket.dispatchEvent(event);
> 
> Hey, here it is again!  ;)
> 
> Other than that, the code seems okay, as far as I can tell.  You might want
> to get a second opinion from squib or bienvenu, but for what it's worth,
> r=me.

Alright, I'll feedback? squib and see what he thinks.

> 
> Thanks,
> Blake.
Comment 26 Mike Conley (:mconley) - (needinfo me!) 2012-04-11 13:34:21 PDT
Comment on attachment 612320 [details] [diff] [review]
Patch v4

squib:

In his review to this patch, Blake noticed that we're repeating an attachmentbucket event firing pattern.  He was wondering if it was worth developing a helper function of some kind for it.

Thoughts?

-Mike
Comment 27 Jim Porter (:squib) 2012-04-15 14:39:03 PDT
Comment on attachment 612320 [details] [diff] [review]
Patch v4

Review of attachment 612320 [details] [diff] [review]:
-----------------------------------------------------------------

Unfortunately, I haven't had a chance to run with this yet, but the code looks good.

I think it would be reasonable to add a function like so in MsgComposeCommands.js:

  function dispatchAttachmentBucketEvent(aEventType, aData) {
    let bucket = document.getElementById("attachmentBucket");
    let event = document.createEvent("CustomEvent");
    event.initEvent(aEventType, true, true, aData);
    bucket.dispatchEvent(event);
  }

::: mail/app/profile/all-thunderbird.js
@@ +488,5 @@
>  // cloud links
>  pref("mail.compose.big_attachments.threshold_kb", 1024);
> +// True if the user should be notified that links will be inserted into
> +// their message when the upload is completed
> +pref("mail.compose.big_attachments.insertNotification", true);

I never know what capitalization style to use, but I think we should be self-consistent and just use under_scores here.

::: mail/components/compose/content/bigFileObserver.js
@@ +50,5 @@
> +
> +    let nb = document.getElementById("attachmentNotificationBox");
> +    let notification = nb.getNotificationWithValue(kUploadNotificationValue);
> +    if (notification)
> +      nb.removeNotification(notification);

What's this for, and why do we do it only for the upload notification?

@@ +118,5 @@
> +      if (attachment.sendViaCloud) {
> +        this.attachmentsRemoved([attachment]);
> +        uploaded.push(attachment);
> +      } else {
> +        this.attachmentsAdded([attachment]);

Wasn't this removed in another patch?
Comment 28 Mike Conley (:mconley) - (needinfo me!) 2012-04-16 07:26:34 PDT
(In reply to Jim Porter (:squib) from comment #27)
> Comment on attachment 612320 [details] [diff] [review]
> Patch v4
> 
> Review of attachment 612320 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think it would be reasonable to add a function like so in
> MsgComposeCommands.js:
> 
>   function dispatchAttachmentBucketEvent(aEventType, aData) {
>     let bucket = document.getElementById("attachmentBucket");
>     let event = document.createEvent("CustomEvent");
>     event.initEvent(aEventType, true, true, aData);
>     bucket.dispatchEvent(event);
>   }

Done!

> 
> ::: mail/app/profile/all-thunderbird.js
> @@ +488,5 @@
> >  // cloud links
> >  pref("mail.compose.big_attachments.threshold_kb", 1024);
> > +// True if the user should be notified that links will be inserted into
> > +// their message when the upload is completed
> > +pref("mail.compose.big_attachments.insertNotification", true);
> 
> I never know what capitalization style to use, but I think we should be
> self-consistent and just use under_scores here.

Good idea.  Done.

> 
> ::: mail/components/compose/content/bigFileObserver.js
> @@ +50,5 @@
> > +
> > +    let nb = document.getElementById("attachmentNotificationBox");
> > +    let notification = nb.getNotificationWithValue(kUploadNotificationValue);
> > +    if (notification)
> > +      nb.removeNotification(notification);
> 
> What's this for, and why do we do it only for the upload notification?

While testing, I noticed that if I had an upload notification showing in a compose window, and I closed the window, and opened a new one, the notification would persist in the new window.

If I had to guess, I think it's because there's some spooky optimisation going on in the background where the compose window isn't actually being destroyed.  Pure conjecture, though. 

> 
> @@ +118,5 @@
> > +      if (attachment.sendViaCloud) {
> > +        this.attachmentsRemoved([attachment]);
> > +        uploaded.push(attachment);
> > +      } else {
> > +        this.attachmentsAdded([attachment]);
> 
> Wasn't this removed in another patch?

It is - bug 742725.
Comment 29 Mike Conley (:mconley) - (needinfo me!) 2012-04-16 08:38:10 PDT
Created attachment 615343 [details] [diff] [review]
Patch v5 (ui-r+/r+ from bwinton, feedback+ from squib)

Resolving issues that squib / bwinton found. Unbitrotting due to bug 744004 landing.
Comment 30 Mike Conley (:mconley) - (needinfo me!) 2012-04-16 09:57:33 PDT
Committed to comm-central as http://hg.mozilla.org/comm-central/rev/7df52c7b6e4a
Comment 31 David :Bienvenu 2012-04-16 11:30:08 PDT
Comment on attachment 615343 [details] [diff] [review]
Patch v5 (ui-r+/r+ from bwinton, feedback+ from squib)

I'll let you sort out the landing of patches on aurora in the right order to avoid bit-rotting.
Comment 32 Mike Conley (:mconley) - (needinfo me!) 2012-04-16 11:56:13 PDT
Committed to comm-aurora as http://hg.mozilla.org/releases/comm-aurora/rev/359552838186

Note You need to log in before you can comment on or make changes to this bug.