Closed Bug 7840 Opened 22 years ago Closed 15 years ago

Ability to specify default directory for downloads

Categories

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

x86
All
enhancement

Tracking

(Not tracked)

VERIFIED FIXED
Future

People

(Reporter: hecker, Assigned: iann_bugzilla)

References

(Blocks 1 open bug)

Details

(Whiteboard: could be dupe of 27493)

Attachments

(2 files, 27 obsolete files)

58.94 KB, patch
neil
: review+
jag+mozilla
: superreview+
Details | Diff | Splinter Review
1.19 KB, patch
neil
: review+
Details | Diff | Splinter Review
(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.
Interesting RFE for all save/publish operations.  Doesn't 1-button publish
already do this?
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)
Assignee: johng → dp
Just moved bug to Bugzilla since this is a 5.0 feature request.  Reassigning to
DP.
Assignee: dp → valeski
Assignee: valeski → dp
not me. this is an fe issue. once the fe receives notification that a file is
coming, they throw the UI, not necko.
Assignee: dp → law
I stand corrected.
Assignee: law → don
Summary: RFE: Ability to designate default directory for FTP downloads → [RFE] Ability to designate default directory for FTP downloads
Target Milestone: M14
Assigning to me for M14 ...
Component: XP File Handling → XPApps
Bug #7380 suggests this but allows specification on a URL-by-URL scale.
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.
Target Milestone: M14 → M15
Move to M15 for later evaluation.
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 ...
OS: Windows 95 → All
Move to M16 for now ...
Target Milestone: M15 → M16
Target Milestone: M16 → Future
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...
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.
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).
tever, would this be your realm? or mine?

don, shall i move this over to Preferences?
Keywords: 4xp
QA Contact: tever
*** Bug 44365 has been marked as a duplicate of this bug. ***
Since Don has left, Vishy is taking his bugs in bulk, pending reassignment.
thanks,
	Vishy
Assignee: don → vishy
nav triage team:

Not a beta1 blocker, marking nsbeta1-
Keywords: nsbeta1-
*** Bug 64759 has been marked as a duplicate of this bug. ***
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.

Please also see bug 64887 for integration of this feature into a built-in
"Download Manager".
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).
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.
[Resummarizing since this doesn't apply just to FTP.]
Summary: [RFE] Ability to designate default directory for FTP downloads → [RFE] Ability to specify default directory for downloads
*** Bug 62389 has been marked as a duplicate of this bug. ***
Taking, cc law.
Assignee: vishy → blakeross
Suggest to add Keywords: *nsCatFood* and *mostfreq* since this should really be
fixed ;)
Whiteboard: could be dupe of 27493
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            |
|                                                        | 
+--------------------------------------------------------+
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 :)
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.
The open/save dialogues of EVERY modern OS allow you to navigate to other
directories/drives from where you are - so don't worry ;)
Status: NEW → ASSIGNED
Target Milestone: Future → mozilla1.0
usability/polish 0.9.4.
Target Milestone: mozilla1.0 → mozilla0.9.4
Target Milestone: mozilla0.9.4 → mozilla1.0
hasn't this been more or less implemented?

resolve as needed, thx!
Assignee: blakeross → law
Status: ASSIGNED → NEW
Component: XP Apps → File Handling
QA Contact: tever → sairuh
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.
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?
->future
Target Milestone: mozilla1.0 → Future
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.
Blocks: 75364
*** Bug 121027 has been marked as a duplicate of this bug. ***
Blocks: 121029
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?
Blocks: 129923
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)

Assignee: law → blaker
Component: File Handling → Download Manager
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).
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.
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.
This is not a defect to be fixed, it is an enhancement that will not be
implemented in the current release cycle. 
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
Keywords: nsbeta1
Nav triage team: nsbeta1-
Keywords: nsbeta1nsbeta1-
Blocks: 164421
QA Contact: sairuh → petersen
Summary: [RFE] Ability to specify default directory for downloads → Ability to specify default directory for downloads
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.
Attached image Error dialog when in Download Manager (obsolete) —
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.
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!
*** Bug 228390 has been marked as a duplicate of this bug. ***
I can't believe this has not been addressed! It's the only thing keeping me from
using Mozilla as my preferred browser!
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?
-> me.  Maybe I can do something with this ;).
Assignee: firefox → cst
Attached patch Patch v0.1 (obsolete) — Splinter Review
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
;).
Attached patch Patch v0.2 (obsolete) — Splinter Review
Patch a few more files - I'm not sure about the two different versions of
nsHelperAppDlg.js, but I patched them both.
Attachment #141286 - Attachment is obsolete: true
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.
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 on attachment 141287 [details] [diff] [review]
Patch v0.2

sorry for the spam... r? on wrong patch
Attachment #141287 - Flags: review?(neil.parkwaycc.co.uk)
Attached patch Patch v0.3 (obsolete) — Splinter Review
Obsoleting that picture, because it is irrelevant.

This adds preliminary changes to the downloads pref panel so non-power-users
can set it.
Attachment #125464 - Attachment is obsolete: true
Attachment #141287 - Attachment is obsolete: true
->nobody, I am obviously not getting anything done here.
Assignee: cst → nobody
UI of upcoming patch relies on fix in bug 250817
Status: NEW → ASSIGNED
Depends on: 250817
This implements UI, and associated backend, as suggested by Hixie in comment 40
Attachment #141900 - Attachment is obsolete: true
Attachment #152890 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #152890 - Attachment description: Updated patch ignoring whitespace changes → Updated patch (v0.4) ignoring whitespace changes
If patch v0.4 is r/sr then this is the patch to be checked in.
Comment on attachment 152890 [details] [diff] [review]
Updated patch (v0.4) ignoring whitespace changes

