7840 - Ability to specify default directory for downloads

Status: VERIFIED FIXED

(SeaMonkey :: Download & File Handling, enhancement, P3)

Description

[hecker]

(This bug imported from BugSplat, Netscape's internal bugsystem.  It
was known there as bug #314236
http://scopus.netscape.com/bugsplat/show_bug.cgi?id=314236
Imported into Bugzilla on 06/09/99 10:19)

Request for enhancement: Customer requesting the ability to designate a default
directory into which downloaded FTP files will be placed by default.  (I'm not
sure they mean automatically, i.e., without going through the "Save As"
dialogue, or as the default initial directory for the "Save As".)

Customer claims this feature was in older versions of Navigator, but I don't
recall this; however I will note that at least on Windows the "Save As" dialog
uses a default directory seemingly based either on the location of the
Communicator executables or on whatever directories had been selected in "Save
As" dialogs for other applications, neither of which is probably the most
reasonable choice.  (Also, using the Communicator program directory
wouldn't work in an enterprise deployment where the Communicator
executables were on a shared network drive, possibly read-only.)

Therefore having a preference for specifying a default directory for "Save As"
in Communicator I think is definitely worth considering.

Comment 1

[byunn]

Interesting RFE for all save/publish operations.  Doesn't 1-button publish
already do this?

Comment 2

[Marek Z. Jeziorek]

mass-setting TFV for enhancements to 5.0 (we will not deal with them in 4.x
tree, unless the resources to fix it and test it come from the 5.0 team)

Comment 3

[johng]

Just moved bug to Bugzilla since this is a 5.0 feature request.  Reassigning to
DP.

Comment 4

[Judson Valeski]

not me. this is an fe issue. once the fe receives notification that a file is
coming, they throw the UI, not necko.

Comment 5

[Suresh Duddi (gone)]

I stand corrected.

Comment 6

[don]

Assigning to me for M14 ...

Comment 7

[Matthew Tuck [:CodeMachine]]

Bug #7380 suggests this but allows specification on a URL-by-URL scale.

Comment 8

[davidm]

This feature was in Communicator 4.x/4.5 on at least the Mac. At the absolute
minimum we should get the default location out of IC.

Comment 9

[don]

Move to M15 for later evaluation.

Comment 10

[Akkana Peck]

I have always wanted this feature.  On Unix, it defaults to the current
directory, which is never where I want to save downloaded files.  And the fix
would be so easy ...

Comment 11

[don]

Move to M16 for now ...

Comment 12

[Jeremy M. Dolan]

This feature *IS* in 4.7, but its pretty well hidden.
Prefences/Navigator/Applications, under the main box, 'Download Files To'... can
we change the summary to remove 'FTP'? This should apply to HTTP/gopher/whatever
else... I was actually on my way to file this RFE but a query turned this bug
up. Any possibility of retargeting, since it's a quick fix, as akkana said...

Comment 13

[gabriel]

I've used the feature in NS4, and it doesn't seem to work for mail attachments,
which is a pain. It would be nice if we had this pref in Mozilla, and it was the
default for *all* types of download.

Comment 14

[Jason Heard]

