408565 - Menu items "Open Containing Folder" / "Show in Finder" shouldn't be grayed out for in-progress/paused downloads

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

With the check-in of the patches on bug 403704 and bug 403707 we match the latest spec for in-progress/paused downloads. But the menu item "Open Containing Folder" / "Show in Finder" is grayed out.

We should also enable the item while a download isn't finished yet. As I can see on the latest mockup the item isn't disabled:

http://people.mozilla.com/~madhava/files/downloadmanager/download%20manager%202007-11-14.png

Comment 1

[Shawn Wilsher :sdwilsh]

How are you downloading this?  It's quite possible that the file doesn't yet exist, so it should be grayed out.

Comment 2

[Ed Lee :Mardak]

Well, I figured we could at least show the directory. Is there a way to reveal a file and have the directory shown but not the file selected? Is that the default behavior?

This would allow the user to see the directory the partial file is being downloaded as well as be ready when the file finishes.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #1)
> How are you downloading this?  It's quite possible that the file doesn't yet
> exist, so it should be grayed out.

It opens the folder where the file is download to. We don't open the file with this menu item. So why preventing the user to open the folder? Edward still points out all I wanted to say.

Comment 4

[Ed Lee :Mardak]

Attachment: v1

Make it always usable and have it do the right function. The original code was causing issues on OS X prompting for an application to handle it... so I made it do launch on the parent.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Is it right, that the return value of onDownloadDblClick is used to enable/disable the context menu entry? It's really hard to bind both things together. Edward, can you create a cvs diff?

Comment 6

[Ed Lee :Mardak]

I didn't understand either of those 2 questions.. ??

onDownloadDblClick doesn't return anything and a command is enabled/disabled with gDownloadViewController's isCommandEnabled function.

cvs diff of what? instead of the hg diff patch that I attached?

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Sorry, I missed the "@@ -669,13 +678,12 @@ var gDownloadViewController = {" line and thought your patch changes code within onDownloadDblClick(aEvent). But now it's clear. Yes, I meant cvs instead of hg but I'll wait until it's fixed now.

Comment 8

[Shawn Wilsher :sdwilsh]

Comment on attachment 293484 [details] [diff] [review]
v1

>+      // "Double click" the parent directory to show where the file should be
>+      parent.launch();
parent should be a nsIFile, so you'll need to QI it to nsILocalFile to be able to call launch.

r=sdwilsh with that fixed.

Comment 9

[Ed Lee :Mardak]

It works without QIing.. so I just add in.. ?

let parent = f.parent.QueryInterface(Ci.nsILocalFile)

Comment 10

[Ed Lee :Mardak]

Attachment: v1.1

(In reply to comment #8)
> >+      parent.launch();
> parent should be a nsIFile, so you'll need to QI it to nsILocalFile to be able
> to call launch.
Added in QueryInterface.

Comment 11

[Shawn Wilsher :sdwilsh]

I don't understand why that works, but as far as I know we need the QI!

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 293624 [details] [diff] [review]
v1.1

Just checking with the man, though I think that this is right ...

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 293624 [details] [diff] [review]
v1.1

a=beltzner for 1.9, and I'm pretty sure it's the right thing to do from the UI perspective

Comment 14

[Ed Lee :Mardak]

Checking in toolkit/mozapps/downloads/content/downloads.js;
/cvsroot/mozilla/toolkit/mozapps/downloads/content/downloads.js,v  <--  downloads.js
new revision: 1.119; previous revision: 1.118
done

Comment 15

[Stephen Donner [:stephend] Not actively reading bugmail]

https://litmus.mozilla.org/show_test.cgi?id=5051

in-litmus+

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2007121905 Minefield/3.0b3pre