1542991 - Once a file has been uploaded, it should be possible to link to it again

Status: RESOLVED FIXED

(Thunderbird :: FileLink, enhancement)

Description

[Geoff Lankow (:darktrojan)]

If you wanted to send the same file to two different people in separate emails, it should be possible to "reuse" an upload that's already happened. This should be fairly simple for files uploaded in the same session, because we remember that information in a map (see ext-cloudFile.js). For files that have been uploaded in previous sessions, we'd need to improve the API and implement stuff in the extensions.

Comment 1

[Geoff Lankow (:darktrojan)]

I hate this defect/enhancement/task thing.

Comment 2

[Geoff Lankow (:darktrojan)]

Attachment: 1542991-cloudfile-previous-1.diff

I haven't written any tests for this yet, or checked the existing ones, but it does work.

Comment 3

[Geoff Lankow (:darktrojan)]

Attachment: 1542991-cloudfile-previous-2.diff

Comment 4

[Geoff Lankow (:darktrojan)]

Comment on attachment 9063453 [details] [diff] [review]
1542991-cloudfile-previous-2.diff

The automation doesn't like my test.

Comment 5

[Geoff Lankow (:darktrojan)]

Attachment: 1542991-cloudfile-previous-3.diff

Better.

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 9063703 [details] [diff] [review]
1542991-cloudfile-previous-3.diff

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

Works, but the menu under menu under menu is slightly clunky. Maybe we should consider having it instead 
Attach > Filelink > [provider] (and then at his same level, the recent uploads)

::: mail/components/compose/content/MsgComposeCommands.js
@@ +1467,5 @@
> +          popup.appendChild(fileItem);
> +        } catch (ex) {
> +          // The path may no longer point to a real file, which is something
> +          // that should be handled gracefully, but isn't.
> +          Cu.reportError(ex);

please put only the "dangerous" code inside the try-catch

@@ +1687,5 @@
> +/**
> + * Attach a file that has already been uploaded to a cloud provider.
> + *
> + * @param filePath the original file path
> + * @param account  the cloud provider to upload the files to

please add types while you're here

@param {string} filePath -
@param {Object} account -

::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
@@ +338,5 @@
>  bigFileHideNotification.text=You won't be notified if you attach more big files to this message.
>  bigFileHideNotification.check=Never notify me of this again.
>  
> +# Displayed after a list of earlier Filelink uploads.
> +cloudFileNewUpload=New upload…

We usually capitalize, so New Upload…

Comment 7

[Geoff Lankow (:darktrojan)]

I'm taking this one back to the drawing board, as there's a number of things that need more thinking about.

Comment 8

[Geoff Lankow (:darktrojan)]

Attachment: 1542991-cloudfile-previous-4.diff

Now without a few fatal flaws.

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 9064999 [details] [diff] [review]
1542991-cloudfile-previous-4.diff

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

This is better, but it's a bit confusing which are providers and which files. Can you change the menu to say that it's going to be Through a named provider. And a separator under the list of providers.

File link -> 

Through [provider1] ...
Through [provider2] ...
-----------------
earlier-upload.mp3
another-oldupload.txt


BTW, the convert to filelink menu is often (always) disabled when starting fresh. (Maybe another bug.)

::: mail/components/compose/content/MsgComposeCommands.js
@@ +1472,5 @@
> +      let file = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsIFile);
> +      file.initWithPath(upload.path);
> +
> +      // TODO: Figure out how to handle files that no longer exist on the filesystem.
> +      if (!file.exists()) {

gray out, strike?

Comment 10

[Magnus Melin [:mkmelin]]

Actually seems I get this clicking around (not sure exactly when):
An error occurred updating the cmd_convertCloud command: [Exception... "[JavaScript Error: "item.cloudFileUpload is undefined" {file: "chrome://messenger/content/messengercompose/MsgComposeCommands.js" line: 1112}]'[JavaScript Error: "item.cloudFileUpload is undefined" {file: "chrome://messenger/content/messengercompose/MsgComposeCommands.js" line: 1112}]' when calling method: [nsIController::isCommandEnabled]" nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)" location: "JS frame :: chrome://global/content/globalOverlay.js :: goUpdateCommand :: line 67" data: yes] globalOverlay.js:71
goUpdateCommand chrome://global/content/globalOverlay.js:71
updateAttachmentItems chrome://messenger/content/messengercompose/MsgComposeCommands.js:1411
onpopupshowing chrome://messenger/content/messengercompose/messengercompose.xul:1

Comment 11

[Geoff Lankow (:darktrojan)]

Attachment: screenshot

How about if I add the ellipsis that these menu items should've always had? I think that makes the distinction reasonably clear.

Comment 12

[Magnus Melin [:mkmelin]]

Comment on attachment 9065982 [details]
screenshot

Guess that's one way yes

Comment 13

[Geoff Lankow (:darktrojan)]

Attachment: 1542991-cloudfile-previous-5.diff

Comment 14

[Geoff Lankow (:darktrojan)]

This should be fine, but here's a Try run anyway:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=3fca7fb40c2ce80892c1cb08a52aab5675881d59

Comment 15

[Geoff Lankow (:darktrojan)]

Attachment: 1542991-cloudfile-previous-6.diff

Gah, I knew I had to fix that test, too…

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1ab5b794ee03
Add UI to reuse CloudFile uploads from the current session. r=mkmelin