Closed
Bug 7840
Opened 26 years ago
Closed 20 years ago
Ability to specify default directory for downloads
Categories
(SeaMonkey :: Download & File Handling, enhancement, P3)
Tracking
(Not tracked)
VERIFIED
FIXED
Future
People
(Reporter: hecker, Assigned: iannbugzilla)
References
(Blocks 1 open bug)
Details
(Whiteboard: could be dupe of 27493)
Attachments
(2 files, 27 obsolete files)
(deleted),
patch
|
neil
:
review+
jag+mozilla
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
neil
:
review+
neil
:
superreview+
|
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?
Comment 2•26 years ago
|
||
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.
Updated•25 years ago
|
Assignee: dp → valeski
Updated•25 years ago
|
Assignee: valeski → dp
Comment 4•25 years ago
|
||
not me. this is an fe issue. once the fe receives notification that a file is
coming, they throw the UI, not necko.
Updated•25 years ago
|
Assignee: dp → law
Comment 5•25 years ago
|
||
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
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 10•25 years ago
|
||
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
Updated•24 years ago
|
Target Milestone: M16 → Future
Comment 12•24 years ago
|
||
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•24 years ago
|
||
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•24 years ago
|
||
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•24 years ago
|
||
tever, would this be your realm? or mine?
don, shall i move this over to Preferences?
Keywords: 4xp
QA Contact: tever
Comment 16•24 years ago
|
||
*** Bug 44365 has been marked as a duplicate of this bug. ***
Comment 17•24 years ago
|
||
Since Don has left, Vishy is taking his bugs in bulk, pending reassignment.
thanks,
Vishy
Assignee: don → vishy
Comment 19•24 years ago
|
||
*** Bug 64759 has been marked as a duplicate of this bug. ***
Comment 20•24 years ago
|
||
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•24 years ago
|
||
Please also see bug 64887 for integration of this feature into a built-in
"Download Manager".
Comment 22•24 years ago
|
||
Comment 23•24 years ago
|
||
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•24 years ago
|
||
[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
Comment 25•24 years ago
|
||
*** Bug 62389 has been marked as a duplicate of this bug. ***
Comment 27•24 years ago
|
||
Suggest to add Keywords: *nsCatFood* and *mostfreq* since this should really be
fixed ;)
Comment 28•24 years ago
|
||
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•24 years ago
|
||
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•24 years ago
|
||
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•24 years ago
|
||
The open/save dialogues of EVERY modern OS allow you to navigate to other
directories/drives from where you are - so don't worry ;)
Updated•23 years ago
|
Status: NEW → ASSIGNED
Target Milestone: Future → mozilla1.0
Updated•23 years ago
|
Target Milestone: mozilla0.9.4 → mozilla1.0
Comment 33•23 years ago
|
||
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
Comment 34•23 years ago
|
||
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•23 years ago
|
||
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 37•23 years ago
|
||
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•23 years ago
|
||
*** Bug 121027 has been marked as a duplicate of this bug. ***
Comment 39•23 years ago
|
||
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•23 years ago
|
||
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
Comment 41•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
This is not a defect to be fixed, it is an enhancement that will not be
implemented in the current release cycle.
Comment 45•22 years ago
|
||
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
Updated•22 years ago
|
QA Contact: sairuh → petersen
Summary: [RFE] Ability to specify default directory for downloads → Ability to specify default directory for downloads
Comment 47•21 years ago
|
||
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•21 years ago
|
||
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•21 years ago
|
||
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•21 years ago
|
||
*** Bug 228390 has been marked as a duplicate of this bug. ***
Comment 51•21 years ago
|
||
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•21 years ago
|
||
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
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.
Attachment #141286 -
Attachment is obsolete: true
Comment 56•21 years ago
|
||
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.
Attachment #141287 -
Flags: review?(neil.parkwaycc.co.uk)
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)
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
Assignee | ||
Comment 61•20 years ago
|
||
UI of upcoming patch relies on fix in bug 250817
Status: NEW → ASSIGNED
Depends on: 250817
Assignee | ||
Comment 62•20 years ago
|
||
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
Assignee | ||
Comment 63•20 years ago
|
||
If patch v0.4 is r/sr then this is the patch to be checked in.
Assignee | ||
Comment 64•20 years ago
|
||
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 | ||
Comment 65•20 years ago
|
||
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)
Assignee | ||
Comment 66•20 years ago
|
||
Attachment #152892 -
Attachment is obsolete: true
Attachment #161867 -
Flags: review?(neil.parkwaycc.co.uk)
Assignee | ||
Comment 67•20 years ago
|
||
fixes nit replacing style="margin-left:2em" with class="indent"
Attachment #161867 -
Attachment is obsolete: true
Assignee | ||
Comment 68•20 years ago
|
||
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)
Assignee | ||
Comment 69•20 years ago
|
||
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)
Assignee | ||
Comment 70•20 years ago
|
||
Attachment #162009 -
Attachment is obsolete: true
Comment 71•20 years ago
|
||
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-
Assignee | ||
Comment 72•20 years ago
|
||
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
Attachment #162428 -
Attachment description: Patch v0.5 revised nowhitespace change version → Patch v0.5w revised nowhitespace change version
Assignee | ||
Comment 73•20 years ago
|
||
Attachment #162029 -
Attachment is obsolete: true
Attachment #162428 -
Flags: review?(neil.parkwaycc.co.uk)
Assignee | ||
Comment 74•20 years ago
|
||
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)
Assignee | ||
Comment 75•20 years ago
|
||
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
Assignee | ||
Comment 76•20 years ago
|
||
Attachment #162429 -
Attachment is obsolete: true
Attachment #162698 -
Flags: review?(neil.parkwaycc.co.uk)
Attachment #162698 -
Flags: review?(neil.parkwaycc.co.uk)
Assignee | ||
Comment 77•20 years ago
|
||
As per patch v0.5aw plus:
* Ensures sound_url is a URL
Attachment #162698 -
Attachment is obsolete: true
Assignee | ||
Comment 78•20 years ago
|
||
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 79•20 years ago
|
||
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)
Assignee | ||
Comment 80•20 years ago
|
||
Attachment #162859 -
Attachment is obsolete: true
Attachment #164675 -
Flags: review?(neil.parkwaycc.co.uk)
Assignee | ||
Comment 81•20 years ago
|
||
Attachment #162858 -
Attachment is obsolete: true
Attachment #164675 -
Flags: review?(neil.parkwaycc.co.uk)
Assignee | ||
Comment 82•20 years ago
|
||
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)
Assignee | ||
Comment 83•20 years ago
|
||
Attachment #164676 -
Attachment is obsolete: true
Comment 84•20 years ago
|
||
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)
Assignee | ||
Comment 85•20 years ago
|
||
All autodownload stuff moved into poseFilePicker function
Revised nsHelperAppDlg promptForSaveToFile function so very similar to
contentAreaUtils functions
Attachment #165489 -
Attachment is obsolete: true
Assignee | ||
Comment 86•20 years ago
|
||
Attachment #165490 -
Attachment is obsolete: true
Attachment #165921 -
Flags: review?(neil.parkwaycc.co.uk)
Comment 87•20 years ago
|
||
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-
Assignee | ||
Comment 88•20 years ago
|
||
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)
Comment 89•20 years ago
|
||
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)
Assignee | ||
Comment 90•20 years ago
|
||
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 91•20 years ago
|
||
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)
Updated•20 years ago
|
Product: Browser → Seamonkey
Comment 92•20 years ago
|
||
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-
Assignee | ||
Comment 93•20 years ago
|
||
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)
Assignee | ||
Comment 94•20 years ago
|
||
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)
Updated•20 years ago
|
Attachment #167914 -
Flags: review?(neil.parkwaycc.co.uk) → review+
Attachment #167914 -
Flags: superreview?(jag)
Comment 95•20 years ago
|
||
Comment on attachment 167914 [details] [diff] [review]
As or not to As patch v0.7a - pud8
rs=jag
Attachment #167914 -
Flags: superreview?(jag) → superreview+
Assignee | ||
Comment 96•20 years ago
|
||
Comment on attachment 167914 [details] [diff] [review]
As or not to As patch v0.7a - pud8
Patch checked in.
Comment 97•20 years ago
|
||
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...">
Assignee | ||
Comment 98•20 years ago
|
||
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)
Updated•20 years ago
|
Attachment #169477 -
Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #169477 -
Flags: superreview+
Attachment #169477 -
Flags: review?(neil.parkwaycc.co.uk)
Attachment #169477 -
Flags: review+
Assignee | ||
Comment 99•20 years ago
|
||
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: 20 years ago
Resolution: --- → FIXED
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.
Description
•