1345167 - Enhance UI display/feedback for external (deleted, detached to file, and http link) attachments

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, enhancement)

Description

[alta88]

1. Linkify external detached to file and http link attachments to display the location in the statusbar on mouseover/focus.
2. Indicate file or link no longer found with strikethrough and tooltip.
3. Fix typo for deleted icon url.
4. Fix save/saveAll for link attachments.
5. Make sure all menus/popups work and look nice.

Comment 1

[alta88]

Attachment: externalAttchExtras.patch

Comment 2

[alta88]

To see this for link attachments, the sample eml here can be used; the 2nd of 3 links is invalid:
https://bugzilla.mozilla.org/attachment.cgi?id=8841429

For an email, one attachment should be deleted, one should be detached to file, one should be detached to file and the file renamed/removed.

Comment 3

[Richard Marti (:Paenglab)]

Comment on attachment 8844539 [details] [diff] [review]
externalAttchExtras.patch

This looks good. Thank you.

Comment 4

[alta88]

Attachment: externalAttchExtras2.patch

Part 2 Improve channel response handling when testing for attachments

Comment 5

[alta88]

A note here: for a detached attachment (which still is found in the original detached-to location) Save As doesn't really make sense as it just opens the Open dialog. Nor does it seem right to Save As a file already saved to disk externally. So the menuitem is disabled in this case and some tests in test-attachment-menus.js fail. These will be fixed, along with adding an external link test, if this is reviewed.

Comment 6

[alta88]

Attachment: externalAttchTest.patch

1. Fix tests for disabled Save for detached.
2. Tests for feed type enclosure attachments, valid links.
3. Fix attachment list contextmenu for Save in non specific location click.

Comment 8

[Thomas D. (:thomas8)]

This looks like promising improvements of attachments UI, thanks and looking forward to this.

(In reply to alta88 from comment #5)
> A note here: for a detached attachment (which still is found in the original
> detached-to location) Save As doesn't really make sense as it just opens the
> Open dialog. Nor does it seem right to Save As a file already saved to disk
> externally. So the menuitem is disabled in this case

I don't see why saving an attachment to one location should prevent the user from saving it to another location again, especially for the non-detached case. Please let's not disable commands which are still applicable and useful.
It does not make sense to needlessly limit commands for users who deliberately keep their attachments within TB (as opposed to detaching them). There are plenty of scenarios where saving attachments to multiple locations or re-saving attachments makes sense and is required, depending on workflow.

For a detached and still linked-up file, it might be OK to disable "Save As" because the attachment is no longer found in the message, so further actions on the file should probably start from file manager. Save As for detached attachments might also wrongly suggest that user is still working on the original attachments, which is not the case. Instead, it would be cool and useful to add an "Open containing folder" link for detached attachments. It would also be great to allow the user to change the filepath stored in TB (of the detached file), so links from message to file system can be maintained even after renaming or moving the detached file to another location.

> and some tests in
> test-attachment-menus.js fail. These will be fixed, along with adding an
> external link test, if this is reviewed.

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 8844539 [details] [diff] [review]
externalAttchExtras.patch

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

Some feedback:
1) I don't seem to get a tooltip showing?
2) We should probably still leave Save As enabled for detached attachments. It doesn't hurt and it's still convenient if you need to save the file to elsewhere.
3) For deleted attachments a mailbox:// url is shown in the status bar. Doesn't seem desirable.

::: mail/base/content/msgHdrViewOverlay.js
@@ +2871,5 @@
> +function saveLinkAttachmentsToFile(aAttachmentInfoArray) {
> +  let URL, Document, DefaultFileName, ContentDisposition,
> +      ContentType, ShouldBypassCache, FilePickerTitleKey,
> +      ChosenData, Referrer, InitiatingDocument, SkipPrompt,
> +      CacheKey;

why are these not camelCase? (and URL as url)

Comment 10

[alta88]

In retrospect, there are some things to improve here:
1. In order to know state of buttons/tooltip, the remote url has to be tested; this means fetching it. Though it's no different from any other remote content in feeds, the difference is that a user can block 3rd party tracking bugs and such in both summary and web page loads using AdBlock or uBlock if they really want to. That can't be done for this fetch. 
2. The whole nsIBinaryInputStream method should be renovated rather than being stepped around, using xhr or something more wrapped.

Not going to get to it soon..

Comment 11

[Jorg K (CEST = GMT+2)]

Any chance to save this work and get it reviewed by someone else? (I'm checking other people's review queues regularly and noticed that these reviews were cancelled.)

Comment 13

[Wayne Mery (:wsmwk)]

alta88, are you able to revisit the patches?

Comment 14

[alta88]

unlikely to revisit this. for the record, bug 1411748 'fix' is ridiculous.

Comment 15

[alta88]

Attachment: externalAttchExtras.patch

Part 1.

Comment 16

[alta88]

Attachment: externalAttchExtras2.patch

Part 2 (async replace of nsIBinaryInputStream)

Comment 17

[alta88]

Comment on attachment 8848519 [details] [diff] [review]
externalAttchTest.patch

obsolete

Comment 18

[alta88]

These patches address prior comments and take care of comment 10. Also:

1. Make Bug 1411732 obsolete by displaying remote links in statusbar.
2. Make Bug 1411748 obsolete by not making that code path possible anymore.
3. Port some Fx overlink behavior.
4. Use fetch() to get remote and internal urls, async, with UI busy feedback.
5. An Open Containing Folder option has been added to detached file attachments (other suggestions are out of scope).

Tests will be forthcoming and need a bit of rework for the new async, ie converting existing test to use add_task(async ()) in addition to new tests for link urls.

Comment 19

[alta88]

Notes: debug has been left in for easy testing, and AttachmentInfo.ALWAYSFETCHSIZE is set to true to demonstrate getting sizes from imap/news attachment urls via fetch(); it will be set to false on the checkin patch since libmime already passes up the size.

Comment 20

[Richard Marti (:Paenglab)]

Comment on attachment 9048945 [details] [diff] [review]
externalAttchExtras.patch

Thanks, looks good.

Tested on Windows. The "Open Containing Folder" doesn't work. I get following error in console:
 NS_ERROR_FILE_UNRECOGNIZED_PATH: Component returned failure code: 0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH) [nsIFile.initWithPath] msgHdrView.js:1991

In downloads tab it works.

Comment 21

[alta88]

(In reply to Richard Marti (:Paenglab) from comment #20)

Comment on attachment 9048945 [details] [diff] [review]
externalAttchExtras.patch

Thanks, looks good.

Tested on Windows. The "Open Containing Folder" doesn't work. I get
following error in console:
NS_ERROR_FILE_UNRECOGNIZED_PATH: Component returned failure code:
0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH) [nsIFile.initWithPath]
msgHdrView.js:1991

In downloads tab it works.

Thanks, trying to save a uri conversion, which trick works on linux but not win; to be fixed.

Comment 22

[alta88]

Attachment: openFolder.patch

Comment 23

[Richard Marti (:Paenglab)]

Comment on attachment 9049292 [details] [diff] [review]
openFolder.patch

Thanks, this works on Windows.

Comment 24

[alta88]

Attachment: externalAttchTestsFinal.patch

Tests and minor tweaks. aceman, this uses a sleep(), as I didn't see a way to wait for next message selection (a la add_task async in xpc) while an async piece is active, and the tests are autogenerated onto the global scope.

Comment 25

[Magnus Melin [:mkmelin]]

Comment on attachment 9048945 [details] [diff] [review]
externalAttchExtras.patch

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

