Closed Bug 284089 Opened 20 years ago Closed 19 years ago

Settings in "Download Folder" section of pref completely ignored

Categories

(Firefox :: Settings UI, defect)

Product:
Firefox
Component:
Settings UI
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: me, Assigned: amotohiko_mozillafirebird)

References

Details

(Whiteboard: [bs] large patch has review, smaller patch would be better but depends on bug 299162)

Attachments

(4 files, 7 obsolete files)

updated patch for Bug 294759.
11.64 KB, patch
mconnor
: review+
benjamin
: approval-aviary1.1a2+
Details | Diff | Splinter Review
updated alternate patch
7.05 KB, patch
Details | Diff | Splinter Review
diff -w version of patch getting landed
9.22 KB, patch
Details | Diff | Splinter Review
187860 patch addon
933 bytes, patch
Details | Diff | Splinter Review 
 
Oops, pressed enter instead of tab :-)

Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050228 Firefox/1.0+

Changing the various settings in the "Download Folders" section of prefs has no
effect.

Steps to Reproduce
1. Goto Tools->Options->Downloads
2a. Change the folder in the "Downloads Folder" section.
or 2b. Change the "Ask me" radio button.
3. Download a file

Actual results
The settings seem to be followed for the first downloaded file in a new profile,
but after that it continues to use those original settings regardless of how
much they are changed in Options.

Expected results
Use settings from "Download Folder" area.

The "All files downloaded to" area of the DM shows the folder you've set in
Options even though the file may end up somewhere else entirely due to this bug.
This bug is caused by non-touching pref "browser.download.defaultFolder".
To "Ask me where to save every file", this pref should be _unset_. To "Save all
files to this folder", this pref should have its folder name or unset.
http://lxr.mozilla.org/mozilla/source/browser/base/content/contentAreaUtils.js#364

But preferences.xml cannot unset any pref, so add this feature first.

Null value means 'unset it'.
Attachment #177356 - Flags: review?
Attached patch patch for Prefwindow V/downloads (obsolete) — — Splinter Review 
UI patch.

