(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)

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

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

I stand corrected.

Assigning to me for M14 ...

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.

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 ...

Move to M16 for now ...

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?

*** 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

nav triage team: Not a beta1 blocker, marking 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.]

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

Taking, cc law.

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

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 ;)

usability/polish 0.9.4.

hasn't this been more or less implemented? resolve as needed, thx!

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

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.

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

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?

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)

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

Nav triage team: nsbeta1-

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.

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 ;).

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 ;).

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

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

Obsoleting that picture, because it is irrelevant. This adds preliminary changes to the downloads pref panel so non-power-users can set it.

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

UI of upcoming patch relies on fix in bug 250817

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

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

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

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 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?

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 on attachment 162428 [details] [diff] [review] Patch v0.5w revised nowhitespace change version Cancelling request as patch has bitrotted

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

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

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)

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 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.

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

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.

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 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 :-/

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

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

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.

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

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

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

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

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

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