Need notification bar when Filelink upload begins

RESOLVED FIXED in Thunderbird 14.0

Status

Thunderbird
Message Compose Window
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

Trunk
Thunderbird 14.0
x86
All
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird13 fixed)

Details

Attachments

(7 attachments, 4 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Updated

6 years ago
Blocks: 698925
(Assignee)

Updated

6 years ago
Assignee: nobody → mconley
(Assignee)

Comment 1

6 years ago
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.
Attachment #608428 - Flags: feedback?(squibblyflabbetydoo)

Comment 2

6 years ago
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?
Attachment #608428 - Flags: feedback?(squibblyflabbetydoo) → feedback+
(Assignee)

Comment 3

6 years ago
(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...
(Assignee)

Comment 4

6 years ago
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

6 years ago
(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?
(Assignee)

Comment 6

6 years ago
(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

6 years ago
(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?
(Assignee)

Comment 8

6 years ago
(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
(Assignee)

Comment 9

6 years ago
Mark:

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

-Mike
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.
(Assignee)

Comment 11

6 years ago
(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.
(Assignee)

Comment 12

6 years ago
Comment on attachment 608428 [details] [diff] [review]
Patch v1 - with tests!

So are we still OK with a notification bar then?
Attachment #608428 - Flags: ui-review?(bwinton)
Attachment #608428 - Flags: review?(dbienvenu)
(Assignee)

Comment 13

6 years ago
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.
Attachment #608428 - Flags: review?(dbienvenu)
(Assignee)

Comment 14

6 years ago
Created attachment 610960 [details]
Ubuntu - single file uploaded
(Assignee)

Comment 15

6 years ago
Created attachment 610967 [details]
Ubuntu - multiple file
(Assignee)

Comment 16

6 years ago
Created attachment 610980 [details] [diff] [review]
Patch v2

Un-bitrotted.
Attachment #608428 - Attachment is obsolete: true
Attachment #608428 - Flags: ui-review?(bwinton)
(Assignee)

Comment 17

6 years ago
Created attachment 610984 [details]
Win 7 - single file uploaded
(Assignee)

Comment 18

6 years ago
Created attachment 610986 [details]
Win 7 - multiple file upload
(Assignee)

Comment 19

6 years ago
Created attachment 610987 [details]
Win XP - single file uploaded
(Assignee)

Comment 20

6 years ago
Created attachment 610989 [details]
Win XP - multiple file upload
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.
(Assignee)

Comment 22

6 years ago
Created attachment 611956 [details] [diff] [review]
Patch v3

This patch will be bitrotted by bug 740956, which is almost certain to land first.
Attachment #610980 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Depends on: 740956
(Assignee)

Comment 23

6 years ago
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.
Attachment #611956 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #612320 - Flags: ui-review?(bwinton)
Attachment #612320 - Flags: review?(bwinton)
(Assignee)

Updated

6 years ago
Blocks: 742524
(Assignee)

Updated

6 years ago
Blocks: 742459
(Assignee)

Updated

6 years ago
Blocks: 742725
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.
Attachment #612320 - Flags: ui-review?(bwinton)
Attachment #612320 - Flags: ui-review+
Attachment #612320 - Flags: review?(bwinton)
Attachment #612320 - Flags: review+
(Assignee)

Comment 25

5 years ago
(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.
(Assignee)

Comment 26

5 years ago
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
Attachment #612320 - Flags: feedback?(squibblyflabbetydoo)

Comment 27

5 years ago
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?
Attachment #612320 - Flags: feedback?(squibblyflabbetydoo) → feedback+
(Assignee)

Comment 28

5 years ago
(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.
(Assignee)

Comment 29

5 years ago
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.
Attachment #612320 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #615343 - Flags: approval-comm-aurora?
(Assignee)

Comment 30

5 years ago
Committed to comm-central as http://hg.mozilla.org/comm-central/rev/7df52c7b6e4a
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 14.0

Comment 31

5 years ago
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.
Attachment #615343 - Flags: approval-comm-aurora? → approval-comm-aurora+
(Assignee)

Comment 32

5 years ago
Committed to comm-aurora as http://hg.mozilla.org/releases/comm-aurora/rev/359552838186
status-thunderbird13: --- → fixed
You need to log in before you can comment on or make changes to this bug.