Always unset pref "browser.download.defaultFolder", since nsHelperAppDlg.js
will set this.
http://lxr.mozilla.org/mozilla/source/toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in#136
Attachment #177357 - Flags: review?
Flags: blocking-aviary1.1?
Flags: blocking-aviary1.1? → blocking-aviary1.1+
*** Bug 289874 has been marked as a duplicate of this bug. ***
You have to request review from a specific person, like bugs@bengoodger.com, who
wrote the preferences code, or mconnor@steelgryphon.com, who is more responsive
in doing reviews.
Assignee: bugs → amotohiko_mozillafirebird
(In reply to comment #3)
> Created an attachment (id=177357) [edit]
> patch for Prefwindow V/downloads
> 
> UI patch.

How can I apply this patch ? 
Need to have sorce files or can I apply it on build (windows) ?
*** Bug 290301 has been marked as a duplicate of this bug. ***
You have to apply it to the sources of course (that's valid for 99,999% of all
patches in bugzilla)
I applied the patch (id=177357).  It applied cleanly to todays' sources (4.14.05)
but had no effect whatever on the bug I reported (bug #29031).

This is a very annoying bug which has been around for awhile. Can I do anything
to help with debugging?
(In reply to comment #9)
> I applied the patch (id=177357).  It applied cleanly to todays' sources (4.14.05)
> but had no effect whatever on the bug I reported (bug #29031).
> 
> This is a very annoying bug which has been around for awhile. Can I do anything
> to help with debugging?

Typo -- that should be bug #290301
Attachment #177356 - Flags: review? → review?(beng)
Attachment #177356 - Flags: review?(beng) → review?(bugs)
Attachment #177357 - Flags: review? → review?(bugs)
(In reply to comment #5)
> You have to request review from a specific person, like bugs@bengoodger.com, who
> wrote the preferences code, or mconnor@steelgryphon.com, who is more responsive
> in doing reviews.
Thank you. I request review these to Ben.


(In reply to comment #9)
> I applied the patch (id=177357).  It applied cleanly to todays' sources (4.14.05)
> but had no effect whatever on the bug I reported (bug #29031).
> 
> This is a very annoying bug which has been around for awhile. Can I do anything
> to help with debugging?
You must apply both attachment #177356 [details] [diff] [review] (backend) and attachment #177357 [details] [diff] [review] (frontend).
Instead of unsetting browser.download.defaultFolder, shouldn't the browser just
follow browser.download.useDownloadDir ?

When browser.download.useDownloadDir is set to 'false' then
'browser.download.defaultFolder' should be ignored, but instead it seems to
check the value of 'browser.download.defaultFolder' making the
'browser.download.useDownloadDir' pref useless ?
Attached patch another patch (obsolete) — — Splinter Review 
(In reply to comment #12)
> Instead of unsetting browser.download.defaultFolder, shouldn't the browser
just
> follow browser.download.useDownloadDir ?
Thank you. This approach is good.

This patch checks 'useDownloadDir' first in both case - alt-click on any links,
and click on download-actioned links (like application/octet-stream and so on).
Attachment #177356 - Attachment is obsolete: true
Attachment #177357 - Attachment is obsolete: true
Attachment #182310 - Flags: review?(bugs)
*** Bug 292678 has been marked as a duplicate of this bug. ***
Attached patch correct patch (obsolete) — — Splinter Review 
oops.
Attachment #182310 - Attachment is obsolete: true
Attachment #182560 - Flags: review?(bugs)
Comment on attachment 177356 [details] [diff] [review]
add ability of unsetting pref to preferences.xml

Removing review requests from obsolete patches.
Attachment #177356 - Flags: review?(bugs)
Attachment #177357 - Flags: review?(bugs)
Attachment #182310 - Flags: review?(bugs)
Related to/duplicate of bug 249591?
*** Bug 295402 has been marked as a duplicate of this bug. ***
out of curiosity, could this have been a regression for the fix for bug 241245?
OS: other → All
(In reply to comment #19)
> out of curiosity, could this have been a regression for the fix for bug 241245?

Not likely, this was a mistake made when implementing the new Prefs dialog.
*** Bug 295384 has been marked as a duplicate of this bug. ***
I have found that setting a download folder in Tools -> Options -> Downloads
only affects the prefs browser.download.dir and browser.download.downloadDir and
NOT browser.download.defaultFolder. If browser.download.defaultFolder is
manually set to the desired download location, Firefox respects that pref and
downloads files to that folder. So it appears that the current patch will fix
the issue of the UI setting the pref correctly.
Comment on attachment 182560 [details] [diff] [review]
correct patch

>@@ -195,8 +194,9 @@ nsUnknownContentTypeDialog.prototype = {
>         }
>       }
>       
>-      if (!result) {
>+      if (!autodownload || !result) {
>         // Use file picker to show dialog.
>+        // We care about invalid value of pref "browser.download.folderList".

this comment doesn't make sense to me

>         var nsIFilePicker = Components.interfaces.nsIFilePicker;
>         var picker = Components.classes["@mozilla.org/filepicker;1"].createInstance(nsIFilePicker);
> 
>Index: toolkit/content/contentAreaUtils.js
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/content/contentAreaUtils.js,v
>retrieving revision 1.69
>diff -u -p -r1.69 contentAreaUtils.js
>--- toolkit/content/contentAreaUtils.js	27 Apr 2005 11:50:24 -0000	1.69
>+++ toolkit/content/contentAreaUtils.js	4 May 2005 07:14:11 -0000
>@@ -385,29 +385,23 @@ function getTargetFile(aData, aSniffer, 
> 
>   const nsILocalFile = Components.interfaces.nsILocalFile;
> 
>-  // ben 07/31/2003:
>-  // |browser.download.defaultFolder| holds the default download folder for 
>-  // all files when the user has elected to have all files automatically
>-  // download to a folder. The values of |defaultFolder| can be either their
>-  // desktop, their downloads folder (My Documents\My Downloads) or some other
>-  // location of their choosing (which is mapped to |browser.download.dir|
>-  // This pref is _unset_ when the user has elected to be asked about where
>-  // to place every download - this will force the prompt to ask the user
>-  // where to put saved files. 
>+  // Get destination directory.
>+  //  First. 'browser.downloads.defaultFolder'.
>+  //  Second. 'browser.downloads.lastDir'.
>+  //  Third. Desktop or Home.
>   var dir = null;
>   try {
>     dir = prefs.getComplexValue("defaultFolder", nsILocalFile);
>   }
>   catch (e) { }
>   
>-  var file;
>-  if (!aSkipPrompt || !dir) {
>-    // If we're asking the user where to save the file, root the Save As...
>-    // dialog on they place they last picked. 

This whole section is not a desired change.  Save As... has an established
behaviour.  Changing that is out of scope of this bug, and not something we
want to break.	This code puts the preferred dir always on defaultFolder, if
you don't manually unset it then whatever its set to previously will always
override.

also, the nested if (!dir) { try/catch; if (!dir)) bits aren't good, just have
three if statements without nesting.

>+  var file;
>+  if (!aSkipPrompt || !useDownloadDir) {
>+    // If we're asking the user where to save the file, root the Save As...
>+    // dialog on they place they last picked. 

and yet, you preserved the comment saying that we root on lastDir, when that
just isn't the case as I understand it.
Attachment #182560 - Flags: review?(bugs) → review-
*** Bug 297233 has been marked as a duplicate of this bug. ***
Anbo, can you come up with a follow-up patch? Mconnor might be able to help.
Attached patch work in progress (obsolete) — — Splinter Review 
This patch removes use of 'browser.download.defaultFolder'.


And I have a question:
There are 2 different definitions of 'Desktop'. Which is correct one?
http://lxr.mozilla.org/mozilla/source/toolkit/content/contentAreaUtils.js#414
http://lxr.mozilla.org/mozilla/source/toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in#142
Attachment #182560 - Attachment is obsolete: true
Attached patch work in progress (updated) (obsolete) — — Splinter Review 
updated per comment #27.
Attachment #186563 - Attachment is obsolete: true
*** Bug 299042 has been marked as a duplicate of this bug. ***
so, this is probably fairly bitrotted with the recent merging of the xpfe fixes.
 If someone can get this cleaned up/finished, I'll review ASAP.
*** Bug 299079 has been marked as a duplicate of this bug. ***
This patch removes using of 'browser.download.defaultFolder'. Instead of it,
use 'b.d.useDownloadDir' to check auto-downloading or showing filepicker, and
'b.d.folderList' and 'b.d.dir' to get destination directory.
This makes 'contentAreaUtils.js' and 'nsHelperAppDlg.js' containing same
functions, getSpecialFolderKey() and getDownloadsFolder().

And removes some Ben's comments. I remove using of 'b.d.defaultFolder' and add
null check to local variable 'dir' in contentAreaUtils.js.
Attachment #186569 - Attachment is obsolete: true
Attachment #187595 - Flags: review?(mconnor)
(In reply to comment #32)
> Created an attachment (id=187595) [edit]
> updated patch for Bug 294759.

I don't think getting rid of defaultFolder is a good idea. Won't that break
existing profiles? I think that the right fix should follow the comment at:

http://lxr.mozilla.org/aviarybranch/source/toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in#108

Also, this doesn't seem to touch the prefs dialog, which currently sets
browser.download.downloadDir and browser.download.dir. Couldn't this be fixed by
replacing browser.download.downloadDir with browser.download.defaultFolder in
the preferences code? Ben seems to have introduced browser.download.downloadDir
with the prefs rewrite and I think that's the cause of most of these problems.
Hardware: PC → All
Comment on attachment 187595 [details] [diff] [review]
updated patch for Bug 294759.

What's this about? We already have a getDownloadFolder function which should
/just/ check for |useDownloadDir| before it checkes for the defaultfolder, etc.
no?
Ignore my last comment,  I read this wrong.
Flags: blocking1.8b3?
(In reply to comment #33)
> I don't think getting rid of defaultFolder is a good idea. Won't that break
> existing profiles?
No.
Options panels of Firefox 1.0 sets prefs "browser.download.useDownloadDir",
"folderList" and "dir".
http://lxr.mozilla.org/aviarybranch/source/toolkit/mozapps/downloads/content/pref-downloads.xul#58
http://lxr.mozilla.org/aviarybranch/source/toolkit/mozapps/downloads/content/pref-downloads.js#184

When user choosed 'Ask me where to save every file', "defaultFolder" is unset
and "useDownloadDir" is set to 'false'.
When user choosed 'Save all files to this folder', "defaultFolder" is set to the
destination directory and "useDownloadDir" is set to 'true'.
These 2 prefs are set 'paired' (sorry for my bad English... I don't know good
wording).
Current codes check "defaultFolder" is defined or not, my patch replaces them to
check "useDownloadDir".


> Also, this doesn't seem to touch the prefs dialog, which currently sets
> browser.download.downloadDir and browser.download.dir. Couldn't this be fixed by
> replacing browser.download.downloadDir with browser.download.defaultFolder in
> the preferences code?
"downloadDir" is typo of "defaultFolder"?
And unsetting is needed.
Flags: blocking1.8b3? → blocking1.8b3+
Whiteboard: mconnor will review 6/29
Here are some of the results of my research:

Usage of prefs:

browser.download.useDownloadDir
 * Set to true when "Ask me where to save every file" is unchecked, false otherwise
 * This triggers the autodownload feature which uses
browser.download.defaultFolder as the destination folder

browser.download.downloadDir
 * This is useless and should be removed from downloads.xul/downloads.js

browser.download.dir
 * This pref is set whenever we choose a download location for a file in an ask
dialog
 * So this is used to basically track the last download folder (and this is
probably used for the default path of the file picker)

browser.download.defaultFolder
 * This pref is created ONLY when we actually start a download and have
useDownloadDir set to true, and it is set to our default download path
(autodownload path)

Anbo Motohiko's patch probably fixes some inconsistencies in other places, but I
believe that downloads.xul/js should be cleaned aswell.
I've done that, but I have one problem: I need to unset defaultFolder (unsetting
a pref) and I don't know how to do it.
Attached patch is this needed? (obsolete) — — Splinter Review 
This is what I had in mind.. The attached patch will fix this bug and get rid
of the useless downloadDir pref, but it has one issue.

Look at the TODO comment in downloads.js... the code in that part does not
work, and it if it did this whole thing could be resolved, so if anyone has any
tips :)
(In reply to comment #38)
> Created an attachment (id=187689) [edit]
> is this needed?
> 
> This is what I had in mind.. The attached patch will fix this bug and get rid
> of the useless downloadDir pref, but it has one issue.
> 
> Look at the TODO comment in downloads.js... the code in that part does not
> work, and it if it did this whole thing could be resolved, so if anyone has any
> tips :)
You can use the 'hasUserValue' property and 'reset' method.
http://lxr.mozilla.org/mozilla/source/toolkit/content/widgets/preferences.xml#201


> +  onChangeUseDownloadDir: function ()
> +  {
> +    var preference = document.getElementById("browser.download.useDownloadDir");
> +    if (preference) {
This should be:
    if (preference.value) {
this patch fixes all the problems with the previous alternate patch, but it
depends on bug 299162 being fixed to work properly.

You can still hack it and use the preferences service, but IMO the preferences
system should allow you to unset prefs!
Attachment #187689 - Attachment is obsolete: true
*** Bug 295849 has been marked as a duplicate of this bug. ***
Attachment #187595 - Flags: review?(mconnor)
Attachment #187595 - Flags: review+
Attachment #187595 - Flags: approval-aviary1.1a2?
Whiteboard: mconnor will review 6/29 → [bs] large patch has review, smaller patch would be better but depends on bug 299162
Depends on: 299162
Comment on attachment 187595 [details] [diff] [review]
updated patch for Bug 294759.

a=bsmedberg for checkin 6/30 only. Please talk to the QA folk to bang on this
for regressions, since it's large and looks unfortunately regression-prone to
me.
Attachment #187595 - Flags: approval-aviary1.1a2? → approval-aviary1.1a2+
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Verified with Windows and Mac DP builds 0701
Status: RESOLVED → VERIFIED
Hi,

the patches applied for this bug seem to be causing problems with the
"saveURL"-function (from "contentAreaUtils.js") which is used very often by
extensions!

In my very own extension I use the following code-snippet to save a URL by
passing a freshly created file (placed a custom folder!) to the saveURL-function:

>    fileObject.initWithPath(folder);
>    fileObject.append(fname)
>    
>    saveURL(link, fileObject, null, false, true, null);

This procedure always worked fine in previous FF versions... even the first
official "Deer Park Alpha 1" showed no problems.

But the last nightly builds seem to contain some changes making it impossible to
freely decide where the downloaded file should be placed. :-(

"saveURL" doesn't care about the path of the fileObject anymore.
All files passed to "saveURL" get saved to the location set in
Tools->Options->Downloads.

Up to now I don't know exactly why and where the changes are that cause this
behaviour... but I guess it has something to do with this bug.
(I'll investigate as soon as I got more time.)

I think that some native method (which is called by "saveURL") got changed to
solve the bug mentioned here.

I hope this will be changed in future versions... otherwise a lot of
extension-developers (DownSort, ImageToolbar, Lget, Save Image in Folder, etc.)
will have a hard time debugging and/or searching for alternatives.

Best regards,

Achim
Hi,

the patches applied for this bug seem to be causing problems with the
"saveURL"-function (from "contentAreaUtils.js") which is used very often by
extensions!

In my very own extension I use the following code-snippet to save a URL by
passing a freshly created file (placed a custom folder!) to the saveURL-function:

>    fileObject.initWithPath(folder);
>    fileObject.append(fname)
>    
>    saveURL(link, fileObject, null, false, true, null);

This procedure always worked fine in previous FF versions... even the first
official "Deer Park Alpha 1" showed no problems.

But the last nightly builds seem to contain some changes making it impossible to
freely decide where the downloaded file should be placed. :-(

"saveURL" doesn't care about the path of the fileObject anymore.
All files passed to "saveURL" get saved to the location set in
Tools->Options->Downloads.

Up to now I don't know exactly why and where the changes are that cause this
behaviour... but I guess it has something to do with this bug.
(I'll investigate as soon as I got more time.)

I think that some native method (which is called by "saveURL") got changed to
solve the bug mentioned here.

I hope this will be changed in future versions... otherwise a lot of
extension-developers (DownSort, ImageToolbar, Lget, Save Image in Folder, etc.)
will have a hard time debugging and/or searching for alternatives.

Best regards,

Achim

Actually, this isn't the bug that would have affected that, the bug merging two
years of fixes from the Mozilla Suite's version of contentAreaUtils.js is what
would have affected this.  Calling saveURL isn't what you want, since the second
arg is just the filename, not a full local file object.  (And yes, we had to
take these changes, it fixed literally dozens of bugs in saving files.)

Extensions looking to implement their own save functionality can use
internalSave directly, you just need to create an ChosenData object. 
http://lxr.mozilla.org/mozilla/source/toolkit/content/contentAreaUtils.js#435
which you can pass to internalSave with most of the same args as saveURL uses.

Thanks, Mike.
I'll give that one a try.

But another question:
Would that mean you'll have to implement two different save-methods in your 
extension to maintain cross-version compatibility? One for FF-versions < 1.1 
(using the old saveURL()-way) and one for FF-versions >= 1.1 where a 
ChosenData-object can be created to use internalSave().

I checked the "old" contentAreaUtils.js ... you don't have internalSave() 
there (but "saveInternal()") ... and of course you don't have ChosenData().

Well... I could live with that. But it's not very neat. ;-)

Anyway... again... thanks for you fast help/response!
yeah, you would, saveURL has always been a wrapper for the internal method anyway.

relying on chrome methods is always going to be a moving target if you're
implementing your own functionality, of course.  The only way to avoid it is to
implement everything yourself using frozen methods.  But that's no fun! :)
*** Bug 299882 has been marked as a duplicate of this bug. ***
òatch (#187860) has a little issue. The "Save as" dialog cannot be displayed at all if browser.download.dir was unassigned at the pref.js . Reason:

spec=/firefox
WARNING: malformed url: no scheme, file nsStandardURL.cpp, line 713
JavaScript error: , line 0: uncaught exception: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getCom
plexValue]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: chrome://global/content/contentAreaUtils.js :: getTargetFile :: line
 485"  data: no]

This patch fix this problem.
(In reply to comment #51)
> Created an attachment (id=206494) [edit]
> 187860 patch addon
You should open a new bug.
sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs,
filter on "beltznerLovesGoats" to get rid of this mass change
QA Contact: mconnor → preferences
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: