1471833 - Implement "Open containing folder" iconic button for "Saved Files" items (about:downloads)

Status: RESOLVED FIXED

(Thunderbird :: Toolbars and Tabs, enhancement)

Description

[torsten.zachert]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:62.0) Gecko/20100101 Firefox/62.0
Build ID: 20180625141512

Steps to reproduce:

n.d.


Actual results:

n.d.


Expected results:

It would be very helpful to have a list with the last saved attachments to open directly or in the explorer, like Firefox has.

Comment 1

[Jorg K (CEST = GMT+2)]

Tools > Saved Files (Ctrl+J), just like Firefox has?

Comment 2

[torsten.zachert]

(In reply to Jorg K (GMT+2) from comment #1)
> Tools > Saved Files (Ctrl+J), just like Firefox has?


Oh, thanks. I didn't find that.

But there is no icon for the toolbar like firefox has, isn't it?

Comment 3

[Jorg K (CEST = GMT+2)]

Right, there doesn't appear to be an icon. We couldn't add one but file download/saving is not one of the primary functions of an e-mail client. The function is available on the menu and via keyboard shortcut. I think that's enough. Richard?

Comment 4

[torsten.zachert]

(In reply to Jorg K (GMT+2) from comment #3)
> Right, there doesn't appear to be an icon. We couldn't add one but file
> download/saving is not one of the primary functions of an e-mail client. The
> function is available on the menu and via keyboard shortcut. I think that's
> enough. Richard?

In Germany and maybe in other countries too it is neccesary to save all business attachments (electronic invoices etc.) separately. Therefore it would be very helpful to have an as easy as possible way to access the currently stored documents.
Using the menu is not easy enough. Shortcut is ok. But I think the best user friendly way, would be a button positioned nearly to the "Save Attachment Button" in the email status bar.

Maybe you can think about that in future.

Comment 5

[Thomas D. (:thomas8)]

(In reply to torsten.zachert from comment #0)
> It would be very helpful to have a list with the last saved attachments to
> open directly or in the explorer, like Firefox has.

Thank you Torsten for this RFE which is spot-on. It has already been filed by me as bug 1351219 (which was somewhat implicitly included in or at least related to 5-year-old bug 907732). Imo it's a significant shortcoming of TB UX for a basic every day task.

While this is essentially a duplicate of bug 1351219, we might want to use this bug to improve things with a minimal solution before going all the way of mimicking the goodness of the FF download indicator button:

For starters, we could just have a simple "Saved Files" button which only links to the "Saved Files" tab.

(In reply to Jorg K (GMT+2) from comment #3)
> Right, there doesn't appear to be an icon. We couldn't add one but file
> download/saving is not one of the primary functions of an e-mail client. The
> function is available on the menu and via keyboard shortcut. I think that's
> enough. Richard?

Huh!? I think many users would disagree with that. Attachments are certainly one of the primary/essential functions of an email client, and accessing saved attachments for further management is an important part of attachment handling.

TB's current workflow for this important function (accessing just saved attachments for further management) leaves a lot to desire, in fact it's broken/undiscoverable to the extent of being non-existent, as the user of story of this bug illustrates: Reporter was not able to find Tools > Saved Files (Ctrl+J), in spite of having an explicit interest in that function. After saving attachments, there's no clue whatsoever in the UI where they have gone and how to access them from within Thunderbird. So most users, like reporter, will be mislead to assume that they have to search for the files again using their OS file manager.

Comment 6

[Thomas D. (:thomas8)]

We might want to use duplicate bug 1471833 to implement at least a minimal version of this until we succeed to get to the full goodness of the FF download experience in TB: Just having a dedicated toolbar button for accessing "Saved Files" directly from main 3-pane would go a long way.

Comment 8

[Magnus Melin [:mkmelin]]

Should be changing code around here: https://searchfox.org/comm-central/rev/c1fa40fa8e939b73cebcd10cd5de3351f2e35841/mail/components/downloads/content/aboutDownloads.js#230

For reference, Firefox code (different) is around here: https://searchfox.org/mozilla-central/rev/444ee13e14fe30451651c0f62b3979c76766ada4/browser/components/downloads/DownloadsSubview.jsm#455

Comment 9

[Somu]

Magnus, Wayne I think I can tackle this too. Please assign this to me and I will make a patch ASAP.

Comment 10

[Magnus Melin [:mkmelin]]

Great, over to Somu then!

Comment 11

[Somu]

Magnus the above searchfox links that you posted is the code which corresponds to the Downloads View when (Cntrl + J) is pressed in TB. Could you please tell me which part of the codebase I should be playing with, so that the main display interface gets changed (Because I need to add a downloads button and give it the same functionality as Ctrl+J). Thankyou.

Comment 12

[Magnus Melin [:mkmelin]]

There has been some other comments, but I think we should focus on the initial goal with this bug: to be able to open the containing folder of the attachment, from the downloads view.

If you compare to firefox, right of each file there is a folder icon which you can click to "Open Containing Folder". That is the part we want to tackle in this bug.

Comment 13

[Somu]

Thankyou Magnus for the clarification. I had one more doubt regarding the UI. In aboutDownload.js there are certain XUL Elements being added. For example, the fileName, size and startDate are being added to the vbox section. But my doubt is, where is the fileName and size and startDate being filled? Which component of the system is responsible for filling in the details? Thankyou.

Comment 14

[Magnus Melin [:mkmelin]]

Those are just the elements to show the data. The actual data looks to be set up in the DownloadItem "constructor" https://searchfox.org/comm-central/rev/c1fa40fa8e939b73cebcd10cd5de3351f2e35841/mail/components/downloads/content/aboutDownloads.js#171

Comment 15

[Somu]

Where can I find the Icon Location or Path to the folder icon which is present in firefox. I need a path something like moz-icon://......

Comment 16

[Magnus Melin [:mkmelin]]

Icons are hooked up by css. Compare to firefox: https://searchfox.org/mozilla-central/rev/9eb2f739c165b4e294929f7b99fbb4d90f8a396b/browser/themes/shared/customizableui/panelUI.inc.css#1768

You need to get the corresponding icon files copied to mail/ instead of browser/ and registered in relevant jar.mn file. Searchfox is your friend here. Look for how it's done for browser/ and do a similar thing for mail.

For firefox, regarding the actual command I think it ends up executing downloadsCmd_show and ends up here:
https://searchfox.org/mozilla-central/rev/9eb2f739c165b4e294929f7b99fbb4d90f8a396b/browser/components/downloads/DownloadsViewUI.jsm#701

Comment 17

[Somu]

Magnus I have faced another issue.

console.log(this.download.target.path);
let file = new FileUtils.File(this.download.target.path);
DownloadsCommon.showDownloadedFile(file);

The value of this.download.target.path is /home/somu/Videos/temp.jpg. But the last line (showDownloadedFile) is giving an error saying that NS_ERROR_FILE_NOT_FOUND. But the file is actually present in the above stated path. Am I missing out something here? Thankyou.

Comment 18

[Magnus Melin [:mkmelin]]

Please post a patch with what you have. Hard to say without any code

Comment 19

[Somu]

Attachment: downloads_show_in_folder.patch

Comment 20

[Somu]

Magnus I have attached the Patch. Please note that I am working on only the clickable functionality as of now. I haven't taken care of the right icon usage and its position either. I will do them once this clickable functionality is working properly. Thankyou.

Comment 21

[Magnus Melin [:mkmelin]]

Comment on attachment 9069937 [details] [diff] [review]
downloads_show_in_folder.patch

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

(plaese use the feedback flag for wip patches and such).

I think the file not found is about the DownloadsCommon.jsm not being found!

::: mail/components/downloads/content/aboutDownloads.js
@@ +10,4 @@
>  
>  ChromeUtils.defineModuleGetter(this, "Downloads", "resource://gre/modules/Downloads.jsm");
>  ChromeUtils.defineModuleGetter(this, "DownloadUtils", "resource://gre/modules/DownloadUtils.jsm");
> +ChromeUtils.defineModuleGetter(this, "DownloadsCommon", "resource://gre/modules/DownloadsCommon.jsm");

This is wrong. Thunderbird doesn't have a DownloadsCommon.jsm. Things under browser/ we don't use directly. Would have to add our own copy of that.

Or well, for this tiny functionality, just copy over the actual needed function(s)

@@ +319,4 @@
>      }
>    },
>  
> +  openFolder() {

nit: two space indent please. only spaces, no tabs

Comment 22

[Somu]

Magnus I actualy copied the DownloadsCommon.jsm from browser/ to obj-x86_64-pc-linux-gnu/dist/bin/modules/DownloadsCommon.jsm and the clickable functionality actually worked. So now the only thing that remains is that how to tell the ./mach build to automatically add DownloadsCommon.jsm to the TB Build. Could you please tell me how to do this? If this is done, then the first part of the Issue would be done with. Thankyou.

Comment 23

[Magnus Melin [:mkmelin]]

You would have to add it under mail/ somewhere and make appropriate changes (see what m-c does for DownloadsCommon.jsm). But, anyway, seems better to just copy over the 10-20 lines for now

Comment 24

[Somu]

Attachment: Screenshot from 2019-06-06 21-54-53.png

Comment 25

[Somu]

Magnus great news. I got the DownloadsCommon.jsm file included in the build of Thunderbird and the clickable functionality is working now. There are now 2 more sub issues to solve -

1. Using the right folder Icon
2. Positioning the Folder Icon correctly (In each Download Element)

I plan on taking up subtask 2 first. But I have tried a lot of ways to get the folder icon below the sender by changing css and even by using another vbox. But they all were in vain. Could you please help me regarding this? (on how to change the position). I have attached the current view of the DownloadsView. Thankyou.

Comment 26

[Magnus Melin [:mkmelin]]

Probably best to use the Developer Tools and inspect the element, both for Firefox and for Thunderbird. Then compare and see that needs changing.

Comment 27

[Somu]

Attachment: Screenshot from 2019-06-07 10-54-58.png

Magnus, is the positioning looking good? Also I am using the same CSS configuration in all 3 linux, windows and Mac OSX for now. Please let me know if any changes you would suggest.

Comment 28

[Somu]

Attachment: downloads_show_in_folder.patch

Magnus, I have faced a new issue. When we double-click on the Folder Icon, it is launching the file. But this is not the expected behaviour. I have trieds to remove this by stopping the Bubbling Process, by using the stopPropagation command. But it is not working. Am I missing something here? I have attached the latest patch for reference. Thankyou.

Comment 29

[Magnus Melin [:mkmelin]]

stopImmediatePropagation() perhaps?

Comment 30

[Somu]

I tried that too Magnus, but it is still not working? Could you please help me with it.

Comment 31

[Magnus Melin [:mkmelin]]

Comment on attachment 9070510 [details] [diff] [review]
downloads_show_in_folder.patch

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

Sorry this is rather wrong.

Comment 32

[Magnus Melin [:mkmelin]]

Attachment: bug1471833_about_downloads.sharedcss.patch

Move to shared css file. Almost completely the same, and looks like the differences are just stuff that were forgot.

Comment 33

[Magnus Melin [:mkmelin]]

Attachment: bug1471833_about_downloads.patch

Add the icon to click. Copied from Firefox. (Though not the js directly, it's too different.)

Comment 34

[Richard Marti (:Paenglab)]

Comment on attachment 9091902 [details] [diff] [review]
bug1471833_about_downloads.sharedcss.patch

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

The CSS looks good. Only it doesn't work because the shared file is packaged on the wrong place.

::: mail/themes/shared/jar.inc.mn
@@ +6,5 @@
>  # actual theme-specific manifests, so that shared resources need only
>  # be specified once. As a result, the source file paths are relative
>  # to the location of the actual manifest.
>  
> +  skin/classic/messenger/aboutDownloads.css                   (../shared/mail/aboutDownloads.css)

This doesn't work as you want to @import from "chrome://messenger/skin/shared/aboutDownloads.css" and this moves the file to "chrome://messenger/skin/aboutDownloads.css".

Better you do it like line 110 ff.

Comment 35

[Richard Marti (:Paenglab)]

Comment on attachment 9091903 [details] [diff] [review]
bug1471833_about_downloads.patch

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

Please can you move the icons to mail/themes/shared/icons/ ? Then all icons are at the same place.

Comment 36

[Magnus Melin [:mkmelin]]

Attachment: bug1471833_about_downloads.sharedcss.patch

Comment 37

[Magnus Melin [:mkmelin]]

Attachment: bug1471833_about_downloads.patch

Comment 38

[Richard Marti (:Paenglab)]

Comment on attachment 9091989 [details] [diff] [review]
bug1471833_about_downloads.sharedcss.patch

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

Looks good. Thanks.

::: mail/themes/shared/jar.inc.mn
@@ +6,5 @@
>  # actual theme-specific manifests, so that shared resources need only
>  # be specified once. As a result, the source file paths are relative
>  # to the location of the actual manifest.
>  
> +  skin/classic/messenger/shared/aboutDownloads.css            (../shared/mail/aboutDownloads.css)

This line could be moved down to around line 110 to be alphabetically sorted.

Maybe you could then move the line with creationDialog.css to the correct position. This seems to be slipped in without sorting.

Comment 39

[Richard Marti (:Paenglab)]

Comment on attachment 9091990 [details] [diff] [review]
bug1471833_about_downloads.patch

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

Thanks.

::: mail/themes/shared/mail/aboutDownloads.css
@@ +97,5 @@
> +
> +/*** Button icons ***/
> +
> +.downloadIconShow > .button-box > .button-icon {
> +%ifdef XP_MACOSX

Sorry to not mention this with the first review.
Instead of preprocessing, could you move this rule to the platform CSS files? We do no preprocessing on CSS files to apply immediately when we change the files and don't need to recompile.

Comment 40

[Magnus Melin [:mkmelin]]

Attachment: bug1471833_about_downloads.sharedcss.patch

Comment 41

[Magnus Melin [:mkmelin]]

Attachment: bug1471833_about_downloads.patch

Comment 42

[Magnus Melin [:mkmelin]]

Thanks for the reviews!

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b72e1eda0630
use a shared CSS file for aboutDownloads.css. r=Paenglab
https://hg.mozilla.org/comm-central/rev/94486e357774
Provide icon to list / open the last saved, recent attachments from download list. r=Paenglab DONTBUILD

Comment 44

[Jorg K (CEST = GMT+2)]

The original patch had
https://hg.mozilla.org/comm-central/rev/94486e357774#l4.12
+ skin/classic/messenger/icons/panel-icon-magnifier.svg (../shared/mail/icons/panel-icon-magnifier.svg)
conditionally:

#ifndef XP_MACOSX
  skin/classic/messenger/icons/panel-icon-folder.svg                (../shared/mail/icons/panel-icon-folder.svg)
#else
  skin/classic/messenger/icons/panel-icon-magnifier.svg             (../shared/mail/icons/panel-icon-magnifier.svg)
#endif

Comment 45

[Thomas D. (:thomas8)]

At least the commit message of this patch isn't right, and one misleading variable name in code, hence removin checkin-needed.

From my reading of the patch (sorry no time to try, screenshot would be helpful), and this bug's history:
Magnus has changed the objective of this bug in comment 12 (for details, see below). Therefore:

Wrong commit message:

Bug 1471833: Provide icon to list / open the last saved, recent attachments from download list. r=Paenglab

Correct commit message:

Bug 1471833: Saved Files list (about:downloads): Add iconic button to open the containing folder of downloaded attachments. r=Paenglab

For the same reason, I think the following variable should be changed:
let downloadButton = document.createXULElement("button");
Imo, downloadButton wrongly suggests that the purpose of this button is to download something, but in reality, it's about opening the containing folder:

    // Show the downloaded file in folder if the folder icon is clicked.	
    downloadButton.addEventListener("click", aEvent => this.show());

So a more appropriate name for this button would be something like:
openFolderButton

If my reading is correct, the current summary of this bug should also be changed accordingly.

Background:

• Original objective: Reporter's request (per current summary):

Summary: Provide icon to list / open the last saved, recent attachments from download list.
It would be very helpful to have a list with the last saved attachments to open directly or in the explorer, like Firefox has.

So that's about adding a toolbar button in the main window for that missing link when after saving attachments, there's zero hint in the UI how to access them, unless searching menus or knowing Ctrl+J by heart (a problem which I have pointed out long ago). Note that reporter is also asking for an easy way to open saved attachments, meaning reporter was clearly not talking about the existing downloads list which already has that functionality. See Jörg's comment 3 which confirms this original understanding.

• New objective (per Magnus comment 12):
  Only add a folder icon next to each download in the list of downloads aka Saved Files (that's useful, too, but only after knowing how to find the downloads list...)

Comment 46

[Thomas D. (:thomas8)]

Midair-collision, Jörg landed this before I succeded to remove checkin-needed.
Comment 45 still applies, and I recommend to fix the commit message and the variable name, because it's pretty confusing otherwise.

Comment 47

[Jorg K (CEST = GMT+2)]

Hmm, commit messages are permanent. If the TB module owner and a TB peer work together, I can only assume that the stuff is correct.

Comment 48

[Magnus Melin [:mkmelin]]

Thomas, well this bug was slightly confusing. If there's something missing, file a new bug with a more clear focus.

I don't see anything wrong with the commit message.
The variable name is just a variable name. It's named that way because that what firefox also uses for this.

Comment 49

[Thomas D. (:thomas8)]

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

Fwiw:

I won't care much if the commit message gets fixed or not, now that it's already landed, but let's get the record straight.
This bug has implemented an "Open containing folder" icon/button for every saved attachment item in the list of "Saved Files" tab, right?
I am failing to see any of that that in the current commit message?

Thomas, well this bug was slightly confusing. If there's something missing, file a new bug with a more clear focus.

Hmmm. The summary is slightly odd because it's arguably repetitive, but otherwise I am failing to locate the confusion until comment 12 where you just decided to do something related, but pretty different from the original intention and description of this bug. Kindly re-read comment 0 up to comment 11 and all of them are only talking about adding a toolbar button to access the Saved Files list/tab sometimes called "downloads list" from the main 3 pane tab. So apparently for reporter, Jörg, myself, and even newbie assignee Somu there was no confusion about the intention of this bug...

I don't see anything wrong with the commit message.
Bug 1471833: Provide icon to list / open the last saved, recent attachments from download list. r=Paenglab

Imho the current commit message is misleading because this bug did NOT add an icon which allows users to list the last saved attachments; we already had that list, but reporter was failing to find it because our UI does not give any easily discoverable clues about the existence of the "Saved Files" list. This bug added an icon/button to "open containing folder" of any one item in the list of "Saved Files".

The variable name is just a variable name. It's named that way because that what firefox also uses for this.

Fair enough (however: pls note that the FF button has multiple functions, but ours hasn't).

Comment 50

[Thomas D. (:thomas8)]

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

There has been some other comments, but I think we should focus on the initial goal with this bug: to be able to open the containing folder of the attachment, from the downloads view.

TB already had that functionality on the context menu of each "Saved Files" item; but now it's more discoverable and more easily accessible, which is great! Thanks for implementing this.

If you compare to firefox, right of each file there is a folder icon which you can click to "Open Containing Folder". That is the part we want to tackle in this bug.

FTR: That is the part that Magnus decided to tackle in this bug.

Comment 51

[Magnus Melin [:mkmelin]]

Like Jörg said, commit messages cannot be changed.