1087233 - Create about:downloads to migrate to Downloads.jsm (Implement "Saved Files" tab)

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

Attachment: WIP patch

Let's go forward! This patch has very primitive view yet.
The most of codes in the patch were copied from browser/components/downloads/ and the patch in bug 901360 and were simplified/modified to our usage.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: show_about_downloads.diff

This patch shows about:downloads in a new tab instead of open nsIDownloadManagerUI.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Attachment: WIP v2

Implemented context menu.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: about_downloads.patch

This patch is ready to be reviewed now.
https://treeherder.mozilla.org/ui/#/jobs?repo=try-comm-central&revision=e3cd6df909f8

This patch lacks some works:
1. CSS  works bug 1092526
2. No progress bar in the list bug 1092527

But I think the new downloads view can replace old nsIDownloadManagerUI so that Thunderbird can work without old download manager.

I would like to leave both of remaining works to someone who are familiar with UI/UX. Of course I can help the guys in coding areas.

Comment 4

[Richard Marti (:Paenglab)]

This works really good. Thank you for your work on this. I'll take bug 1092526.

A small feedback about your patch:

Closing and reopening TB deletes the download/file save history. The history should be saved, if not the menuitem "Remove From History" is useless.

The file size and start time are on two lines. On FX they are on one line separated with a "-". This makes the richlistitem taller than needed.

For me important for bug 1092526. Are you planning to add a button for "open containing Folder"? This could also be made in a followup bug.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

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

Thank you for your feedback.

> Closing and reopening TB deletes the download/file save history. The history
> should be saved, if not the menuitem "Remove From History" is useless.

I've opened a bug for it. 
Bug 1092529. Magnus's radar is finer than my memory. ;-) 


> The file size and start time are on two lines. On FX they are on one line
> separated with a "-". This makes the richlistitem taller than needed.

Yes, I've changed the structures because I prefer that each item has its own DOM elements.
And also I want to add a 'sender' field, and I think it should be larger than other fields because the sender information is more important than others.

BUT it's ok to change the layouts as you wish.

> For me important for bug 1092526. Are you planning to add a button for "open
> containing Folder"? This could also be made in a followup bug.

I can't decide it yet because I am too novice about UI/UX to consider how useful opening the folder on mail client.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: about_downloads.patch v2

This patch includes the fix for bug 1092529.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: about_downloads.patch v3

Fix a problem that saved file which has been already removed from disk is still shown in the view.
Now able to remove multiple selected items in the view.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Note that the file size in the view is currently wrong because of bug 1087228.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8518108 [details] [diff] [review]
about_downloads.patch v3

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

This patch looks good, but I'm having trouble getting any saved downloads to show up in the tab. When I open the tab after saving a file, it just says "No saved files". No errors in the Error Console or anything like that.

Is there a pref that needs to be flipped?

Minusing until I hear about how I can test this.

