736169 - Filelink uploads should have a progress bar

Status: ASSIGNED

(Thunderbird :: FileLink, enhancement)

Description

[Mike Conley (:mconley) (:⚙️)]

Right now, we just have a spinner indicating that a file is being uploaded, but the user is not given any feedback regarding how far along the upload is.  This can be particularly frustrating with large files.

We should have a progress bar instead - perhaps behind the attachmentitem in the attachment bucket.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Hey car, any update on this? How are things coming? Email me if you're having any difficulty.

Comment 2

[zeyu [:car]]

Hi Mike,

Sorry for disappearing for a while, I just survived from my midterm tests :P. I replaced the uuid and modified the uploadFile in the interface, what should I do next?

Comment 3

[Mike Conley (:mconley) (:⚙️)]

(In reply to zeyu [:car] from comment #2)
> Hi Mike,
> 
> Sorry for disappearing for a while, I just survived from my midterm tests
> :P. I replaced the uuid and modified the uploadFile in the interface, what
> should I do next?

That's OK. Glad you survived!

The next step is to modify the YouSendIt, Box and UbuntuOne implementations to use the new interface. You'll also need to modify the current tests, or add new ones, to accomodate this new behaviour.

Pick any one, and then attach the progress listener object to the XMLHttpRequest that's being used to listen to that upload.

Once you've done that, we need to hook up the UI - I can guide you through that once the above step is done for at least one Filelink provider.

Can you attach a work-in-progress patch of whatever you have already (I know it's not much), just so I can look at it? Thanks!

Comment 4

[zeyu [:car]]

Attachment: modify nsIMsgCloudFileProvider.idl

Comment 5

[zeyu [:car]]

Attachment: add XMLHttpRequest to monitor progress

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 670787 [details] [diff] [review]
add XMLHttpRequest to monitor progress

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

::: mail/components/cloudfile/nsUbuntuOne.js
@@ +129,5 @@
>  
>      // Some ugliness here - we stash requestObserver here, because we might
>      // use it again in _getUserInfo.
>      this.requestObserver = aCallback;
> +    var req = new XMLHttpRequest();

Hm, I don't think we want this here.

We're going to have to make further alterations. Ubuntu One, for example, uses our OAuth module in order to communicate with the Ubuntu One server. The OAuth module makes use of our "http.jsm" module, which simplifies the XMLHttpRequest process.

So what we'll need to do is modify oauth.jsm (mail/base/modules/oauth.jsm) and doXHRequest in http.jsm (mail/base/modules/http.jsm) to take a new parameter which will be our progress listening function.

Then, in here, in nsUbuntuOne.js, we'll need to pass aListener to the nsUbuntuOneFileUploader, which will pass it to OAuth.jsm (via signAndSend), which will pass it to http.jsm (via doXHRequest).

It's a long twisted road, but in the end, it'll get us attaching the progress listener to the right XMLHttpRequest object.

Comment 7

[zeyu [:car]]

So as florian said, I can use this.request.addEventListener("progress", aListener, false) to attach aListener to the XMLHttpRequest. My question is, how can I make use of it?

Comment 8

[Mike Conley (:mconley) (:⚙️)]

(In reply to zeyu [:car] from comment #7)
> So as florian said, I can use this.request.addEventListener("progress",
> aListener, false) to attach aListener to the XMLHttpRequest. My question is,
> how can I make use of it?

We'll worry about that at the caller level. We just need to make sure each Filelink component is prepared to deal with the listener, and then in the portion of the front-end code that calls uploadFile, we'll pass in the listener.

Get the other back-end components ready, and then we'll go through the front-end bit.  And then testing!

Comment 9

[zeyu [:car]]

Attachment: attach listener

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 672834 [details] [diff] [review]
attach listener

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

So far so good.

You can either fix up the rest of the providers (YouSendIt or Box), or, we can focus on the UI, and do YouSendIt / Box afterwards.

::: mail/components/cloudfile/nsIMsgCloudFileProvider.idl
@@ +37,5 @@
>     * @param aCallback callback when finished.
>     *
>     * @throws nsIMsgCloudFileProvider.offlineErr if we are offline.
>     */
> +  void uploadFile(in nsIFile aFile, in nsIRequestObserver aCallback, in nsIDOMEvnetListener aListener);

I doubt this will compile properly... typo - nsIDOMEvnetListener -> nsIDOMEventListener

::: mail/components/cloudfile/nsUbuntuOne.js
@@ +141,5 @@
>        return;
>      }
>      this._uploadingFile = aFile;
> +    //attach progress listener
> +    this.request.addEventListener("progress", aListener, false);

This should probably be optional. Only attach the listener if one exists.

Comment 11

[zeyu [:car]]

Attachment: attach listener only when aListener exits

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 672838 [details] [diff] [review]
attach listener only when aListener exits

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

::: mail/components/cloudfile/nsUbuntuOne.js
@@ +142,5 @@
>      }
>      this._uploadingFile = aFile;
> +    //attach progress listener
> +    if (aListener) {
> +    this.request.addEventListener("progress", aListener, false);

Hm, actually, so in this scope, there's no such thing as "this.request", so this whole thing doesn't make too much sense.

The request is actually created and managed within nsUbuntuOneFileUploader.

So what you should do is pass this listener to nsUbuntuOneFileUploader, and get it to hold a reference to it. Then, in the uploadFile function of nsUbuntuOneFileUploader, attach the listener (if one exists).

Comment 13

[zeyu [:car]]

Attachment: modify nsUbuntuOneFileUploader constructor

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 672852 [details] [diff] [review]
modify nsUbuntuOneFileUploader constructor

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

::: mail/components/cloudfile/nsUbuntuOne.js
@@ +141,5 @@
>        return;
>      }
>      this._uploadingFile = aFile;
> +    //attach progress listener
> +    if (aListener) {

We don't need to do this here anymore.

@@ +673,5 @@
>  
>    /**
>     * Kicks off the upload request for the file associated with this Uploader.
>     */
>    uploadFile: function nsU1FU_uploadFile() {

You still need to attach the listener in here.

Comment 15

[zeyu [:car]]

Attachment: attach listener in nsU1FU_uploadFile

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 672863 [details] [diff] [review]
attach listener in nsU1FU_uploadFile

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

One thing to fix, and then I think you've got the basics of it.

This will, of course, need a full review when it's all done - but I'm giving feedback+ (with the correction below) because you're on the right track.

::: mail/components/cloudfile/nsUbuntuOne.js
@@ +710,5 @@
>                          Ci.nsIMsgCloudFileProvider.uploadErr);
>        }.bind(this), this);
> +    //attach progress listener
> +    if (aListener) {
> +      this.request.addEventListener("progress", aListener, false);

aListener doesn't exist in this scope. You need to use this.listener.

Comment 17

[zeyu [:car]]

Attachment: use this.listener instead of aListener

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Jim:

You know the attachmentitem binding better than anyone else I know.

What's the practicality of modifying the binding for allowing a progress bar to sit behind the filename while an upload is in progress?

(Assume for a second that we use XUL's progressmeter, but override its moz-appearance for readability...not sure if that's the right approach, but it's the one I have in mind).

-Mike

Comment 19

[Jim Porter (:squib)]

I'd rather we make another binding that inherits from attachmentitem, and then bind that with `attachmentitem[uploading]`. However, this would mean that whenever I got around to adding inline renaming for attachments, you wouldn't be able to rename an attachment as it's uploading (I do the same sort of thing there). I think that's probably ok though, since renaming a cloud attachment in the attachment list isn't a very good idea anyway, unless you want to confuse people.

Comment 20

[Mike Conley (:mconley) (:⚙️)]

car:

Ok, so the next step is to create the XBL binding that Jim is talking about. That binding will have some kind of progress bar (we'll probably have to hand-roll our own) that it will display behind the attachmentitem's filename.

Now would be an excellent time to read up on XBL:

https://developer.mozilla.org/en-US/docs/XBL

And study our attachmentitem binding:

http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWidgets.xml#417

-Mike

Comment 21

[zeyu [:car]]

Attachment: attach listener

Comment 22

[zeyu [:car]]

Hi Mike,

I think in the previous patch the listener was not properly attached. MDN suggests that a upload progress event listener should be attached in this way:

req.upload.addEventListener("progress",listener,false) 
insteand of 
req.addEventListener("progress",listener,false)

I tried this but still not working. I suspect that after signAndSent, this.requst.opend() has been fired. And if it was fired then we cannot attach a listener to it.

Comment 23

[zeyu [:car]]

*this.request.open()

Comment 24

[zeyu [:car]]

Hi Mike,

I checked signAndSend() in oauth.jsm,
http://mxr.mozilla.org/comm-central/source/mail/base/modules/oauth.jsm#89

it returns  doXHRequest in http.jsm
http://mxr.mozilla.org/comm-central/source/mail/base/modules/http.jsm#9

which did fire send by xhr.send(). So I think we cannot attach a listener to this.request after signAndSend. So I wonder if there is any alternative way to attach the listener?

Comment 25

[Mike Conley (:mconley) (:⚙️)]

Gar - you're right. According to MDN:

"You need to add the event listeners before calling open() on the request.  Otherwise the progress events will not fire."

So, we might have to go back to Plan A (passing the event listeners to oauth and http.jsm), unless Florian has a better suggestion?

Comment 26

[Florian Quèze [:florian]]

(In reply to Mike Conley (:mconley) from comment #25)
> Gar - you're right. According to MDN:
> 
> "You need to add the event listeners before calling open() on the request. 
> Otherwise the progress events will not fire."

It would be nice to know if this is part of a spec or if it's a bug.

It's strange that the behavior is not the same for progress events of the data we are sending (apparently the listener needs to be set before starting the request) and progress events of the data we receive (I'm sure we can set them after starting the request; otherwise our twitter code wouldn't work at all).

> So, we might have to go back to Plan A (passing the event listeners to oauth
> and http.jsm), unless Florian has a better suggestion?

I'm afraid we will have to do that.

Comment 27

[zeyu [:car]]

Attachment: My work so far, though percentComplete still not there

Comment 28

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 679104 [details] [diff] [review]
My work so far, though percentComplete still not there

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

::: mail/base/modules/http.jsm
@@ +11,5 @@
>                .createInstance(Ci.nsIXMLHttpRequest);
>    xhr.mozBackgroundRequest = true; // no error dialogs
> +  if (aListener) {
> +  dump("aListener is defined, http.jsm");
> +  xhr.upload.addEventListener("progress", aListener, false);

I don't think xhr.upload exists...I think it should just be:

xhr.addEventListener("progress", aListener, false);

::: mail/base/modules/oauth.jsm
@@ +156,5 @@
>  
>      let authorization =
>        "OAuth " + params.map(function (p) p[0] + "=\"" + p[1] + "\"").join(", ");
>      let headers = (aHeaders || []).concat([["Authorization", authorization]]);
> +    if(aListener) {

The existence of aListener is checked in http.jsm, I don't think we need to do it here.

Comment 29

[Mike Conley (:mconley) (:⚙️)]

(In reply to Mike Conley (:mconley) from comment #28)
> Comment on attachment 679104 [details] [diff] [review]
> My work so far, though percentComplete still not there
> 
> Review of attachment 679104 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mail/base/modules/http.jsm
> @@ +11,5 @@
> >                .createInstance(Ci.nsIXMLHttpRequest);
> >    xhr.mozBackgroundRequest = true; // no error dialogs
> > +  if (aListener) {
> > +  dump("aListener is defined, http.jsm");
> > +  xhr.upload.addEventListener("progress", aListener, false);
> 
> I don't think xhr.upload exists...I think it should just be:
> 
> xhr.addEventListener("progress", aListener, false);
> 

I'm totally wrong, and you're totally right. xhr.upload.addEventListener("progress", function) is a thing you can do. Sorry about that.

Comment 30

[zeyu [:car]]

Attachment: WiP

In the patch I have a dump(this.listener) in nsU1FU_uploadFile().

However when I try to upload to UbuntuOne I always get "undefined". So the listener was still not properly attached? But I think my approach is correct, kind of confused.

Comment 31

[zeyu [:car]]

Attachment: WIP

I modified nsBox.js in this patch, nsBox.js did not use http.jsm and auth.jsm, it used XHRequest in its nsBoxFileUploader directly. However it still did not work. When I check the this.listener in both Box & UbuntuOne uploader, both said the listener is not defined.

Comment 32

[zeyu [:car]]

Hi Mike,

Can you give me some feedback? I suspect that the way the listener is defined is not correct, since I attached the listener for Box which is simpler to do because it did not use any external modules.

Comment 33

[Mike Conley (:mconley) (:⚙️)]

(In reply to zeyu [:car] from comment #32)
> Hi Mike,
> 
> Can you give me some feedback? I suspect that the way the listener is
> defined is not correct, since I attached the listener for Box which is
> simpler to do because it did not use any external modules.

Hey car,

Yeah - I'll try poking at this tonight or tomorrow night. Hopefully we can get to the bottom of this.

Thanks,

-Mike

Comment 34

[Mike Conley (:mconley) (:⚙️)]

Attachment: Semi-working patch for UbuntuOne

Hey car,

I had some time tonight, so I took your patch and ran with it for a bit. I was able to get it working for the Ubuntu One provider - note that it only works when using the Attach > Filelink > Ubuntu One menu path.

We'll need to hook up a listener for the other paths as well, but we'll deal with that later. Take a look at the difference between our two patches, and let me know if you have any questions.

Comment 35

[zeyu [:car]]

Attachment: Working for all 3 providers

This patch works for all the 3 providers and work both by directly attaching a file or convert a attachment into a cloudfile.

Comment 36

[zeyu [:car]]

Attachment: layout 1

I got two layouts for progressbar.

Comment 37

[zeyu [:car]]

Attachment: layout 2

In this pic, the file name is inside the progressbar. Which one do you think is better? I prefer the second one because the first one is a bit too long.

Comment 38

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 692585 [details] [diff] [review]
Working for all 3 providers

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

Just some nits, but I think we're ready to start showing some progress bars.

What I'd suggest is extending the attachment item so that the label for the uploaded file appears over a progressmeter - you can do this with a XUL stack. We'll also want to override the native styles of the progressmeters, because text over top of those native progressmeters is likely to be unreadable.

::: mail/base/modules/http.jsm
@@ +10,5 @@
>    var xhr = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"]
>                .createInstance(Ci.nsIXMLHttpRequest);
>    xhr.mozBackgroundRequest = true; // no error dialogs
> +  if (aListener) {
> +  xhr.upload.addEventListener("progress", aListener, false);

Indent line 14 two spaces. Trim whitespace on line 15.

::: mail/components/cloudfile/nsBox.js
@@ +163,5 @@
>  
>      // if we're uploading a file, queue this request.
>      if (this._uploadingFile && this._uploadingFile != aFile) {
>        let uploader = new nsBoxFileUploader(this, aFile,
>                                                 this._uploaderCallback

The indentation on lines 167 - 169 looks a bit off.

Should be:

let uploader = new nsBoxFileUploader(this, aFile,
                                     this._uploaderCallback.bind(this),
                                     aCallback, aListener);

@@ +720,5 @@
>    },
>  };
>  
>  function nsBoxFileUploader(aBox, aFile, aCallback,
> +                                 aRequestObserver, aListener) {

More busted indentation here. Should be:

function nsBoxFileUploader(aBox, aFile, aCallback,
                           aRequestObserver, aListener) {

::: mail/components/cloudfile/nsIMsgCloudFileProvider.idl
@@ +12,1 @@
>  interface nsIMsgCloudFileProvider : nsISupports {

Put a newline before line 12.

@@ +38,5 @@
>     * @param aCallback callback when finished.
>     *
>     * @throws nsIMsgCloudFileProvider.offlineErr if we are offline.
>     */
> +  void uploadFile(in nsIFile aFile, in nsIRequestObserver aCallback, [optional] in nsIDOMEventListener aListener);

Let's break this up over two lines.

::: mail/components/cloudfile/nsUbuntuOne.js
@@ -6,5 @@
>   *
>   * This component handles the UbuntuOne implementation of the
>   * nsIMsgCloudFileProvider interface.
>   */
> -

Why was this line removed?

@@ +102,5 @@
>        this._uploadingFile = nextUpload.file;
>        this._uploader = nextUpload;
>        try {
> +        this.uploadFile(nextUpload.file, nextUpload.callback,
> +            nextUpload.listener);

Fix the indentation here.

@@ +116,2 @@
>  
>    /** 

While you're here, please trim this whitespace.

@@ +125,2 @@
>      if (Services.io.offline)
>        throw Ci.nsIMsgCloudFileProvider.offlineErr;

Please put these newlines back.

::: mail/components/cloudfile/nsYouSendIt.js
@@ +232,5 @@
>        return this._fileExceedsLimit(aCallback, '2GB', 0);
>      if (aFile.fileSize > this._maxFileSize)
>        return this._fileExceedsLimit(aCallback, 'Limit', 0);
>      if (aFile.fileSize > this._availableStorage)
>        return this._fileExceedsLimit(aCallback, 'Quota', 

Let's trim the whitespace here while we're at it.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +1240,5 @@
>    if (lastDirectory)
>      fp.displayDirectory = lastDirectory;
>  
>    let files = [];
> +  

Please trim this whitespace.

@@ +1255,5 @@
>      let i = 0;
>      let items = AddAttachments(attachments, function(aItem) {
>        let listener = new uploadListener(attachments[i], files[i], aProvider);
>        try {
> +	      let progressListener = function progressListener(aEvent) {

The indentation in lines 1259-1268 is really off. We use two-space indentation. Also, we prefer using let over var.

@@ +1286,5 @@
>   * @param aProvider the cloud provider to upload the files to
>   */
>  function convertListItemsToCloudAttachment(aItems, aProvider)
>  {
> +  dump("startuploading");

Remove this if you don't need it anymore.

Comment 39

[zeyu [:car]]

Hi Mike,

Did you see my last email?

Comment 40

[Mike Conley (:mconley) (:⚙️)]

(In reply to zeyu [:car] from comment #39)
> Hi Mike,
> 
> Did you see my last email?

Please check your mail.

Comment 41

[zeyu [:car]]

Attachment: fix style & add progress bar

Comment 42

[zeyu [:car]]

Attachment: screen shot of progress bar

Comment 43

[Mike Conley (:mconley) (:⚙️)]

Hey car! That's *great* progress! I'll hopefully have a review for you up tonight.

Comment 44

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 717481 [details] [diff] [review]
fix style & add progress bar

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

This is the right way forward.

I don't think we're going to want the native appearance of the progressmeters though - it's going to be too hard to read text on top of them. Instead, we should choose a solid colour with no gradient (maybe something like Highlight, and then set the label to be HighlightText - see https://developer.mozilla.org/en-US/docs/CSS/color_value)

Anyhow, this is the right idea! Keep going!  :D

::: mail/base/content/mailWidgets.xml
@@ +466,5 @@
>                       xbl:inherits="src=image"/>
>          </xul:hbox>
>          <xul:hbox class="attachmentcell-text" flex="1">
>            <xul:hbox class="attachmentcell-nameselection" flex="1">
> +            <xul:stack class="attachment-progress">

What does this class do?

@@ +472,3 @@
>                <xul:label class="attachmentcell-name" xbl:inherits="value=name"
>                           flex="1" crop="center"/>
> +              <xul:hbox align="center">

What is this hbox for?

Comment 45

[zeyu [:car]]

Attachment: override progress meter

Comment 46

[zeyu [:car]]

Attachment: screenshot

Comment 47

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 725429 [details] [diff] [review]
override progress meter

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

This is looking pretty good car - making good progress. Next, we need to hook up your listener to the UI. See below.

::: mail/base/content/mailWidgets.xml
@@ +467,5 @@
>          </xul:hbox>
>          <xul:hbox class="attachmentcell-text" flex="1">
>            <xul:hbox class="attachmentcell-nameselection" flex="1">
> +            <xul:stack>
> +              <xul:progressmeter mode="determined" value="55"/>

I believe if you set xbl:inherits="value=progress" on this node, then setting a progress attribute on this attachmentitem will forward that progress value to the progressmeter as "value".

Once you've done that, you need to hook your progress listener to the attachmentitem so that it sets the progress attribute.

Comment 48

[zeyu [:car]]

Attachment: a partially working patch

now progressmeter will get updated as file is being linking. Both directly linking and converting to link work.

Two problems: 
1. If had multiple files to be linking, the first item's progressmeter always get updated while others' are just blank. The way I pick the progressmeter to get updated is not correct.

2. Sometimes it cannot reach 100%, in terminal the dumped percent will end up around about 97%.

Comment 49

[zeyu [:car]]

Attachment: only first progress meter get updated & cannot reach 100%

Comment 50

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 728756 [details] [diff] [review]
a partially working patch

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

::: mail/base/content/mailWidgets.xml
@@ +467,5 @@
>          </xul:hbox>
>          <xul:hbox class="attachmentcell-text" flex="1">
>            <xul:hbox class="attachmentcell-nameselection" flex="1">
> +            <xul:stack>
> +              <xul:progressmeter id="attachmentitem-progress" mode="determined" xbl:inherits="value=progress"/>

The problem here is that id="attachmentitem-progress" is being cloned for each attachmentitem, and id's are supposed to be unique.

I don't think you even *need* an ID. Setting the progress attribute directly on the attachmentitem should work, since you used xbl:inherits="value=progress".

::: mail/components/compose/content/MsgComposeCommands.js
@@ +1255,5 @@
>      let i = 0;
>      let items = AddAttachments(attachments, function(aItem) {
>        let listener = new uploadListener(attachments[i], files[i], aProvider);
>        try {
> +        let progressmeter = document.getElementById("attachmentitem-progress");

You shouldn't need to query attachmentitem-progress directly here. aItem is the attachmentitem that's been added to the attachment list. Just do:

aItem.setAttribute("progress", percentComplete);

Comment 51

[zeyu [:car]]

Attachment: use aItem.setAttribute to set progress

Comment 52

[Mike Conley (:mconley) (:⚙️)]

(In reply to zeyu [:car] from comment #48)
> Created attachment 728756 [details] [diff] [review]
> a partially working patch
> 
> now progressmeter will get updated as file is being linking. Both directly
> linking and converting to link work.
> 
> Two problems: 
> 1. If had multiple files to be linking, the first item's progressmeter
> always get updated while others' are just blank. The way I pick the
> progressmeter to get updated is not correct.
> 

Your change to use aItem.setAttribut should fix this.

> 2. Sometimes it cannot reach 100%, in terminal the dumped percent will end
> up around about 97%.

This is likely because the request is finishing, and not sending out a final progress event. When the upload finishes, we should stop showing the progress meter.

Comment 53

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 731868 [details] [diff] [review]
use aItem.setAttribute to set progress

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

::: mail/base/content/mailWidgets.xml
@@ +467,5 @@
>          </xul:hbox>
>          <xul:hbox class="attachmentcell-text" flex="1">
>            <xul:hbox class="attachmentcell-nameselection" flex="1">
> +            <xul:stack>
> +              <xul:progressmeter mode="determined" xbl:inherits="value=progress"/>

add a class here, attachmentcell-progress

::: mail/components/compose/content/MsgComposeCommands.js
@@ +1255,5 @@
>      let i = 0;
>      let items = AddAttachments(attachments, function(aItem) {
>        let listener = new uploadListener(attachments[i], files[i], aProvider);
>        try {
> +	      let progressListener = function progressListener(aEvent) {

So I think it's time we start cleaning up the alignment here.

We do two-space indentation, and you should be doing that here as well.

You don't need to name the function if its being assigned to a variable, so

let progressListener = function(aEvent) { ... } is fine

@@ +1257,5 @@
>        let listener = new uploadListener(attachments[i], files[i], aProvider);
>        try {
> +	      let progressListener = function progressListener(aEvent) {
> +	        if (aEvent.lengthComputable) {
> +          let percentComplete = (aEvent.loaded / aEvent.total)*100;

Put spaces on either side of the *

@@ +1260,5 @@
> +	        if (aEvent.lengthComputable) {
> +          let percentComplete = (aEvent.loaded / aEvent.total)*100;
> +          aItem.setAttribute("progress", percentComplete);
> +	        } else {
> +	          //some message

Instead of this comment, maybe report an error to the console via Components.utils.reportError?

@@ +1314,5 @@
>      try {
>        let listener = new uploadListener(item.attachment, file,
>                                          aProvider);
> +      let progressmeter = document.getElementById("attachmentitem-progress");
> +      let progressListener = function progressListener(aEvent) {

This is an annoying duplication.

Perhaps we should have a function, getProgressListener, that takes an attachment item, and returns a progress listener function which correctly sets attributes on that item? That could be used both here and earlier in this file.

::: mail/themes/gnomestripe/mail/compose/messengercompose.css
@@ +11,5 @@
>  @import url("chrome://global/skin/toolbar.css");
>  
>  @namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
>  
> +.progress-bar {

We only want to show progress bars for attachmentitems that are uploading, so let's give those attachmentitem's an attribute when the upload is in progress (you can do that in uploadListener's onStartRequest function), and removes that attribute when the upload is completed (the uploadListener's onStopRequest function).

That attribute - suppose it's "uploading" - should be used to display the progressmeter.

So,

attachmentitem:not([uploading]) .attachmentcell-progress {
  display: none;
}

I think we need to be more specific, and not change every single progress bar out there - so for these rules, how about:

.attachmentcell-progress > .progress-bar {
  -moz-appearance: none;
  background-color: rgb(135, 227, 255);
}

.attachmentcell-progress > .progress-remainder {
  -moz-appearance: none;
  background-color: rgb(235,235,235);
}

We'll, of course, need these styles for Windows and OSX.

Comment 54

[zeyu [:car]]

Hi Mike,

When I was re-write my code according to your review, I find two problems that I don't quite understand:

1. You suggested that we use getProgressListener(aItem) to get the listener instead of repeating the code. Therefore I added:

function getProgressListener(aItem)
{
  this.item = aItem;
  this.listener = function(aEvent){
    if (aEvent.lengthComputable) {
      let percentComplete = (aEvent.loaded / aEvent.total) * 100;
      this.item.setAttribute("progress", percentComplete);
    } else {
        Components.utils.reportError("Cannot compute the event's length.");
    }
  };
  return this.listener;
}
to MsgComposeCommand.js and let progressListener = getProgressListener(aItem);

And it seems that the program stuck there. Even if I set

function getProgressListener(aItem){
  return "getProgressListener";
}

and then dump(getProgressListener(aItem); it still stuck at that line of code.


2. 

attachmentitem:not([uploading]) .attachmentcell-progress {   
  display: none;
}

this works when the attachmentitem is not being uploading, however if uploading starts the progress bar still doesn't show up.

Comment 55

[Mike Conley (:mconley) (:⚙️)]

Hey car - I just checked up on this bug and noticed you asked me a question! Next time you have a question for me, make sure to set the needinfo? flag on my email address so that I don't miss it.

Comment 56

[zeyu [:car]]

(In reply to Mike Conley (:mconley) from comment #55)
> Hey car - I just checked up on this bug and noticed you asked me a question!
> Next time you have a question for me, make sure to set the needinfo? flag on
> my email address so that I don't miss it.

Sure, I am just not very familiar with bugzilla. Thanks!

Comment 57

[Mike Conley (:mconley) (:⚙️)]

(In reply to zeyu [:car] from comment #54)
> Hi Mike,
> 
> When I was re-write my code according to your review, I find two problems
> that I don't quite understand:
> 
> 1. You suggested that we use getProgressListener(aItem) to get the listener
> instead of repeating the code. Therefore I added:
> 
> function getProgressListener(aItem)
> {
>   this.item = aItem;
>   this.listener = function(aEvent){
>     if (aEvent.lengthComputable) {
>       let percentComplete = (aEvent.loaded / aEvent.total) * 100;
>       this.item.setAttribute("progress", percentComplete);
>     } else {
>         Components.utils.reportError("Cannot compute the event's length.");
>     }
>   };
>   return this.listener;
> }

What I've asked for is a function that returns a listener. There is no need to hold a reference to aItem - and there are also some scoping issues here (inside the listener function, "this" is different from the outer "this").

It can be much simpler - something like:

function getProgressListener(aItem) {
  return function(aEvent) {
    if (aEvent.lengthComputable) {
      let percentComplete = (aEvent.loaded / aEvent.total) * 100;
      aItem.setAttribute("progress", percentComplete);
    } else {
      Components.utils.reportError("Some error message.");
    }
  }
}

> 2. 
> 
> attachmentitem:not([uploading]) .attachmentcell-progress {   
>   display: none;
> }
> 
> this works when the attachmentitem is not being uploading, however if
> uploading starts the progress bar still doesn't show up.

Using DOMi[1], try manually setting the uploading attribute. Does that help any?

[1]: https://developer.mozilla.org/en-US/docs/DOM_Inspector/Introduction_to_DOM_Inspector

Comment 58

[zeyu [:car]]

Attachment: set "uploading" attribute "true" while uploading, "false" otherwise.

Comment 59

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 767081 [details] [diff] [review]
set "uploading" attribute "true" while uploading, "false" otherwise.

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

::: mail/base/modules/http.jsm
@@ +6,5 @@
>  
>  const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
>  
>  function doXHRequest(aUrl, aHeaders, aPOSTData, aOnLoad, aOnError, aThis,
> +                     aMethod, aListener, aLogger) {

Hrm, so there's a problem here now.

In bug 884319, mail/base/modules/http.jsm was put into toolkit/modules as Http.jsm. The function interface has also flipped around to use a single object parameter instead of a ton of individual parameters.

You're going to need to modify your change here for toolkit/modules/Http.jsm.

::: mail/base/modules/oauth.jsm
@@ +86,5 @@
>     * @param aThis passed as first param to success and error callbacks
>     * @param aOAuthParams additional params to pass to request
>     */
>    signAndSend: function(aUrl, aHeaders, aMethod, aPOSTData, aOnLoad, aOnError, aThis,
> +                        aOAuthParams, aListener) {

I don't see any callers of signAndSend passing a listener. Why not?

::: mail/components/compose/content/MsgComposeCommands.js
@@ +1258,5 @@
>      let i = 0;
>      let items = AddAttachments(attachments, function(aItem) {
>        let listener = new uploadListener(attachments[i], files[i], aProvider);
>        try {
> +	      let progressListener = getProgressListener(aItem);

Indentation here is off.

Comment 60

[dev.0]

Hey everybody,
are there still plans to resolve this issue?
A progress bar while uploading an attachment via file link really is a substantial feature (not only in my opinion). Not having this feature feels kind of ancient, since file link has been implemented about 2.5 years ago.

Regards

Comment 61

[ian.cheong]

Agree that the lack of a progress bar is a serious shortcoming. From a usability perspective, perhaps the progress bar is not the most important. 

Life would be simpler for most users if we could:
a. initiate Filelink
b. queue for sending - so upload can proceed in background and link added without me needing to see it
c. get feedback on if there is a problem
d. see progress on pending Filelink uploads

Currently without being able to send and without being able to see progress percentage or upload speed, I am paralysed waiting for the file to upload and if i get busy doing something else, could end up forgetting to send it at all.

Comment 62

[Magnus Melin [:mkmelin]]

Nowadays, you'd probably put an html:progress somewhere around here: https://searchfox.org/comm-central/rev/364e5ba7492c8a13e104662a7e43769819e339f7/mail/base/content/mailWidgets.js#1526

Comment 63

[John Bieling (:TbSync)]

With multiple attachments being able to upload at the same time, I do not think an additional progress bar in the UI is such a good idea. I would instead use the icon of the uploading attachment itself. Currently, it is just a spinner during uploads. Without having looked at the code:

@Magnus: Could we use upload progress events to change the icon to toggle between the standard spinner (to show activity on long uploads) and pie-chart-style progress indicators for 10,20,30,40.50,60,70,80,90 % progress ?

@Alessandro: What do you think of this? I got inspired by the overall download progress indicator of Firefox.

Comment 64

[Magnus Melin [:mkmelin]]

I think the idea earlier in this bug to make the the attachment item itself show the progress of itself is good. An overall indicator is not quite as appealing since sizes are different and one might e.g. want to know "is this going to ever finish" which is very different if you're sending 500MB or 2MB.

Comment 65

[John Bieling (:TbSync)]

Nono, I meant an individual indicator, using the file icon of each attachment being uploaded, showing the progress of each individual attachment.

Sorry for confusing this by mentioning the overall Firefox indicator. I was inspired by the LOOK of that firefox progress indicator, but of course I want that for each attachment.

Having a real additional progress bar for each uploading attachment could clutter the UI, that is why I was proposing to repurpose the spinner icon, which is currently indicating an attachment is uploading, but could show actual progress information as well.

Comment 66

[Magnus Melin [:mkmelin]]

Ah, ok. It's hard to say what would look best without trying it out.

Comment 67

[Magnus Melin [:mkmelin]]

But especially for large uploads it may not be clear that there's progress if you have something like the pie chart indicator.

Comment 68

[John Bieling (:TbSync)]

That is why I said it should toggle between pie chart status and spinner mode. I saw that once and it looked very nice, but cannot remember where. I will do a Mockup if Alessandro is not throwing this idea out of the door directly.

Comment 69

[Alessandro Castellani [:aleca]]

I like this idea, and a visual indication of the uploading progress is definitely a needed addition.

By briefly thinking about it, there are potentially 2 approaches we could try out:

• Using the piechart progress icon like firefox which temporarily replaces the attachment icon for each single attachment. So you see which one is still uploading. A tooltip title should show the progress info.
• Using the animated loading.svg temporarily replacing the attachment icon for each single attachment, plus a tiny progress bar at the bottom of the attachment itself.

Looking forward to seeing your mock-ups, John.

Comment 70

[John Bieling (:TbSync)]

Attachment: WIP: Bug 736169 - Add support to specify upload progress for cloudFiles. r=darktrojan

Depends on D140328

Comment 71

[Teal Dulcet [:tdulcet] (TB Council)]

This is the most requested feature for my FileLink add-on and it looks like we have a working patch from almost a year ago which John said he was planning for TB 102, so is there anything now blocking this from landing in time for 115? I do see that "WIP" prefix on the patch, but it seems like he already made the changes requested by Geoff. I would be happy to help test a try build if needed.