178227 - Can not manually edit Download directory path

Status: RESOLVED WONTFIX

(Firefox :: Settings UI, defect, P3)

Description

[Yuting Kuo]

1. Tool->Preferences->Download
2. Can not edit or paste path to the default download directory field.

Comment 1

[Chris Lyon]

*** This bug has been marked as a duplicate of 178218 ***

Comment 2

[David P James]

Re comment 1: this bug is not a duplicate of bug 178218, which has to do with
the inability to save images, not the inability to specify a directory for
downloads.

Reopening.

Comment 3

[Chris Lyon]

Oops, my mistake.

Confirming on 20021103 on WinXP

OS->All
Severity->Minor

Comment 4

[André Dahlqvist]

Please re-test this with a new build. Blake has landed some fixes for the
Download Panel today.

Comment 5

[André Dahlqvist]

Nevermind, I checked it myself and it is still happening. Blake is aware of it.

Comment 6

[Kai Lahmann (is there, where MNG is)]

eigher I missunderstand this bug, or it's now really gone...

Comment 7

[David P James]

The bug still exists, at least in Linux. Try to type in a directory target (that
is, don't use the Browse button).

Comment 8

[Mike Connor [:mconnor]]

Attachment: patch to allow direct editing of the download path

This was apparently by design, but obviously people want the option.

Comment 9

[:deanis74]

Comment on attachment 127102 [details] [diff] [review]
patch to allow direct editing of the download path

You need to do more than this.	If we're going to allow free-text entry in this
field, we need to check that the directory is valid when OK is pressed.

Comment 10

[David P James]

Why? So we can play nanny and protect users from themselves? Besides, I would
hazard a guess that the only people who enter paths manually actually know the
directory exists and are unlikely to need such a checking facility anyway (I
would enter the path manually because I find the gtk-ish filepicker such a PITA
to use, but that's another issue). Not only that, chances are that the entry
would be pasted from a path gleaned from a file manager like Windows Explorer or
Konqueror. At any rate, this setting can be modified in about:config, so I did
and I entered an incorrect directory path. Using Linux, Firebird defaulted to my
user home directory. So even with erroneous user input Firebird uses the default
setting instead. Given that, I see no point in adding a bunch of code to do some
system lookup for practically no purpose at all.

Comment 11

[:deanis74]

Because it's a good idea.  Because it makes things safe.  Because if an average
user accidentally types something wrong in that field without realizing it, they
don't have to spend 2 hours figuring out where their download actually went.

Comment 12

[Mike Connor [:mconnor]]

David, its a good point, I shouldn't try to write anything after 3 AM because I
don't think about all cases. :) Its not a huge bunch of code, and its a
user-friendly feature, not just power user-friendly like AppSuite. 

New patch coming up with the requested check.  Do we want to have "'C:\foo' does
not exist, do you wish to create it?" as the error dialog?  If I'm doing this
anyway, its probably best to include that in the same function, its not like
it'd add that much more code.

Comment 13

[:deanis74]

That's a good question.  I guess it would be helpful to offer to create the
directory, but it's not really necessary for the first step.  If it's easier to
just allow free-text and only check if the directory exists, go for that.

If this is to be displayed to the user when they click OK, will it be clear what
the problem is if they've switch to another prefs category?

Comment 14

[Stefan Pietschmann]

"Do we want to have "'C:\foo' does
not exist, do you wish to create it?" as the error dialog?"

yup, I think that would be the best. it's safe, userfriendly and shouldn't take
too much code if you say so.

Comment 15

[Mike Connor [:mconnor]]

patch is working, localization isn't done.  Also going to include the fix for
bug 204750 since I have to add a file for properties anyway.

Comment 16

[Mike Connor [:mconnor]]

Attachment: patch with check and warning/option to create directory

this also includes the fix for bug 204750 since I was using the same text
string for another window.

Comment 17

[:deanis74]

Comment on attachment 127325 [details] [diff] [review]
patch with check and warning/option to create directory

>+         alert( prefbundle.getString("invalidDirStart") + " " + givenValue.value 
>+                                              + prefbundle.getString("invalidDirEnd") );
>+invalidDirStart=Unable to create directory
>+invalidDirEnd=.  This is not a valid directory name.

I haven't looked at this whole patch yet but this, and the other place it's
done, did catch my eye.  You don't have to split the string into two parts. 
Use %s in the string, and getFormattedString to format it.  See the mediaSize
string in pageInfo.properties and how it's formatted in pageInfo.js.

Comment 18

[Mike Connor [:mconnor]]

Attachment: updated patch without window switching bug

calling the check from onunload for pref-navigator.xul was a Bad Idea(tm), 
from onchange is far more sensible, sorry for the spam guys

Dean, if you get a chance to look at this please do.

Comment 19

[Mike Connor [:mconnor]]

Attachment: fourth time's the charm?

Dean, I knew there had to be a better way of doing that, I meant to go back and
figure it out but it looks like I forgot to, thanks!

Comment 20

[noririty]

Comment on attachment 127329 [details] [diff] [review]
fourth time's the charm?

works fine here@winXP

Comment 21

[Mike Connor [:mconnor]]

Attachment: update to better handle trying to set a blank path

if existing path is cleared, switches pref to always ask, if they try any other
path they end up in a filepicker.

I'm still not perfectly happy with the way this bit of UI works, but this adds
the requested functionality without any ways around the check (that I've found)


I'd like to see this implemented and get some feedback on this before I do a
wider rewrite of this section

Comment 22

[:deanis74]

Comment on attachment 127515 [details] [diff] [review]
update to better handle trying to set a blank path

>+     if ( givenValue.value == "" ) { // no directory, reset back to Always Ask
>+       downloadDir.selectedItem = document.getElementById("alwaysAskRadio");
>+     } else {
>+       var checkValue = {value:false};

Maybe I'm missing something, I'm still in vacation mode.  Should that be
givenValue.value instead of just value?

>+descPopup=The directory %S does not exist.  Do you wish to create it?

Need a better name than descPopup.

>+downloadDirTitle=Select Download Directory
>+invalidDir=Unable to create directory %S.  This is not a valid directory name.
>+blankDir=You did not specify a download directory.
>\ No newline at end of file

Newline, please.

I'm talking with a couple others about this, too.  I like the functionality
that this patch implements, but I want to make sure it's ok with them before we
check it in.

Comment 23

[Mike Connor [:mconnor]]

Attachment: requested changes from Dean's comment

>Maybe I'm missing something, I'm still in vacation mode.  Should that be
givenValue.value instead of just value?

No, this is just a false value that gets passed to ConfirmEx as we don't need a
checkbox.

>>+descPopup=The directory %S does not exist.  Do you wish to create it?

>Need a better name than descPopup.

changed to invalidDirPopup

>+downloadDirTitle=Select Download Directory
>+invalidDir=Unable to create directory %S.  This is not a valid directory
name.
>+blankDir=You did not specify a download directory.
>\ No newline at end of file

Newline, please.

oops, done

Comment 24

[noririty]

taking

Comment 25

[noririty]

checked in.

Comment 26

[:deanis74]

Comment on attachment 127702 [details] [diff] [review]
requested changes from Dean's comment

I know this has been checked in, but...


>+// check download directory is valid
>+function checkDownloadDirectory() {
>+   var dloadDir = Components.classes["@mozilla.org/file/local;1"]
>+                      .createInstance(Components.interfaces.nsILocalFile);

Looking at other code, the "." before "createInstance" should like up with the
"." before "classes".

>+   if (!dloadDir) return false;

This should be on two lines.

>+   if (downloadDir.selectedItem == document.getElementById("alwaysAskRadio")) { return; }

Ditto.

>+     var promptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
>+                           .getService(Components.interfaces.nsIPromptService);

Similar comment about lining up the "." on the second line.

>+     if ( givenValue.value == "" ) { // no directory, reset back to Always Ask

No spaces after "(" and before ")", and the comment should be on the next line.

>+         dloadDir.create(1,0777);

- Need a space after the comma.
- Do we have access to the DIRECTORY_TYPE constant in this script?  If so, we
should use that instead of 1.
- It's been a while since I had my Sun box on my desk, but 0777 will give
read/write/exec to everyone, won't it?	Do we really want to do this?

>+         alert( prefbundle.getFormattedString("invalidDir", [givenValue.value]) );

No spaces after "(" and before closing ")".

Sorry for not noticing these sooner.

Comment 27

[:deanis74]

Comment on attachment 127702 [details] [diff] [review]
requested changes from Dean's comment

Let me respond to a couple of my questions.

- Do we have access to the DIRECTORY_TYPE constant in this script?  If so, we
should use that instead of 1.

Should be able to use "nsIFile.DIRECTORY_TYPE".

- It's been a while since I had my Sun box on my desk, but 0777 will give
read/write/exec to everyone, won't it?	Do we really want to do this?

The xp filepicker creates the directory using 0755.  We should be consistent
with that.

http://lxr.mozilla.org/mozilla/source/toolkit/components/filepicker/content/fil
epicker.js#705


Also, I'm pretty sure that using alert() in chrome is frowned-upon.  I think
that the alert() call should be promptService.alert().

Comment 28

[Mike Connor [:mconnor]]

Attachment: clean up patch, dean's comments, minor tweak

minor tweaks, like the formatting (old habits die hard)

Dean, the alert and the 0777 were things I was thinking about yesterday. 
Thanks for the feedback.

Comment 29

[:deanis74]

Comment on attachment 128219 [details] [diff] [review]
clean up patch,  dean's comments, minor tweak

Looks great.  Just change the dialog title from "Creating Download Directory
Failed" to "Error creating %S".  This can be done by whoever checks this in.  I
can do that tonight if someone doesn't beat me to it.

Comment 30

[:deanis74]

Comment on attachment 128219 [details] [diff] [review]
clean up patch,  dean's comments, minor tweak

checked in

Comment 31

[Simon Paquet [:sipaq]]

verified with 20030723 trunk build on W2k

Comment 32

[Ben Goodger (use ben at mozilla dot org for email)]

I'm about to break this with download changes. Don't worry, I'm going to fix it
again shortly. Reopening and taking this bug so it's on my mind.  

Comment 33

[Ben Goodger (use ben at mozilla dot org for email)]

-> me. 

Comment 34

[Ben Goodger (use ben at mozilla dot org for email)]

-> 0.9

Comment 35

[Ben Goodger (use ben at mozilla dot org for email)]

Would help if I actually shifted it. 

Comment 36

[Karl Guertin]

UI has changed, looks like Ben fixed it but didn't close this bug. 

Comment 37

[Stefan Pietschmann]

(In reply to comment #36)
> UI has changed, looks like Ben fixed it but didn't close this bug. 

not fixed - actually it's not possible at all at the moment. you can only choose
a folder from a tree-like view. there's no way of manually editing paths or
pasting one there. the question is, if this is a WONTFIX?

Comment 38

[Mike Connor [:mconnor]]

its not a WONTFIX, but editable dropdowns aren't working currently, that's what
needs to happen to fix this bug.

Comment 39

[Lisa Barr]

This is the 1st time I've used Bugzilla, so I'm not sure I'm doing this right,
but I have a similar problem.  If this isn't the right place to put this
question (or comment) would you please direct me to the correct place?

Thanks,

OK-

Earlier this week, I downloaded a file to A:  From that point on, even after I
changed back to the desktop for the download location, each time I tried to
download, it would attempt to download to A:

After I cleaned up my download manager file, and deleted the offending entry, it
stopped doing it.

it is as if a switch was thrown that couldn't be reset.

is this possible?

Comment 40

[troy]

(I think this is the right bug to jump in on.)

1)  I want my downloads to go to /home/troy/.downloads .  The 1.5.0.1 (linux) Preferences->Downloads and "Choose Download Folder" dialog give me no way to do this.  I've also tried about:config and editing browser.download.lastDir and browser.download.defaultFolder, but downloads still go to Desktop.

2)  The each-directory-as-a-button representation of the path is annoying, even if you're not trying to get to '.' files.  Just let me edit the string.

Comment 41

[troy]

OK, I found my workaround.  Changing from the default and checking about:config let me see that browser.download.dir and browser.download.lastDir were the parameters to change.

Still, it would be nice if I could do this without about:config.

Comment 42

[troy]

OK.  Just figured out that my complaint (comment #40) is with the Gnome File Selector.  And, CTRL-L brings up a field you can edit.  How a Firefox user is supposed to know that, I don't know.

Comment 43

[Mike Connor [:mconnor]]

I would think that if you're a GNOME user, you've learned how to do things in the filepicker.  There's the whole "running GTK apps on KDE" issue, but the Qt port for Firefox is again stillborn, so tough luck there I guess.

Comment 44

[Phil Ringnalda (:philor)]

*** Bug 342673 has been marked as a duplicate of this bug. ***

Comment 45

[Phil Ringnalda (:philor)]

*** Bug 359951 has been marked as a duplicate of this bug. ***

Comment 46

[Timwi]

So what's up with this bug now? It's been fixed FOUR YEARS AGO, but never checked in? Why is the last patch designated as obsolete?

Comment 47

[Jared Wein [:jaws] (please needinfo? me)]

I still see the bug, so I don't think it can be closed as FIXED, but I do think it can be closed as WONTFIX. The potential for users to foot-gun themselves here is too high to build this capability in for all users.

If users really desire the ability to hand-edit paths they could write an add-on that provides it. We shouldn't add a feature for all users that can be broken as easily as accidentally typing characters in to a field or hitting Paste in the wrong location.