::: mail/components/downloads/content/aboutDownloads.js
@@ +16,5 @@
> +XPCOMUtils.defineLazyModuleGetter(this, "DownloadUtils", "resource://gre/modules/DownloadUtils.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "OS", "resource://gre/modules/osfile.jsm");
> +
> +const DownloadsView = {
> +  init: function () {

Note that we have the option to use the ECMAScript6 style for function definitions in object literals now - example:

const Something = {
  init() {
    // ...
  },

  foo(bar) {
    // ...
  },
};

Not saying you have to use it, just wanted to point out that it was available.

@@ +35,5 @@
> +    });
> +  },
> +
> +  insertOrMoveItem: function (aItem) {
> +    var compare = (a, b) => {

let, not var

@@ +52,5 @@
> +    this.listElement.insertBefore(aItem.element, at);
> +  },
> +
> +  onDownloadAdded: function (aDownload) {
> +    function isPurgedFromDisk(download) {

I believe this breaks some strictness with inline function definitions? Let's use:

let isPurgedFromDisk = download => {
};

instead.

@@ +121,5 @@
> +    return true;
> +  },
> +
> +  isCommandEnabled: function (aCommand) {
> +    switch (aCommand) {

Seems strange to have a switch for just a single case. Maybe just use a conditional block here, unless you're planning on expanding the number of commands.

@@ +135,5 @@
> +    return false;
> +  },
> +
> +  doCommand: function (aCommand) {
> +    switch (aCommand) {

Same as above, re single-case switch

::: mail/installer/package-manifest.in
@@ +398,4 @@
>  ; download progress
>  @BINPATH@/components/nsHelperAppDlg.js
>  @BINPATH@/components/nsHelperAppDlg.manifest
>  @BINPATH@/components/nsDownloadManagerUI.js

Will this stuff need to be removed at some point?

::: mail/test/mozmill/downloads/test-about-downloads.js
@@ +154,5 @@
> +                    {"class": "toolbarbutton-menubutton-button"}));
> +  }
> +}
> +
> +function test_save_attachment_files_in_list() {

It would be lovely if each of these tests had a brief docstring describing exactly what's being tested.

::: mailnews/base/src/nsMessenger.cpp
@@ +1722,5 @@
>      mimeService->GetFromTypeAndExtension(m_contentType, EmptyCString(), getter_AddRefs(mimeinfo));
> +
> +    // Set saveToDisk explicitly to avoid launching the saved file.
> +    // See http://hg.mozilla.org/mozilla-central/file/814a6f071472/toolkit/components/jsdownloads/src/DownloadLegacy.js#l164
> +    mimeinfo->SetPreferredAction(nsIHandlerInfo::saveToDisk);

You'll need a mailnews reviewer for this too.

Comment 11

[Magnus Melin [:mkmelin]]

^^^ just a reminder, so this doesn't fall of radar this close to the goal line :)

Comment 12

[Hiroyuki Ikezoe (:hiro)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #10)
> Comment on attachment 8518108 [details] [diff] [review]
> about_downloads.patch v3
> 
> Review of attachment 8518108 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch looks good, but I'm having trouble getting any saved downloads to
> show up in the tab. When I open the tab after saving a file, it just says
> "No saved files". No errors in the Error Console or anything like that.
> 
> Is there a pref that needs to be flipped?

That's because of bug 914517.
Once bug 1092744 is fixed, most of cases will be fixed except downloading via double click (i.e. double click attachment and then save it).

> ::: mail/components/downloads/content/aboutDownloads.js
> @@ +16,5 @@
> > +XPCOMUtils.defineLazyModuleGetter(this, "DownloadUtils", "resource://gre/modules/DownloadUtils.jsm");
> > +XPCOMUtils.defineLazyModuleGetter(this, "OS", "resource://gre/modules/osfile.jsm");
> > +
> > +const DownloadsView = {
> > +  init: function () {
> 
> Note that we have the option to use the ECMAScript6 style for function
> definitions in object literals now - example:
> 
> const Something = {
>   init() {
>     // ...
>   },
> 
>   foo(bar) {
>     // ...
>   },
> };
> 
> Not saying you have to use it, just wanted to point out that it was
> available.

Will do.

> @@ +35,5 @@
> > +    });
> > +  },
> > +
> > +  insertOrMoveItem: function (aItem) {
> > +    var compare = (a, b) => {
> 
> let, not var

Will do.

> @@ +52,5 @@
> > +    this.listElement.insertBefore(aItem.element, at);
> > +  },
> > +
> > +  onDownloadAdded: function (aDownload) {
> > +    function isPurgedFromDisk(download) {
> 
> I believe this breaks some strictness with inline function definitions?
> Let's use:
> 
> let isPurgedFromDisk = download => {
> };
> 
> instead.

Will do.

> @@ +121,5 @@
> > +    return true;
> > +  },
> > +
> > +  isCommandEnabled: function (aCommand) {
> > +    switch (aCommand) {
> 
> Seems strange to have a switch for just a single case. Maybe just use a
> conditional block here, unless you're planning on expanding the number of
> commands.

OK. Will do.

> @@ +135,5 @@
> > +    return false;
> > +  },
> > +
> > +  doCommand: function (aCommand) {
> > +    switch (aCommand) {
> 
> Same as above, re single-case switch

Will do.

> ::: mail/installer/package-manifest.in
> @@ +398,4 @@
> >  ; download progress
> >  @BINPATH@/components/nsHelperAppDlg.js
> >  @BINPATH@/components/nsHelperAppDlg.manifest
> >  @BINPATH@/components/nsDownloadManagerUI.js
> 
> Will this stuff need to be removed at some point?

nsDownloadManagerUI.js can be removed. But nsHelperAppDlg.js is still necessary for double clicking download.

> ::: mail/test/mozmill/downloads/test-about-downloads.js
> @@ +154,5 @@
> > +                    {"class": "toolbarbutton-menubutton-button"}));
> > +  }
> > +}
> > +
> > +function test_save_attachment_files_in_list() {
> 
> It would be lovely if each of these tests had a brief docstring describing
> exactly what's being tested.

Will do.

> ::: mailnews/base/src/nsMessenger.cpp
> @@ +1722,5 @@
> >      mimeService->GetFromTypeAndExtension(m_contentType, EmptyCString(), getter_AddRefs(mimeinfo));
> > +
> > +    // Set saveToDisk explicitly to avoid launching the saved file.
> > +    // See http://hg.mozilla.org/mozilla-central/file/814a6f071472/toolkit/components/jsdownloads/src/DownloadLegacy.js#l164
> > +    mimeinfo->SetPreferredAction(nsIHandlerInfo::saveToDisk);
> 
> You'll need a mailnews reviewer for this too.

Thanks for the suggestion. I will ask another person for this part.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #12)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #10)
> > Comment on attachment 8518108 [details] [diff] [review]
> > about_downloads.patch v3
> > 
> > Review of attachment 8518108 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This patch looks good, but I'm having trouble getting any saved downloads to
> > show up in the tab. When I open the tab after saving a file, it just says
> > "No saved files". No errors in the Error Console or anything like that.
> > 
> > Is there a pref that needs to be flipped?
> 
> That's because of bug 914517.
> Once bug 1092744 is fixed, most of cases will be fixed except downloading
> via double click (i.e. double click attachment and then save it).

I was wrong. With the fix for bug 1092744, downloaded file by double click is also shown in the list of about:downloads.

Comment 15

[Richard Marti (:Paenglab)]

Do we need this in TB 38?

Comment 16

[Richard Marti (:Paenglab)]

Hiro, if this is wanted for TB 38, would it be possible for you to work on this bug to fix it in time?

I could provide a string only patch to not miss the string freeze of February 24.

Comment 17

[Richard Marti (:Paenglab)]

Adding bug 1130488 to depending on this bug because this probably would needs to be reverted for this bug.

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Attachment: about_downloads.patch v4

IIRC, this patch includes whole fixes in comment #12, and also fix for Bug 1087228.

Push a try now:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d07a43332710

Comment 19

[Kent James (:rkent)]

So close, surely we can get this done for TB 38. Mike, are you going to be able to review this?

Comment 20

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8566451 [details] [diff] [review]
about_downloads.patch v4

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

This is, actually, a pretty stunning patch. Really excellent code in here, very readable.

I'll admit that I'm kind of under a time crunch to review this, so I can't say I was nearly as thorough as I'd normally be. What I can say for certain, however, is that this patch does nothing evil, and seems to use the JS Download API's correctly. It also has tests, which are awesome. Thank you, Hiro. :)

Comment 21

[Kent James (:rkent)]

Pushed https://hg.mozilla.org/comm-central/rev/274371a1c41f

Comment 22

[Philip Chee]

> diff --git a/mail/components/DownloadsStartup.js b/mail/components/DownloadsStartup.js
> new file mode 100644
> --- /dev/null
> +++ b/mail/components/DownloadsStartup.js
This file isn't needed any more. See Bug 1114624 and
https://hg.mozilla.org/mozilla-central/rev/afbba6c11b19#l2.2
https://hg.mozilla.org/mozilla-central/rev/afbba6c11b19#l3.2

Comment 23

[Thomas D. (:thomas8)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #5)
> > For me important for bug 1092526. Are you planning to add a button for "open
> > containing Folder"? This could also be made in a followup bug.
> 
> I can't decide it yet because I am too novice about UI/UX to consider how
> useful opening the folder on mail client.

"Open containing folder" is very important functionality, not having which really sucks for further management of downloaded files. The round iconic button used in FF is just right (if we make that keyboard-accessible, too), but as a temporary solution, even contextual menu (as in old download manager) would suffice.

- Does the current patch include "Open containing folder" functionality?
- If not, can we please include just the string for that so that we can land the UI later?

Comment 24

[[e-mail]]

Comment on attachment 8566451 [details] [diff] [review]
about_downloads.patch v4

>+/**
>+ * This component enables the JavaScript API for downloads at startup.  This
>+ * will eventually be removed when nsIDownloadManager will not be available
>+ * anymore (bug 851471).
>+ */
Wasn't the first step of that already done?

>+               .then(null, Cu.reportError);
Why not use .catch ?

Comment 25

[Kent James (:rkent)]

Neil and Philip, unless you are able to prepare a patch addressing your issues as a quick followup landing, please use new bugs to request changes to this.

Comment 26

[Magnus Melin [:mkmelin]]

"Open congaing folder" is available.

Comment 27

[Magnus Melin [:mkmelin]]

containing