use XDG downloads dir as default downloads dir

VERIFIED FIXED in mozilla1.9beta5

Status

()

--
enhancement
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: chpe, Assigned: chpe)

Tracking

Trunk
mozilla1.9beta5
x86
Linux
Points:
---
Bug Flags:
blocking1.9 -
wanted1.9 +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

Created attachment 284517 [details] [diff] [review]
proposed patch: use XDG download dir, with a fallback to the current default if the XDG user dirs are disabled

nsDownloadManager::GetDefaultDownloadsDirectory currently uses $HOME/Downloads (localised string) on linux. The XDG user directories (see URL) allow the user to customise the directory name, while still providing a localised name by default, and to maintain the directory across locale changes.

To integrate better with GNOME (and other DEs too, since it's a freedesktop.org spec) which has adopted this spec, the download manager should use the XDG Download directory as its default download directory (used when browser.download.folderList = 1).

(Note that this is not a dup of bug 259356 since that bug is about following the XDG base dir spec for the profile location etc.)
Assignee: nobody → chpe
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment on attachment 284517 [details] [diff] [review]
proposed patch: use XDG download dir, with a fallback to the current default if the XDG user dirs are disabled

This depends on the patch in bug 399500 which adds that key to the dir service; if that doesn't get into 1.9 I'll have to come up with another approach here; maybe define a NS_UNIX_DEFAULT_DOWNLOAD_DIR key and query that before falling back to $HOME/Downloads (so I can provide the correct dir in my own directory provider).
Attachment #284517 - Flags: review?(comrade693+bmo)
Comment on attachment 284517 [details] [diff] [review]
proposed patch: use XDG download dir, with a fallback to the current default if the XDG user dirs are disabled

>+#if defined(XP_UNIX)
>+  rv = dirService->Get(NS_UNIX_XDG_DOWNLOAD_DIR,
>+                       NS_GET_IID(nsILocalFile),
>+                       getter_AddRefs(downloadDir));
>+  // fallback to Home/Downloads
>+  if (NS_FAILED(rv)) {
I'm inclined to just copy/paste this code here, instead of doing this ifdef stuff because it's not immediately clear as to what's going on.  I'd rather have easy to read code than save a few bytes of source file size.
Attachment #284517 - Flags: review?(comrade693+bmo) → review-
Created attachment 286148 [details] [diff] [review]
updated patch

This just copies the code instead of the #ifdef, as requested by the reviewer.

Also I changed the dir service key to add NS_UNIX_DEFAULT_DOWNLOAD_DIR instead of using the XDG key directly (there's precedent for this in the OSX and WIN DEFAULT_DOWNLOAD keys); this is because I don't have much hope that bug 399500 will make it into 1.9, but I still need this way to provide the right dir from my own dir service provider.
Attachment #284517 - Attachment is obsolete: true
Attachment #286148 - Flags: review?(comrade693+bmo)
Comment on attachment 286148 [details] [diff] [review]
updated patch

Requesting additional review from bsmedberg for the addition of the NS_UNIX_DEFAULT_DOWNLOAD_DIR define in nsDirectoryServiceDefs.h .
Attachment #286148 - Attachment is patch: true
Attachment #286148 - Attachment mime type: application/octet-stream → text/plain
Attachment #286148 - Flags: review?(benjamin)
Comment on attachment 286148 [details] [diff] [review]
updated patch

>+  // fallback to Home/Downloads
>+  if (NS_FAILED(rv)) {
>+    rv = dirService->Get(NS_OS_HOME_DIR,
NS_UNIX_HOME_DIR?  They may point to the same, but I think it makes more sense here

>+                        NS_GET_IID(nsILocalFile),
>+                        getter_AddRefs(downloadDir));
nit: +1 space on the start of both of these lines

r=sdwilsh with these fixed.
Attachment #286148 - Flags: review?(comrade693+bmo) → review+

Comment 6

11 years ago
I hope this bugs gets fixed before the release of Fx 3.0, because...

Currently, Firefox seems to default to "~/Desktop" as a download dir, however it seems like GNOME is now localizing the name of that folder, so just "~/Desktop" no longer "just works". I'm not sure, this *COULD* be a wrong translation in lt, but it seems a little odd when Firefox tells it downloads stuff to Desktop, but downloads it to ~ instead.
This patch is just putting the infrastructure in place for that; actually using the XDG dir is bug 399500.

The patch is waiting for r/sr on the addition of the #define in xpcom/; afterwards I'll post a patch with the nits from comment 5 adressed.
Requesting blocking per comment 6
Flags: blocking-firefox3?
Nice to have, but not going to block on it if we don't get the support landed.  There's far too much other stuff we want to get into users' hands.
Flags: wanted-firefox3+
Flags: blocking-firefox3?
Flags: blocking-firefox3-

Updated

11 years ago
Attachment #286148 - Flags: review?(benjamin) → review+
Created attachment 308851 [details] [diff] [review]
patch for check-in

Combined with the patch from bug 399500, this completes parity of all three platforms of choosing the correct downloads directory. Without these two patches, problems like what comment #6 mentions are likely to happen on non-English Linux installs, which will greatly confuse users.
Attachment #286148 - Attachment is obsolete: true
Attachment #308851 - Flags: approval1.9?
Comment on attachment 308851 [details] [diff] [review]
patch for check-in

a1.9+=damons

Do I have to point out that time was spent on a non-blocker here?  /me looks to see if there are other blockers that sdwilsh could look at?  :)
Attachment #308851 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
(In reply to comment #11)
> Do I have to point out that time was spent on a non-blocker here?  /me looks to
> see if there are other blockers that sdwilsh could look at?  :)

Do note that sdwilsh gave his r+ on 2007-10-26 08:48:25 PDT, so the blockers-only speech isn't as useful here. :)
Checking in toolkit/components/downloads/src/nsDownloadManager.cpp;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp,v  <--  nsDownloadManager.cpp
new revision: 1.171; previous revision: 1.170
done
Checking in xpcom/io/nsDirectoryServiceDefs.h;
/cvsroot/mozilla/xpcom/io/nsDirectoryServiceDefs.h,v  <--  nsDirectoryServiceDefs.h
new revision: 1.32; previous revision: 1.31
done
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 beta5

Comment 14

11 years ago
Verifying fixed with Lithuanian XDG settings (GNOME) and the following build:

Mozilla/5.0 (X11; U; Linux i686; lt; rv:1.9b5pre) Gecko/2008031304 Minefield/3.0b5pre
Status: RESOLVED → VERIFIED
Product: Firefox → Toolkit

Comment 15

10 years ago
l18n problem :

i'm playing with Firefox 3.5~b4 within Ubuntu 9.04 Jaunty beta and my XDG_DOWNLOAD is automatically chosen by Firefox to store files that i've downloaded which is great (and might be new in 3.5 version since i didn't noticed that before)

But although my user-dirs.dirs file says :
XDG_DOWNLOAD_DIR="$HOME/Téléchargements"
Firefox 3.5 offers me the choice to "Save files to Download" whereas it should say "Save files to Téléchargements" in my case (which is the l18n version of "Download" set in the user-dirs.dirs file)
File a new bug please.  Commenting in old resolved bugs is a surefire way to have that comment ignored.

Comment 17

10 years ago
ok, that's Bug 488588
You need to log in before you can comment on or make changes to this bug.