On the PC, it currently defaults to the last directory you saved a file to, 
which for me isn't much of a pain, b/c I only have to find the directory once.  
On Linux (all unix platforms?) it always defaults to the current directory 
(which is always mozilla's binary directory b/c it must be run from there), so 
finding the directory to put the files in is a pain (and in the process 
eliminates the filename, but that's another problem).

Comment 15

[sairuh (rarely reading bugmail)]

tever, would this be your realm? or mine?

don, shall i move this over to Preferences?

Comment 16

[sairuh (rarely reading bugmail)]

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

Comment 17

[Viswanath Ramachandran]

Since Don has left, Vishy is taking his bugs in bulk, pending reassignment.
thanks,
	Vishy

Comment 18

[Paul Chen]

nav triage team:

Not a beta1 blocker, marking nsbeta1-

Comment 19

[Keyser Sose]

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

Comment 20

[Peter Lairo]

This is something sorely missing in mozilla. This bug is now over two years old!
It seems to be a "low cost, high benefit" bug.

Summary of fixes needed:
- add to preferences
   > download files to this directory <BROWSE> OR
   > download files to last used directoy (Mozilla only, not OS)

This may also be integrated with an INTERNAL download manager (ala GetRight),
for which i will file a bug report today.

Comment 21

[Peter Lairo]

Please also see bug 64887 for integration of this feature into a built-in
"Download Manager".

Comment 22

[Matthew T (active 1999-2002)]

I suggest this bug be wontfixed and bug 27493 be fixed instead. With bug 27493, 
you would be able to change the default directory without having to put in any 
extra effort (such as changing prefs).

Comment 23

[Akkana Peck]

They're not quite the same; with a smart dialog, like in bug 27493, if I ever
download to somewhere else then it won't remember to go back to my usual default
place the next time I run the app, which this bug would let me do.

Still, the smart dialog would be a big step in the right direction -- it would
be good enough.

Comment 24

[Matthew T (active 1999-2002)]

[Resummarizing since this doesn't apply just to FTP.]

Comment 25

[Matthew T (active 1999-2002)]

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

Comment 26

[Blake Ross]

Taking, cc law.

Comment 27

[Peter Lairo]

Suggest to add Keywords: *nsCatFood* and *mostfreq* since this should really be
fixed ;)

Comment 28

[Peter Lairo]

Please also add the keyword *mostfreq* since once the people (>20) from bug
27493 come over here it's gonna become a bit crowded.

All that bug wants is to havec save/open directory be "smart", which is covered
by my comment here on 2001-01-10.

- Add to preferences:

+- Open from / Save to Directory ------------------------+
|                                                        |
|    <x> Allways download to this directory:             |
|        [ directory displayed here ]         <BROWSE>   |
|                                                        |
|    < > download files to last used directoy            |
|                                                        | 
+--------------------------------------------------------+

Comment 29

[Peter Lairo]

Having the number of dupes be the criteria for a *mostfreq* bug seems silly to
me. It merely reflects the number of people who are too clueless to see if their
bug has already been reported. Wouldn't it be better to base *mostfreq* on the
number of persons in the CC list? I think so :)

Comment 30

[gabriel]

For me it's not a case of _always_ wanting to download to that directory, but
when the 'save as...' dialogue comes up it should default to here. The user
should still have the ability to choose a different directory if they so desire.

Comment 31

[Peter Lairo]

The open/save dialogues of EVERY modern OS allow you to navigate to other
directories/drives from where you are - so don't worry ;)

Comment 32

[Blake Ross]

usability/polish 0.9.4.

Comment 33

[sairuh (rarely reading bugmail)]

hasn't this been more or less implemented?

resolve as needed, thx!

Comment 34

[a.haakmat]

Sorry if this is not the appropriate place, but it occurred to me that this bug
is actually the source of another frustration that I have been having with
Mozilla, which is the fact that it stores downloaded files in a temporary file.

The reasoning is apparently that you need a temporary file to store the data
that is coming down the socket while the user is selecting a download location.
But when you have a user preference that tells you where to store the file, then
you don't need to bug the user, and you don't need to store downloads in a
temporary file. Also this may help towards bug 103258.

Comment 35

[Akkana Peck]

Don't we have a bug somewhere on bring able to download directly to the
destination file, so we don't have to copy the whole thing after download from
one filesystem to the other?

Comment 36

[Peter Trudelle]

->future

Comment 37

[Peter Lairo]

OK, this bug is now over THREE years old. I have the feeling that those in
charge of it may be using Linux and don't really understand the importance of
having a definable place to put downloaded files under Windows. EVERY decent
download manager (e.g., GetRight, Gozilla, etc.) allows the user to define a
default DL directory!

Please also see bug 22796 for what other useful features this neglected bug is
blocking. BLECH :(

This bug should easily be: Pri.=P2, Sev.=Normal, TM=0.9.9, nsBeta1, nsBranch

This is particularly annoying since (on my machine: 2001-12-17, win98) Mozilla
is always offering some totally *obscure directory* as the starting point for
downloaded files EVERY TIME!!! :( : ( :(

I can't begin to tell you how much I want this bug fixed and how infuriating it
is to see that it has been ignored for so long.

Comment 38

[Boris Zbarsky [:bzbarsky]]

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

Comment 39

[[not reading bugmail]]

my win2k box opens My Documents folder on the desktop as the default whenever I
go to save files.. I use that.  

Boris, will download manager take this into account do you know?

Comment 40

[Hixie (not reading bugmail)]

Seems like this would be something to add to the Downloads pref panel.

  (*) Prompt for download location
       (*) Default to last download directory
       ( ) Default to [ c:\downloads\                  ]  (browse)

  ( ) Always download files to  [ c:\downloads\        ]  (browse)

Comment 41

[u27431]

I would like to humbly suggest another possibility which my examination of this
and related bugs doesn't seem to cover (I could be wrong).

1) Keep the current "download to last folder used" behaviour.
2) Add a "Go to Default Download Folder" button to the file picker.

Then I can download multiple files to a specific directory for a while, and can
quickly jump back to my default whenever I want.

Some means for setting the default would be needed.  Could be a preferences
thing or a "Set Default" button in the file picker (probably the former is
better to stop the picker getting ugly).

Comment 42

[[e-mail]]

fdjsouthey, redesign of the filepicker is discussed in bug 125133 - there
a list of most recent save directories *and* a list of favorite/bookmarked
save directories is planned, so I guess what you describe will basically be
covered by that.

However this bug seems to be about differenct issues: 
 - setting a fixed download dir for specific types of downloads (e.g. FTP)
   (thats not a filepicker issue)
 - allowing by a pref to always download any type to a "default" download
   dir, probably without even showing the filepicker. Not a filepicker
   issue either, but the setting of the default dir could be an option
   in the planned "favorite/bookmarked directories" menu of the filepicker.

I like the second version, because it makes it very easy and convenient
to download a bigger number of files to a single directory. 

Another possibility - one I would like - would be to allow to define 
the subsequent download dir in the filepicker, then do something
like Ctrl-click to download to that dir without showing the filepicker.

Comment 43

[Lane Roathe]

I can't believe this bug has not been fixed yet! This is one of the two bugs
that keeps me from using Mozilla. The suggested layout in comment #40 would be
perfect for me. I want to just click on a URL and have the download start and
complete without further intervention.

Comment 44

[Peter Trudelle]

This is not a defect to be fixed, it is an enhancement that will not be
implemented in the current release cycle. 

Comment 45

[Becker-Freyseng, Christoph]

Hello,
acutally I'd like to improve dlmgr. and a default directory as discussed here is
great. Unfortunately I do not have a lot knowledge about XUL and what's the real
problem not enough knowledge how those calls to the Mozilla-API work e.g.: How
to read user-prefs, tell Mozilla where to save downloads etc.
So if you know about these things I'd be happy to start coding.

Thanks

Comment 46

[Samir Gehani]

Nav triage team: nsbeta1-

Comment 47

[Scott Advani]

My default dirctory has somehow gotten set to %TEMP% on my system, so when a
download completes, Mozilla attempts to copy from %TEMP% to the default target,
which is %TEMP%, and the download disappears.  I therefore cannot download
*anything* using Mozilla.

Comment 48

[Richard Trout]

Attachment: Error dialog when in Download Manager

I reproduce Comment #47 on Windows 2000. For downloads that don't allow
right-click 'Save Link as...' and are .zip or .exe files, download won't work
as I have no option to save in another location, and Mozilla can't seem to
access %TEMP%. 'Show File Location' option disappears in Download Manager, ie.
file deleted by Moz before it can be copied.

Attaching dialog that appears on error.

Comment 49

[Paul]

The DL manager is one of the weekest points in this browser and it still doesn't
work quite right. While others are having critical problems (#47 #43) I am
annoyed that my desktop gets filled up with files when I am done browsing. Why
not increase the priority?

PS, We forgot to bake this thing a cake for its fourth birthday!

Comment 50

[Alfonso Martinez]

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

Comment 51

[Lorin Rivers]

I can't believe this has not been addressed! It's the only thing keeping me from
using Mozilla as my preferred browser!

Comment 52

[Paul]

In OS X using a multiuser environment this bug is a deal breaker for Mozilla. I
downloaded Moz and now the default download directory is /Users/me/Desktop/
which has file privileges 700 (only I can read write and execute from my
desktop). So when the user "other" tries to download a file, they get an access
violation error, they don't have write privileges to /Users/me/Desktop/.

While I could change my Desktop to a have privileges of 766 (I can read write
and execute, others can read and write), the files would then be in a very
inconvenant place and I would have no privacy in my desktop (and users would be
going there all the time to look at stuff). The files would also be cluttering
up my desktop and other usesers would start good but then inevetably get mad at
me when I tried to delete them... It would just be an awful situation.

This is not an RFE, this is a necessary condition for mozilla working in a
multiuser environment on OS X.

BTW, any linux users have this problem?

Comment 53

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

-> me.  Maybe I can do something with this ;).

Comment 54

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Patch v0.1

If the pref "browser.download.default_dir" is set, the "Save file" window will
now default to that directory.	I have to figure out what nsHelperAppDlg.js is
used for, because it implements very similar functionality (so I'll probably
have to change it too).  I don't use FireFox, so it might be a while before I
patch it.  Obviously a GUI change would help make it visible to "normal" users
;).

Comment 55

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Patch v0.2

Patch a few more files - I'm not sure about the two different versions of
nsHelperAppDlg.js, but I patched them both.

Comment 56

[Clement Cherlin]

Firefox 0.8 already has a preference page for this, but at the moment only has
"Save all files to folder X" and "Ask me where to save every file", which
defaults to the most recently selected folder. It would be really nice if there
was a "Ask me where to save every file, but always start browsing from folder X"
option.

Comment 57

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Oh, toolkit/ is firefox.  I shouldn't have patched that.

I need to talk to a UI person about figuring out whether setting a default
directory should ALWAYS default to that directory, or if there should be a way
to use the last-used directory sometimes with a different menu option, or by
holding shift or something.

Comment 58

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Comment on attachment 141287 [details] [diff] [review]
Patch v0.2

sorry for the spam... r? on wrong patch

Comment 59

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Patch v0.3

Obsoleting that picture, because it is irrelevant.

This adds preliminary changes to the downloads pref panel so non-power-users
can set it.

Comment 60

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

->nobody, I am obviously not getting anything done here.

Comment 61

[Ian Neal]

UI of upcoming patch relies on fix in bug 250817

Comment 62

[Ian Neal]

Attachment: Updated patch (v0.4) ignoring whitespace changes

This implements UI, and associated backend, as suggested by Hixie in comment 40

Comment 63

[Ian Neal]

Attachment: Patch v0.4 including all whitespace changes

If patch v0.4 is r/sr then this is the patch to be checked in.

Comment 64

[Ian Neal]

Comment on attachment 152890 [details] [diff] [review]
Updated patch (v0.4) ignoring whitespace changes

Cancelling request until I unbitrot patch

Comment 65

[Ian Neal]

Attachment: Unbitrotted patch (v0.4aw) - whitespace changes ignored

Comment 66

[Ian Neal]

Attachment: Unbitrotted patch (v0.4a) - includes whitespace changes

Comment 67

[Ian Neal]

Attachment: Tweaked patch v0.4bw - no whitespace changes

fixes nit replacing style="margin-left:2em" with class="indent"

Comment 68

[Ian Neal]

Attachment: Patch v0.4b - with whitespace changes too

Comment 69

[Ian Neal]

Attachment: Patch v0.4cw correct browser-prefs.js - wpud8 version

For some reason, probably to do with timestamps, the browser-prefs.js in my
tree wasn't being updated from cvs. Now fixed and revised patch attached.

Comment 70

[Ian Neal]

Attachment: Patch v0.4c - pud8 version

Comment 71

[[e-mail]]

Comment on attachment 162028 [details] [diff] [review]
Patch v0.4cw correct browser-prefs.js - wpud8 version

>+        var prefs = Components.classes[prefSvcContractID].getService(prefSvcIID)
>+                                                         .getBranch("browser.download.");
You should name this variable "branch", to make it clear.

>+        const kDownloadDirPref = "default_dir";
You're inconsistent, you use "dir" literally.

>+        if (!result) {
I think it would be cleaner to early return the result (from inside the
try/catch).

>+                    startDir = prefs.getComplexValue(kDownloadDirPref, nsILocalFile);
>+                    startDir = prefs.getComplexValue("dir", nsILocalFile);
I don't understand this fuss about a default download dir and a last download
dir.
a) you automagically download into a dir X
b) you prompt to download into a dir X
c) as b) but you overwrite X with the selected dir