Cancelling request until I unbitrot patch
Attachment #152890 - Flags: review?(neil.parkwaycc.co.uk)
Assignee: nobody → bugzilla
Attachment #152890 - Attachment is obsolete: true
Attachment #161867 - Attachment description: Unbitrotted patch (v0.4a) - whitespace changes ignored → Unbitrotted patch (v0.4aw) - whitespace changes ignored
Attachment #161867 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #152892 - Attachment is obsolete: true
Attachment #161867 - Flags: review?(neil.parkwaycc.co.uk)
fixes nit replacing style="margin-left:2em" with class="indent"
Attachment #161867 - Attachment is obsolete: true
Attachment #161868 - Attachment is obsolete: true
Attachment #162008 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #162008 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #162008 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #162008 - Flags: review?(neil.parkwaycc.co.uk)
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.
Attachment #162008 - Attachment is obsolete: true
Attachment #162028 - Flags: review?(neil.parkwaycc.co.uk)
Attached patch Patch v0.4c - pud8 version (obsolete) — Splinter Review
Attachment #162009 - Attachment is obsolete: true
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?
Attachment #162028 - Flags: review?(neil.parkwaycc.co.uk) → review-
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
Attachment #162028 - Attachment is obsolete: true
Depends on: 264675
Attachment #162428 - Attachment description: Patch v0.5 revised nowhitespace change version → Patch v0.5w revised nowhitespace change version
Attachment #162029 - Attachment is obsolete: true
Attachment #162428 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 162428 [details] [diff] [review]
Patch v0.5w revised nowhitespace change version

Cancelling request as patch has bitrotted
Attachment #162428 - Flags: review?(neil.parkwaycc.co.uk)
Attached patch Unbitrotted patch (v0.5aw) (obsolete) — Splinter Review
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
Attachment #162428 - Attachment is obsolete: true
Attachment #162429 - Attachment is obsolete: true
Attachment #162698 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #162698 - Flags: review?(neil.parkwaycc.co.uk)
As per patch v0.5aw plus:
* Ensures sound_url is a URL
Attachment #162698 - Attachment is obsolete: true
Attachment #162699 - Attachment is obsolete: true
Attachment #162858 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #162858 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #162859 - Flags: review?(neil.parkwaycc.co.uk)
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)
Attachment #162859 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #162859 - Attachment is obsolete: true
Attachment #164675 - Flags: review?(neil.parkwaycc.co.uk)
Attached patch wpud8 version of patch v0.6a (obsolete) — Splinter Review
Attachment #162858 - Attachment is obsolete: true
Attachment #164675 - Flags: review?(neil.parkwaycc.co.uk)
Attached patch One dir pref patch v0.6b - pud8 (obsolete) — Splinter Review
This patch:
* Uses only one pref setting for storing download dir
* Removes the As... from the various menus
* Does everything else previous patches did
Attachment #164675 - Attachment is obsolete: true
Attachment #165489 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #164676 - Attachment is obsolete: true
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.
Attachment #165489 - Flags: review?(neil.parkwaycc.co.uk)
All autodownload stuff moved into poseFilePicker function
Revised nsHelperAppDlg promptForSaveToFile function so very similar to
contentAreaUtils functions
Attachment #165489 - Attachment is obsolete: true
Attachment #165490 - Attachment is obsolete: true
Attachment #165921 - Flags: review?(neil.parkwaycc.co.uk)
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.
Attachment #165921 - Flags: review?(neil.parkwaycc.co.uk) → review-
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.
Attachment #165921 - Attachment is obsolete: true
Attachment #165922 - Attachment is obsolete: true
Attachment #166041 - Flags: review?(neil.parkwaycc.co.uk)
Blocks: 264675
No longer depends on: 264675
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 :-/
Attachment #166041 - Flags: review?(neil.parkwaycc.co.uk)
Tweaked as per comments above plus made nsHelperAppDlg and contentAreaUtils set
branch.download.dir the same way by adding a .parent.
Attachment #166041 - Attachment is obsolete: true
Attachment #166099 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 166099 [details] [diff] [review]
Bubbles and parents patch v0.6e - pud8

Nice :-)
Attachment #166099 - Flags: review?(neil.parkwaycc.co.uk) → review+
Attachment #166099 - Flags: superreview?(jag)
Product: Browser → Seamonkey
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.
Attachment #166099 - Flags: superreview?(jag) → superreview-
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
Attachment #166099 - Attachment is obsolete: true
Attachment #167771 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #167771 - Flags: review?(neil.parkwaycc.co.uk)
Changes in this patch:
* Uses goSetMenuValue function as suggested by Neil
* Corrects spelling of unnamed
* Simplifies aSuggestedFileExt substitution
Attachment #167771 - Attachment is obsolete: true
Attachment #167914 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #167914 - Flags: review?(neil.parkwaycc.co.uk) → review+
Attachment #167914 - Flags: superreview?(jag)
Comment on attachment 167914 [details] [diff] [review]
As or not to As patch v0.7a - pud8

rs=jag
Attachment #167914 - Flags: superreview?(jag) → superreview+
Comment on attachment 167914 [details] [diff] [review]
As or not to As patch v0.7a - pud8

Patch checked in.
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...">
It was overlooked in the original patch - thanks for the catch
Attachment #169477 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #169477 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #169477 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #169477 - Flags: superreview+
Attachment #169477 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #169477 - Flags: review+
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
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
No longer blocks: 164421
Blocks: 158464
Blocks: 321981
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.