Closed Bug 1087233 Opened 5 years ago Closed 5 years ago

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

Categories

(Thunderbird :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 38.0

People

(Reporter: hiro, Assigned: hiro)

References

(Blocks 5 open bugs)

Details

Attachments

(1 file, 6 obsolete files)

Attached patch WIP patch (obsolete) — Splinter Review
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.
Attached patch show_about_downloads.diff (obsolete) — Splinter Review
This patch shows about:downloads in a new tab instead of open nsIDownloadManagerUI.
Attached patch WIP v2 (obsolete) — Splinter Review
Implemented context menu.
Attachment #8509347 - Attachment is obsolete: true
Attachment #8509348 - Attachment is obsolete: true
Attached patch about_downloads.patch (obsolete) — Splinter Review
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.
Attachment #8510680 - Attachment is obsolete: true
Attachment #8515447 - Flags: review?(mconley)
Blocks: 1092529
Blocks: 1092527
Blocks: 1092526
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.
(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.
Blocks: 1092744
Blocks: 1092748
Assignee: nobody → hiikezoe
Status: NEW → ASSIGNED
Attached patch about_downloads.patch v2 (obsolete) — Splinter Review
This patch includes the fix for bug 1092529.
Attachment #8515447 - Attachment is obsolete: true
Attachment #8515447 - Flags: review?(mconley)
Attachment #8517763 - Flags: review?(mconley)
No longer blocks: 1092529
Duplicate of this bug: 1092529
Attached patch about_downloads.patch v3 (obsolete) — Splinter Review
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.
Attachment #8517763 - Attachment is obsolete: true
Attachment #8517763 - Flags: review?(mconley)
Attachment #8518108 - Flags: review?(mconley)
Depends on: 1087228
Note that the file size in the view is currently wrong because of bug 1087228.
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.
Attachment #8518108 - Flags: review?(mconley) → review-
^^^ just a reminder, so this doesn't fall of radar this close to the goal line :)
Flags: needinfo?(hiikezoe)
(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.
Flags: needinfo?(hiikezoe)
(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.
Duplicate of this bug: 1087228
Do we need this in TB 38?
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.
Flags: needinfo?(hiikezoe)
Adding bug 1130488 to depending on this bug because this probably would needs to be reverted for this bug.
Blocks: 1130488
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
Attachment #8518108 - Attachment is obsolete: true
Flags: needinfo?(hiikezoe)
Attachment #8566451 - Flags: review?(mconley)
So close, surely we can get this done for TB 38. Mike, are you going to be able to review this?
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. :)
Attachment #8566451 - Flags: review?(mconley) → review+
Pushed https://hg.mozilla.org/comm-central/rev/274371a1c41f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 38.0
> 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
(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?
Flags: needinfo?(hiikezoe)
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 ?
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.
"Open congaing folder" is available.
Flags: needinfo?(hiikezoe)
containing
Duplicate of this bug: 724262
Depends on: 1138478
Summary: Create about:downloads to migrate to Downloads.jsm → Create about:downloads to migrate to Downloads.jsm (Implement "Saved Files" tab)
Blocks: 1152736
Blocks: 1152741
Blocks: 1152743
Blocks: 851471
Blocks: 1206334
You need to log in before you can comment on or make changes to this bug.