Closed Bug 1542991 Opened 5 years ago Closed 5 years ago

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


(Thunderbird :: FileLink, enhancement)

Not set


(Not tracked)

Thunderbird 68.0


(Reporter: darktrojan, Assigned: darktrojan)




(1 file, 6 obsolete files)

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
Depends on: 1531595
Attachment #9057769 - Attachment is obsolete: true
Attachment #9063453 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9063453 [details] [diff] [review]

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


Attachment #9063453 - Attachment is obsolete: true
Attachment #9063703 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9063703 [details] [diff] [review]

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/
@@ +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]

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] ...

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[";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

Attached 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]

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
Add UI to reuse CloudFile uploads from the current session. r=mkmelin

Closed: 5 years 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.