>+        while (aLocalFile.exists()) {
>+            var parts = /.+-(\d+)(\..*)?$/.exec(aLocalFile.leafName);
>+            if (parts) {
>+                aLocalFile.leafName = aLocalFile.leafName.replace(/((\d+)\.)|((\d+)$)/,
>+                                                             function (str, dot, dotNum, noDot, noDotNum, pos, s) {
>+                                                                 return (parseInt(str) + 1) + (dot ? "." : "");
>+                                                             });
>+            }
>+            else {
>+                aLocalFile.leafName = aLocalFile.leafName.replace(/\.|$/, "-1$&");
>+            }
>+        }
Ewww. Did you understand that or just copy it? Not that I expect you to be able
to understand my version ;-)
/(-\d+)?(\.[^.]+)?$/.test(aLocalFile.leafName); aLocalFile.leafName =
RegExp.leftContext + (RegExp.$1 - 1) + RegExp.$2;

>   if (!aChosenData)
>     initFileInfo(fileInfo, aURL, aDocument, contentType);
>+  else
>+    file = aChosenData.file;
Hmmm... do we actually use aChosenData anywhere?

>-    userDirectory: getUserDownloadDir(prefs),
I think you're the last caller, so you can remove the definition too.

>+      var ifile = dirSvc.get("ProfD", nsIFile);
>+      path = ifile.QueryInterface(nsILocalFile);
IIRC you can .get an nsILocalFile directly, saving you a QI.