::: mail/base/content/mailWidgets.xml
@@ +341,5 @@
>            let border = this._childNodes[0].boxObject.width -
>                         this._childNodes[0].clientWidth;
>  
> +          for (let child of this._childNodes) {
> +            child.width = "";

what is this for?

::: mail/base/content/msgHdrView.js
@@ +1773,5 @@
>      if (this.isEmpty) {
> +      let bundleMessenger = document.getElementById("bundle_messenger");
> +      let prompt = bundleMessenger.getString(this.isExternalAttachment ?
> +                                               "externalAttachmentNotFound" :
> +                                               "emptyAttachment");

nit: strange alignment. Either align under this, or wrap the line with 2 space more indention

@@ +1818,5 @@
> +   *
> +   * @returns true if the attachment is an http link, false otherwise.
> +   */
> +  get isLinkAttachment() {
> +    return this.isExternalAttachment && this.url.startsWith("http");

please make the second part

/^https?:/.test(this.url)

@@ +1909,5 @@
> +
> +  /**
> +   * Open a file attachment's containing folder.
> +   *
> +   * @returns {void}

I don't think we want void return documented

@@ +2316,5 @@
> +  let firstAttachment = attachmentList.firstElementChild.attachment;
> +  let isExternalAttachment = firstAttachment.isExternalAttachment;
> +  let displayUrl = isExternalAttachment ? firstAttachment.displayUrl : "";
> +  let tooltiptext = isExternalAttachment || firstAttachment.isDeleted ?
> +                      "" : attachmentName.getAttribute("tooltiptextopen");

please make this new line start under the let t of the previous line (2 space indent for the new line)

Comment 26

[Magnus Melin [:mkmelin]]

Comment on attachment 9048946 [details] [diff] [review]
externalAttchExtras2.patch

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

::: mail/base/content/msgHdrView.js
@@ +1720,5 @@
>  
>    // Remove [?&]part= from remote urls, after getting the partID.
>    // Remote urls, unlike non external mail part urls, may also contain query
>    // strings starting with ?; PART_RE does not handle this.
> +  let match;

maybe delcare it for each if/else clause, so it can stay "local"

@@ +1758,5 @@
> +
> +    if (this.message != gFolderDisplay.selectedMessage) {
> +console.log("AttachmentInfo.save: message changed");
> +console.log("AttachmentInfo.save: was - "+ this.message.subject);
> +console.log("AttachmentInfo.save: is  - "+ gFolderDisplay.selectedMessage.subject);

remove the debug logging

@@ +1880,5 @@
> +   * NOTE: For internal mailbox, imap, news urls the response body must get
> +   * the content length with getReader().read() but we don't need to do this
> +   * here as libmime streams it already in addAttachmentField(). For imap or
> +   * news urls with credentials (username, userPass), we must remove them
> +   * as Request fails such urls with a MSG_URL_HAS_CREDENTIALS error.

please move this note down to where it's relevant

@@ +1900,5 @@
> +    let empty = true;
> +    let size = 0;
> +    let options = { method: "HEAD" };
> +
> +

nit: double blank line

@@ +1909,5 @@
> +      if (uri.userPass)
> +        url = url.replace(uri.userPass + "@", "");
> +    }
> +
> +console.log("AttachmentInfo.isEmpty: url2 - " + url);

remove

@@ +2329,5 @@
>        }
>      }
>  
> +    let expanded = Services.prefs.getBoolPref("mailnews.attachments.display.start_expanded");
> +    if (expanded)

no need for the expanded tmp var

@@ +2491,5 @@
> +    if (isFetching) {
> +      // Set elements busy to show the user this is potentially a long network
> +      // fetch for the link attachment.
> +      attachmentIcon.setAttribute("src", "chrome://global/skin/icons/loading.png");
> +      attachmentItem.setAttribute("busy", true);

I'd rather use classes for UI things like this.
attachmentItem.classList.add("loading");

@@ +2511,5 @@
> +      return;
> +    }
> +
> +    currentAttachments[index].size = attachmentInfo.size;
> +    let tooltiptextexternalnotfound = attachmentName.getAttribute("tooltiptextexternalnotfound");

some camelCasing wouldn't hurt

@@ +2563,5 @@
> +/**
> + * Calculate the total size of all attachments in the message as emitted to
> + * |currentAttachments| and return a pretty string.
> + *
> + * @returns {String} sizeStr

an example would be better than the var name

@@ +2986,5 @@
>  /**
>   * Link attachments are passed as an array of AttachmentInfo objects. This
>   * is meant to download http link content using the browser method.
>   *
>   * @param {AttachmentInfo}[] aAttachmentInfoArray - Array of attachmentInfo.

while we're here: {AttachmentInfo[]}

@@ +3002,5 @@
>      }
>  
> +    let empty = await attachment.isEmpty();
> +console.log("saveLinkAttachmentsToFile: isEmpty - " + empty);
> +    if (empty || attachment.message != gFolderDisplay.selectedMessage) {

could compare the messages before going off checking for empty

Comment 27

[Magnus Melin [:mkmelin]]

Comment on attachment 9052256 [details] [diff] [review]
externalAttchTestsFinal.patch

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

::: mail/test/mozmill/attachment/test-attachment-menus.js
@@ +443,5 @@
> +
> +  // Test funcs are generated in the global scope, and there isn't a way to
> +  // do this async (like within an async add_task in xpcshell) so await can
> +  // force serial execution of each test. Wait here for the fetch() to complete.
> +  controller.sleep(1000);

can't you make the generated function async and await here?
At https://searchfox.org/comm-central/rev/2a6de7ec2104e7efa2a3f29fb0d4162c49ad2b67/mail/test/mozmill/attachment/test-attachment-menus.js#382

  ... = async function() {

  }

Comment 28

[alta88]

(In reply to Magnus Melin [:mkmelin] from comment #25)

Comment on attachment 9048945 [details] [diff] [review]
externalAttchExtras.patch

Review of attachment 9048945 [details] [diff] [review]:

::: mail/base/content/mailWidgets.xml
@@ +341,5 @@

       let border = this._childNodes[0].boxObject.width -
                    this._childNodes[0].clientWidth;

•      for (let child of this._childNodes) {
  

•        child.width = "";
  

what is this for?

The code calculates optimal layout for the list, if an item's size changes after resolving it, there will be false cropping for certain (short) widths unless it's laid out again. So the existing widths need to be reset.

All other comments have been incorporated. The forthcoming rollup patch includes the 3 separate patches, removes debug, changes ALWAYSFETCHSIZE to false for checkin, and moves the minor tweaks from the tests patch (you can see them in the 2 non test files in that attachment, but they're not re-review worthy imo).

Comment 29

[alta88]

Attachment: externalAttchExtrasFinal.patch

rollup patch.

Comment 30

[alta88]

Comment on attachment 9054766 [details] [diff] [review]
externalAttchExtrasFinal.patch

oh, i missed that one of them didn't have r+ yet.

Comment 31

[alta88]

Regarding comment 5 and subsequent comments not wanting to do that, it should be noted that currently if a detached attachment is Save As, the result saved will be a 0 length file (possibly overriding the existing detached file, and thus datalossy), as the part is considered deleted. So I'd reiterate Save As should be disabled for detached, as the existing code wasn't designed to take a detached file and save it elsewhere.

Also, the patch from Bug 1411732 should be reverted. It's actually dead code as there is currently no such thing as a link attachment in a message that is not also a feed message.

Comment 32

[Jorg K (CEST = GMT+2)]

That means, simply remove this hunk from all branches, right?
https://hg.mozilla.org/comm-central/rev/759b892f48b80de23a4d5a80c959f72b9c25f0ad#l1.12

Comment 33

[Magnus Melin [:mkmelin]]

Comment on attachment 9054766 [details] [diff] [review]
externalAttchExtrasFinal.patch

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

I think there's a few things to do here, but r=mkmelin with these fixed/addressed

::: mail/base/content/mailWidgets.xml
@@ +340,5 @@
>            let border = this._childNodes[0].boxObject.width -
>                         this._childNodes[0].clientWidth;
>  
> +          for (let child of this._childNodes) {
> +            child.width = "";

could you add the explanation into the code?

but, something's wrong here. the loop sets the width (but doesn't use it), then use the last width for everyone in the next loop. 

combine those loops?

::: mail/base/content/mailWindow.js
@@ +259,5 @@
>      if (status.length > 0)
>        this.showStatusString(status);
>    },
>  
> +  setOverLink(url, anchorElt) {

please add jsdoc documentation about this

::: mail/base/content/messageWindow.xul
@@ +46,5 @@
>  %quickFilterBarDTD;
>  <!ENTITY % msgViewPickerDTD SYSTEM "chrome://messenger/locale/msgViewPickerOverlay.dtd" >
>  %msgViewPickerDTD;
> +<!ENTITY % aboutDownloadsDTD SYSTEM "chrome://messenger/locale/aboutDownloads.dtd">
> +%aboutDownloadsDTD;

I think it would be better to add a new entitty for this menu instead of reusing one from aboutDownloads

::: mail/base/content/msgHdrView.js
@@ +633,5 @@
>        // libmime returns -1 if it never managed to figure out the size.
> +      // If we want fetch() to check again for internal attachments, set
> +      // |last.size| to null (instead of -1, which means final fail resolution).
> +      if (size == -1)
> +        last.size = null;

Mixing types is really not nice :/

@@ +1717,5 @@
> +  // attachment here. For internal attachments and link attachments with a
> +  // reported size, libmime streams values to addAttachmentField() which
> +  // updates this object. For external file attachments, |size| is updated
> +  // in the isEmpty() function by fetch() when the list is built.
> +  // Deleted attachments are resolved to -1.

Instead of having size null and keeping track of that "special meaning", I think you should pull that into another variable like sizeChecked

@@ +1815,5 @@
> +   *
> +   * @returns true if the attachment is a detached file, false otherwise.
> +   */
> +  get isFileAttachment() {
> +    return this.isExternalAttachment && this.url.startsWith("file:");

came to mind, maybe we should make sure this is case insensitive. Maybe easiest then to do the regexp like below, but add "i"

@@ +2349,5 @@
>      gBuildAttachmentsForCurrentMsg = true;
>    }
>  }
>  
> +function displayAttachmentsForExpandedViewExternal() {

not sure exactly where this goes wrong, but for a detached attachment even if you correct the path it still displays as not found (anymore).

@@ +2544,5 @@
> +/**
> + * Calculate the total size of all attachments in the message as emitted to
> + * |currentAttachments| and return a pretty string.
> + *
> + * @returns {String} sizeStr (e.g. 123 KB or 3.1MB)

@returns {String} a description of the attachment size (e.g. 123 KB or 3.1MB)

@@ +2721,5 @@
>    item = popup.insertBefore(item, popup.childNodes[indexOfSeparator]);
>  
> +  if (attachment.isExternalAttachment) {
> +    if (!attachment.hasFile) {
> +      item.setAttribute("notfound", true);

can we please use a class for this too instead

Comment 34

[Magnus Melin [:mkmelin]]

Oh, and please add your email to user in the commit message. The commit message appears to have some junk at the end too

Comment 35

[alta88]

(In reply to Magnus Melin [:mkmelin] from comment #33)

Comment on attachment 9054766 [details] [diff] [review]
externalAttchExtrasFinal.patch

Review of attachment 9054766 [details] [diff] [review]:

::: mail/base/content/msgHdrView.js
@@ +633,5 @@

   // libmime returns -1 if it never managed to figure out the size.

•  // If we want fetch() to check again for internal attachments, set
  

•  // |last.size| to null (instead of -1, which means final fail resolution).
  

•  if (size == -1)
  

•    last.size = null;
  

Mixing types is really not nice :/

So that's what the original code did. What should the unset value be, undefined? How is that better. IMO changing this paradigm now is unnecessary work, so please confirm you want it redone.

Comment 36

[alta88]

(In reply to Jorg K (GMT+2) from comment #32)

That means, simply remove this hunk from all branches, right?
https://hg.mozilla.org/comm-central/rev/759b892f48b80de23a4d5a80c959f72b9c25f0ad#l1.12

Yes, but rather than bitrot this patch, I'll remove that hunk here; I didn't mean strictly hg revert. It doesn't need to be removed from branches unless you want.

Comment 37

[alta88]

(In reply to Magnus Melin [:mkmelin] from comment #27)

Comment on attachment 9052256 [details] [diff] [review]
externalAttchTestsFinal.patch

Review of attachment 9052256 [details] [diff] [review]:

::: mail/test/mozmill/attachment/test-attachment-menus.js
@@ +443,5 @@

• // Test funcs are generated in the global scope, and there isn't a way to
• // do this async (like within an async add_task in xpcshell) so await can
• // force serial execution of each test. Wait here for the fetch() to complete.
• controller.sleep(1000);

can't you make the generated function async and await here?
At
https://searchfox.org/comm-central/rev/
2a6de7ec2104e7efa2a3f29fb0d4162c49ad2b67/mail/test/mozmill/attachment/test-
attachment-menus.js#382

... = async function() {

}

That's the first thing of very many things I tried. I don't think js understands a syntax that adds a variable function member with a prefix to |this| or any other object. And even if it did, the main mozmill function calling test_* pattern names in |this| would also have to be changed to be async. Rather out of scope here. But perhaps someone does know of a way to autogen async funcs for mozmill that isn't hypothetical.

Another way is to just write out all the funcs in the file (2 files) but having a simple sleep() is much less ugly.

Comment 38

[Magnus Melin [:mkmelin]]

(In reply to alta88 from comment #35)

So that's what the original code did. What should the unset value be,
undefined? How is that better. IMO changing this paradigm now is unnecessary
work, so please confirm you want it redone.

Looks like there is only a few occurrences of size null checks.
I see the problem. I would add a boolean to AttachmentInfo, say fileSizeOk. For the fileSizeOk == false case (meaning size == null currently) I'd let size be whatever it is, but not use it for anything until fileSizeOk is true.

Comment 39

[alta88]

(In reply to Magnus Melin [:mkmelin] from comment #33)

Comment on attachment 9054766 [details] [diff] [review]
externalAttchExtrasFinal.patch

Review of attachment 9054766 [details] [diff] [review]:

I think there's a few things to do here, but r=mkmelin with these
fixed/addressed

::: mail/base/content/mailWidgets.xml
@@ +340,5 @@

       let border = this._childNodes[0].boxObject.width -
                    this._childNodes[0].clientWidth;

•      for (let child of this._childNodes) {
  

•        child.width = "";
  

could you add the explanation into the code?

sure.

but, something's wrong here. the loop sets the width (but doesn't use it),
then use the last width for everyone in the next loop.

combine those loops?

no, after width is set on each element, it has to be cleared otherwise the natural width as gotten from scrollWidth won't be.. natural. and each item has to be checked to get widest, and then each has to have the widest width set separately. if it were an html grid there wouldn't be this junk.

::: mail/base/content/mailWindow.js
@@ +259,5 @@

 if (status.length > 0)
   this.showStatusString(status);

},

• setOverLink(url, anchorElt) {

please add jsdoc documentation about this

sure.

::: mail/base/content/messageWindow.xul
@@ +46,5 @@

%quickFilterBarDTD;
<!ENTITY % msgViewPickerDTD SYSTEM "chrome://messenger/locale/msgViewPickerOverlay.dtd" >
%msgViewPickerDTD;
+<!ENTITY % aboutDownloadsDTD SYSTEM "chrome://messenger/locale/aboutDownloads.dtd">
+%aboutDownloadsDTD;

I think it would be better to add a new entitty for this menu instead of
reusing one from aboutDownloads

duplicitous, but fine.

::: mail/base/content/msgHdrView.js
@@ +633,5 @@

   // libmime returns -1 if it never managed to figure out the size.

•  // If we want fetch() to check again for internal attachments, set
  

•  // |last.size| to null (instead of -1, which means final fail resolution).
  

•  if (size == -1)
  

•    last.size = null;
  

Mixing types is really not nice :/

@@ +1717,5 @@

• // attachment here. For internal attachments and link attachments with a
• // reported size, libmime streams values to addAttachmentField() which
• // updates this object. For external file attachments, |size| is updated
• // in the isEmpty() function by fetch() when the list is built.
• // Deleted attachments are resolved to -1.

Instead of having size null and keeping track of that "special meaning", I
think you should pull that into another variable like sizeChecked

fine.

@@ +1815,5 @@

• • @returns true if the attachment is a detached file, false otherwise.
• */
• get isFileAttachment() {
• return this.isExternalAttachment && this.url.startsWith("file:");

came to mind, maybe we should make sure this is case insensitive. Maybe
easiest then to do the regexp like below, but add "i"

the path is gotten purely internally, the nsIFile spec is always lowercase, as per spec for URI schemes, so not at all necessary.

@@ +2349,5 @@

 gBuildAttachmentsForCurrentMsg = true;

}
}

+function displayAttachmentsForExpandedViewExternal() {

not sure exactly where this goes wrong, but for a detached attachment even
if you correct the path it still displays as not found (anymore).

it works for me to rename a file, load the message, get not found, rename it back, reload, and have it found again. so i don't understand your STR or what 'correct the path' means.

@@ +2544,5 @@

+/**

• • Calculate the total size of all attachments in the message as emitted to
• • |currentAttachments| and return a pretty string.
• • @returns {String} sizeStr (e.g. 123 KB or 3.1MB)

@returns {String} a description of the attachment size (e.g. 123 KB or 3.1MB)

@@ +2721,5 @@

item = popup.insertBefore(item, popup.childNodes[indexOfSeparator]);

• if (attachment.isExternalAttachment) {
• if (!attachment.hasFile) {

•  item.setAttribute("notfound", true);
  

can we please use a class for this too instead

fine.

Comment 40

[alta88]

Attachment: externalAttchExtrasFinal.patch

Comment 41

[alta88]

Attachment: externalAttchTestsFinal.patch

unbitrot.

Comment 42

[Magnus Melin [:mkmelin]]

(In reply to alta88 from comment #39)

• get isFileAttachment() {
• return this.isExternalAttachment && this.url.startsWith("file:");

came to mind, maybe we should make sure this is case insensitive. Maybe
easiest then to do the regexp like below, but add "i"

Yeah, looks like it is normalized earlier. Hard to know that this was the case.

For the rename thing: I can't reproduce now. Well, I can, but not under normal circumstances, and I wasn't able to see why that test case doesn't work.

Comment 43

[Magnus Melin [:mkmelin]]

Comment on attachment 9055227 [details] [diff] [review]
externalAttchTestsFinal.patch

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

controller.sleep(1000) is ugly, but r=mkmelin

Comment 44

[Magnus Melin [:mkmelin]]

Sent to try: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a857da17f04679c76a9cb0da738b4f1d8d298843

Comment 45

[alta88]

Attachment: externalAttchExtrasFinal.patch

fix linting.

Comment 46

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d167921e42eb
Enhance UI display/feedback for external (deleted, detached to file, and http link) attachments. r=mkmelin
https://hg.mozilla.org/comm-central/rev/a240e0e27ce5
Test adjustments and new tests. r=mkmelin

Comment 47

[Jorg K (CEST = GMT+2)]

I took the liberty to shorten the commit message and to move the extra goodies below in the header.

Comment 48

[alta88]

Attachment: attachFollowup.patch

followup fix, the change out of null forgot to update a check so links get checked again even if resolved.

kind sheriff, could you land at your convenience please.

Comment 49

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/875a13f010ee
Follow-up: Don't test link attachments more than once, comment/log tweaks. r=me

Comment 50

[Jorg K (CEST = GMT+2)]

Done.

Comment 51

[Jorg K (CEST = GMT+2)]

(In reply to alta88 from comment #36)

(In reply to Jorg K (GMT+2) from comment #32)

That means, simply remove this hunk from all branches, right?
https://hg.mozilla.org/comm-central/rev/759b892f48b80de23a4d5a80c959f72b9c25f0ad#l1.12

Yes, but rather than bitrot this patch, I'll remove that hunk here; I didn't mean
strictly hg revert. It doesn't need to be removed from branches unless you want.

Just for the record: the hunk in question was removed here:
https://hg.mozilla.org/comm-central/rev/d167921e42eba41654952c307cec2f0fcaa022d2#l6.190

I assume that we won't backport the changes here, so I guess it will stay on the other branches.

Comment 52

[[e-mail]]

Comment on attachment 9055492 [details] [diff] [review]
externalAttchExtrasFinal.patch

>+   // NOTE: For internal mailbox, imap, news urls the response body must get
>+   // the content length with getReader().read() but we don't need to do this
>+   // here as libmime streams it already in addAttachmentField(). For imap or
>+   // news urls with credentials (username, userPass), we must remove them
>+   // as Request fails such urls with a MSG_URL_HAS_CREDENTIALS error.
>+    if (url.startsWith("imap://") || url.startsWith("news://")) {
Sorry, I don't understand. Why do you only do this for imap and news urls?

>+      let uri = makeURI(url);
>+      if (uri.username)
>+        url = url.replace(uri.username + "@", "");
>+      if (uri.userPass)
>+        url = url.replace(uri.userPass + "@", "");
Eww, string manipulation is not the way to go here. I know these days you have to mutate the uri to clear its userPass but if that's too hard you could consider using new URL(url) instead.