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

RESOLVED FIXED in Thunderbird 68.0

Status

enhancement
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: darktrojan, Assigned: darktrojan)

Tracking

Trunk
Thunderbird 68.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

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.

I hate this defect/enhancement/task thing.

Type: defect → enhancement

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

Assignee: nobody → geoff
Status: NEW → ASSIGNED
Depends on: 1531595
Attachment #9057769 - Attachment is obsolete: true
Attachment #9063453 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9063453 [details] [diff] [review]
1542991-cloudfile-previous-2.diff

The automation doesn't like my test.
Attachment #9063453 - Flags: review?(mkmelin+mozilla)

Better.

Attachment #9063453 - Attachment is obsolete: true
Attachment #9063703 - Flags: review?(mkmelin+mozilla)
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…

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

Now without a few fatal flaws.

Attachment #9063703 - Attachment is obsolete: true
Attachment #9063703 - Flags: review?(mkmelin+mozilla)
Attachment #9064999 - Flags: review?(mkmelin+mozilla)
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?
Attachment #9064999 - Flags: review?(mkmelin+mozilla) → review+

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

Posted image screenshot (obsolete) —

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

Attachment #9065982 - Flags: feedback?(mkmelin+mozilla)
Comment on attachment 9065982 [details]
screenshot

Guess that's one way yes
Attachment #9065982 - Flags: feedback?(mkmelin+mozilla) → feedback+
Attachment #9064999 - Attachment is obsolete: true
Attachment #9065982 - Attachment is obsolete: true
Attachment #9066001 - Flags: review+

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

Attachment #9066001 - Attachment is obsolete: true
Attachment #9066008 - Flags: review+

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

Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 68.0
You need to log in before you can comment on or make changes to this bug.