>+function setPrefDLElements()
>+{
>+  gLocation.disabled = (gPrompt.value != 0);
>+  gChooseButton.disabled = ((gLocation.value == 0) && (gPrompt.value == 0));
>+}
You mustn't enable locked preferences.

>+  if (ret == nsIFilePicker.returnOK) {
>+    var localFile = fp.file.QueryInterface(nsILocalFile);
>+    var viewable = fp.file.path;
>+    document.getElementById("downloadFolder").value = viewable;
>+    pref.setComplexValue(kDownloadDirPref, nsILocalFile, localFile)
>+  }
Hmm... so this saves immediately?

Comment 72

[Ian Neal]

Attachment: Patch v0.5w revised nowhitespace change version

Changes made:
* pref replaced with branch where appropiate
* kDownloadDirPref replaced by kDefaultDirPref to be consistent
* revised replicated filepicker logic as suggested (yes I did understand what
was there, and worked out what yours did too though I couldn't find leftContext
in my O'Reilly Javascript bible).
* left aChosenData in - it has only just been introduced so I guess it is for a
future enhancement
* removed getUserDownloadDir
* did the nsILocalFile directly in pref-download.js
* honoured prefLocking in pref-download.js
* ComplexValue prefs do get saved immediately in other parts of prefwindow

Comment 73

[Ian Neal]

Attachment: Revised Patch v0.5 - whitespace change version

Comment 74

[Ian Neal]

Comment on attachment 162428 [details] [diff] [review]
Patch v0.5w revised nowhitespace change version

Cancelling request as patch has bitrotted

Comment 75

[Ian Neal]

Attachment: Unbitrotted patch (v0.5aw)

This patch (without whitespace changes) also:
* Persists the last sound directory and passes it in to the filepicker
* Filters by .wav / .wave
* Respects pref locking in the sound when finished part of pref-download

Comment 76

[Ian Neal]

Attachment: Patch v0.5a - version of 0.5aw what includes whitespace changes

Comment 77

[Ian Neal]

Attachment: Patch v0.5bw updated pref-download.js

As per patch v0.5aw plus:
* Ensures sound_url is a URL

Comment 78

[Ian Neal]

Attachment: Patch v0.5b - whitespace change version of v0.5bw

Comment 79

[[e-mail]]

Comment on attachment 162859 [details] [diff] [review]
Patch v0.5b - whitespace change version of v0.5bw

Hmm... what should happen to Save (doc|frame|link) As...?

>+        const kDefaultDirPref = "default_dir";
Someone still needs to persuade me of the utility of two prefs...

>+                result = this.validateLeafName(startDir, aDefaultFile, aSuggestedFileExtension);
validateLeafName is quite short and could probably be inlined.

>+                if (result)
>+                    return result;
validateLeafName always appears to be successful.

>+            if (!startDir || (startDir && !startDir.exists())) 
Ooh, a trailing space! Actually, I just wanted to point out that (~A || (A &&
~B)) == (~A || (~A && ~B) || (A && ~B)) == (~A || ~B)

>+        var parts;
>+        while (aLocalFile.exists()) {
>+            parts = /(-\d+)?(\.[^.]+)?$/.test(aLocalFile.leafName);
>+            aLocaleFile.leafName = RegExp.leftContext + (RegExp.$1 - 1) + RegExp.$2;
You don't need to declare parts outside the loop, or at all in fact, because
you never use it.

>+    // If aChosenData is null then the file picker is shown.
>+    if (!aChosenData) {
I don't see why this is so far down the file... surely you don't need to get
prefs in this case?

>+  // Make sure sound_url setting is actually a URL
>+  var soundURL = gFinishedSound.value;
>+  if (soundURL !="" && soundURL.indexOf("file://") == -1) {
Is this really what you want? (I can't remember what the back end does).

>+WAVFiles=Audio WAV Files
Wave Audio Files? Or maybe just Sounds? (as per sndrec32 or sound.cpl)

Comment 80

[Ian Neal]

Attachment: Revised patch v0.6a - pud8 version

Comment 81

[Ian Neal]

Attachment: wpud8 version of patch v0.6a

Comment 82

[Ian Neal]

Attachment: One dir pref patch v0.6b - pud8

This patch:
* Uses only one pref setting for storing download dir
* Removes the As... from the various menus
* Does everything else previous patches did

Comment 83

[Ian Neal]

Attachment: Patch v0.6bw - wpud8 version of v0.6b

Comment 84

[[e-mail]]

Comment on attachment 165489 [details] [diff] [review]
One dir pref patch v0.6b - pud8

>+        var saveDir = true;
>+        // Pull in the user's preferences and get the correct download directory.
>         try {
>+            saveDir = (branch.getIntPref("location") == 0);
>+            if (!autoDownload) {
>+                var startDir = branch.getComplexValue(kDownloadDirPref, nsILocalFile);
>+                if (startDir.exists())
>+                    picker.displayDirectory = startDir;
>             }
>+        } catch (ex) {
>         }
Surely if we're auto downloading and the pref exists we'd have returned by now,
so you can save yourself the test. Also, I was wondering whether your use of
int prefs was delibrate to allow for future expansion or just because that's
the default type for radiogroups.

>+        // If not using default location save the user's choice of directory
>         result = picker.file;
> 
>+        if (result && saveDir && !autoDownload)
>+            branch.setComplexValue(kDownloadDirPref, nsILocalFile, result.parent);

Might be worth saving this in the autoDownload case in case it doesn't exist
yet.

>+    if (!dir || !autoDownload) {
Might be worth swapping the clauses i.e. if (dir && autoDownload) { ... } else
{ // Show the file picker

>+      var fpParams = {
>+        fp: makeFilePicker(),
>+        prefs: branch,
>+        userDirectory: dir,
>+        fpTitleKey: aFilePickerTitleKey,
>+        isDocument: isDocument,
>+        fileInfo: fileInfo,
>+        contentType: contentType,
>+        saveMode: saveMode,
>+        saveDir: (branch.getIntPref("location") == 0) && !autoDownload
>+      };
Originally I didn't think poseFilePicker should be saving the preference. But
then I noticed that it relied so heavily on preferences, it might actually make
for neater code to move the autodownload behaviour into poseFilePicker.

Comment 85

[Ian Neal]

Attachment: Revised poseFilePicker function - patch v0.6c - pud8

All autodownload stuff moved into poseFilePicker function
Revised nsHelperAppDlg promptForSaveToFile function so very similar to
contentAreaUtils functions

Comment 86

[Ian Neal]

Attachment: Patch v0.6cw - wpud8 version of v0.6c

Comment 87

[[e-mail]]

Comment on attachment 165921 [details] [diff] [review]
Revised poseFilePicker function - patch v0.6c - pud8

>+        if ((branch.getIntPref("location") == 0) || autoDownload)
Switch these both times, please (if you're still using them, see below)

>+  var dir = prefWindow.getPref("localfile", kDownloadDirPref);
I don't see this implemented anywhere, did you accidentally omit it?

>+  // Make sure sound_url setting is actually a URL
I think you should use URIFixup for this, see pref-proxies.js (this is what
stopped me giving you conditional r+)

>+<!ENTITY promptDownload.label "Prompt for download location and default to">
>+<!ENTITY promptDownload.accesskey "r">
>+<!ENTITY lastLocation.label "Last download folder">
>+<!ENTITY lastLocation.accesskey "L">
>+<!ENTITY defaultLocation.label "Default download folder">
>+<!ENTITY defaultLocation.accesskey "e">
>+<!ENTITY autoDownload.label "Automatically download files to default download folder">
>+<!ENTITY autoDownload.accesskey "A">
This isn't strictly true as there isn't a default download folder any more.
Perhaps you need to reorganize them? You might also want to have the download
folder enabled at all times.

Was there any particular reason you used integer preferences?
Also, I think you can dispense with the -w diffs now.

Comment 88

[Ian Neal]

Attachment: Boolean pref radiogroups - patch v0.6d - pud8

Changes since last patch:
* New radiogroups use boolean prefs.
* Changed wording from Default to Specified Download Folder.
* Minor streamling of code in contentAreaUtils.js - tests for aChoosenData just
the once now.
* Implementation of get/set Pref "localfile" moved from bug 264675 to here.
* Added FixURL function to nsPrefWindow.js and changed pref-download.js and
FixProxyURL in pref-proxies.js to use it.

Comment 89

[[e-mail]]

Comment on attachment 166041 [details] [diff] [review]
Boolean pref radiogroups - patch v0.6d - pud8

Looks good, although I haven't finished testing it yet.

>+        // If not using specified location save the user's choice of directory
>+        if (branch.getBoolPref("lastLocation") || autoDownload)
I wondered if there was virtue in swapping these, but as the first test is more
likely to be true and the second false, I suppose not. 

>+  if (aChosenData)
>+    file = aChosenData.file;
>+  else
>     initFileInfo(fileInfo, aURL, aDocument, contentType);
It looks as if you're missing an { here.

>+    return URIFixup.createFixupURI(url, nsIURIFixup.FIXUP_FLAG_NONE).spec;
>+  } catch (ex) { }
>+  return url;
>+}
Not sure whether it's better style to return the url in the catch. Or maybe use
url = URIFixup.createFixupURI(...).spec; in the try.

>+    <radiogroup id="autoDownload" oncommand="setPrefDLElements();"
>+                preftype="bool" prefstring="browser.download.autoDownload">
>+      <radio value="false" label="&promptDownload.label;" accesskey="&promptDownload.accesskey;"/>
>+      <radiogroup id="downloadLocation" class="indent" oncommand="setPrefDLElements();"
The oncommand event should bubble up to the autoDownload group, saving you the
trouble of handling it twice.

>+    initialDir = fileHandler.getFileFromURLSpec(gFinishedSound.value)
>+                            .QueryInterface(nsILocalFile);
Isn't this the sound file, rather than the dir, so it needs a .parent before
the .QueryInterface?

>+<!ENTITY autoDownload.label "Automatically download files to default download folder">
current download folder? (also need to change the properties string)

>+WAVFiles=Sounds (*.wav)
I'm never sure whether the (*.wav) should be included in the string. I think
the different filepickers do different things :-/

Comment 90

[Ian Neal]

Attachment: Bubbles and parents patch v0.6e - pud8

Tweaked as per comments above plus made nsHelperAppDlg and contentAreaUtils set
branch.download.dir the same way by adding a .parent.

Comment 91

[[e-mail]]

Comment on attachment 166099 [details] [diff] [review]
Bubbles and parents patch v0.6e - pud8

Nice :-)

Comment 92

[jag (Peter Annema)]

Comment on attachment 166099 [details] [diff] [review]
Bubbles and parents patch v0.6e - pud8

Global nit: most of this file uses 4 space indents, therefore per Mozilla style
guidelines (when in Rome, ...) so should you.

>Index: embedding/components/ui/helperAppDlg/nsHelperAppDlg.js
>===================================================================

>+        var branch = Components.classes[prefSvcContractID].getService(prefSvcIID)
>+                                                         .getBranch("browser.download.");

Indent the previous line one more space to line up the '.'s (fall-out from pref
-> branch)

>+          if (aDefaultFile == "")
>+            aDefaultFile = "unnamed" + (aSuggestedFileExtension ? "." + aSuggestedFileExtension : "");

Perhaps overkill, but I think it would be appreciated if "unnamed" is
localizable. Perhaps a follow-up bug?

>Index: xpfe/communicator/resources/content/contentAreaUtils.js
>===================================================================

Comparing saveAsType to 0 and 2 isn't really informative. I'm assuming there's
some meaningful name associated with these "magic" values, perhaps you could
add those names as consts in this code (if they're not already consts on an
interface somewhere)?

>+    // Since we're automatically downloading, we don't get the file picker's 
>+    // logic to check for existing files, so we need to do that here.
>+    //
>+    // Note - this code is identical to that in nsHelperAppDlg.js. 
>+    // If you are updating this code, update that code too! We can't share code
>+    // here since that code is called in a js component.

Yeah, I really don't like this duplication, but it's hard to get around... It'd
be nice if you could split it off into a function which would have the same
name in both places, making it easier to find.

>Index: xpfe/communicator/resources/locale/en-US/contentAreaCommands.dtd
>===================================================================

I'm not sure I'm happy with removing "As..." from these labels. By convention
we have an ellipsis ("...") to indicate a dialog coming up requiring the user's
input to complete the action.

What we should do is have both labels and have one or the other be used based
on the pref's setting. Perhaps leave "As..." for now and file a follow-up bug
on that.

>Index: xpfe/components/prefwindow/resources/content/nsPrefWindow.js
>===================================================================
>+function FixURL(url)
>+{
>+  const nsIURIFixup = Components.interfaces.nsIURIFixup;
>+  try {
>+    var URIFixup = Components.classes["@mozilla.org/docshell/urifixup;1"]
>+                             .getService(nsIURIFixup);
>+    url = URIFixup.createFixupURI(url, nsIURIFixup.FIXUP_FLAG_NONE).spec;
>+  } catch (ex) { }
>+  return url;
>+}

Not sure this belongs in here... I don't wanna start a precedent where
nsPrefWindow.js becomes a placeholder for misc. utility functions. That said,
I'm not sure where it would belong. The encapsulated code isn't so big or
complex that you couldn't just inline it, though.

>Index: xpfe/components/prefwindow/resources/content/pref-download.js
>===================================================================

...
>+var gPromptLocked;
>+var gFinishedSoundLocked;
>+var ioService;

Inconsistent naming. gIOService, please.


>+function Browse()
>+{
>+  var initialDir = null;
>+  if (gFinishedSound.value !="") {

Missing space between != and ""

> function PreviewSound()
> {
>   if (!gSound)
>     gSound = Components.classes["@mozilla.org/sound;1"].createInstance(Components.interfaces.nsISound);
> 
>+  gSound.play(ioService.newURI(gFinishedSound.value, null, null));
> }

What about when the finished sound field has no value, but the checkbox is
checked? I think having the system beep sound is fine in that case... (Perhaps
the preview button should be enabled too in that case, so the user can hear the
system beep, and discover it?)

Looks good otherwise. Please fix the nits and address my comments.

Comment 93

[Ian Neal]

Attachment: Initial As/No As patch v0.7 - pud8

Changes since last patch:
* 4 space indents followed in nsHelperAppDlg.js
* "unamed" made localizable via nsHelperAppDlg.properties
* saveAsTypes given meaningful const names
* split replicated file picker into separate functions
* "As..." now appears when autoDownload is false
* inlined FixURL function and removed from nsPrefWindow.js
* renamed ioService to gIOService
* if sound_url is blank preview plays system beep sound

Comment 94

[Ian Neal]

Attachment: As or not to As patch v0.7a - pud8

Changes in this patch:
* Uses goSetMenuValue function as suggested by Neil
* Corrects spelling of unnamed
* Simplifies aSuggestedFileExt substitution

Comment 95

[jag (Peter Annema)]

Comment on attachment 167914 [details] [diff] [review]
As or not to As patch v0.7a - pud8

rs=jag

Comment 96

[Ian Neal]

Comment on attachment 167914 [details] [diff] [review]
As or not to As patch v0.7a - pud8

Patch checked in.

Comment 97

[Peter Weilbacher]

Wow, good work that I only discovered today. Don't let anybody say that there is
no UI work going on for Seamonkey. :-)

As this bug is not yet marked resolved let me add a minor nit here instead of
opening a new bug: The "Choose Folder" button in the prefs is missing the
ellipsis (or is there a reason for that?).
In xpfe/components/prefwindow/resources/locale/en-US/pref-download.dtd:
-<!ENTITY chooseDownloadFolder.label "Choose Folder">
+<!ENTITY chooseDownloadFolder.label "Choose Folder...">

Comment 98

[Ian Neal]

Attachment: Missing ... fix v0.1

It was overlooked in the original patch - thanks for the catch

Comment 99

[Ian Neal]

Comment on attachment 169477 [details] [diff] [review]
Missing ... fix v0.1

Checking in pref-download.dtd;
/cvsroot/mozilla/xpfe/components/prefwindow/resources/locale/en-US/pref-downloa
d.dtd,v  <--  pref-download.dtd
new revision: 1.17; previous revision: 1.16
done

Comment 100

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

Verified FIXED using SeaMonkey 1.5a;Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060110 Mozilla/1.0