Closed
Bug 399498
Opened 17 years ago
Closed 17 years ago
use XDG downloads dir as default downloads dir
Categories
(Toolkit :: Downloads API, enhancement)
Tracking
()
VERIFIED
FIXED
mozilla1.9beta5
People
(Reporter: chpe, Assigned: chpe)
References
()
Details
Attachments
(1 file, 2 obsolete files)
3.35 KB,
patch
|
damons
:
approval1.9+
|
Details | Diff | Splinter Review |
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.)
Updated•17 years ago
|
Assignee: nobody → chpe
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 1•17 years ago
|
||
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 2•17 years ago
|
||
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-
Assignee | ||
Comment 3•17 years ago
|
||
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)
Assignee | ||
Comment 4•17 years ago
|
||
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 5•17 years ago
|
||
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•17 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.
Assignee | ||
Comment 7•17 years ago
|
||
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.
Comment 9•17 years ago
|
||
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•17 years ago
|
Attachment #286148 -
Flags: review?(benjamin) → review+
Comment 10•17 years ago
|
||
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 11•17 years ago
|
||
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+
Updated•17 years ago
|
Keywords: checkin-needed
Comment 12•17 years ago
|
||
(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. :)
Comment 13•17 years ago
|
||
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
Closed: 17 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 beta5
Comment 14•17 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
Updated•16 years ago
|
Product: Firefox → Toolkit
Comment 15•16 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)
Comment 16•16 years ago
|
||
File a new bug please. Commenting in old resolved bugs is a surefire way to have that comment ignored.
Comment 17•16 years ago
|
||
ok, that's Bug 488588
You need to log in
before you can comment on or make changes to this bug.
Description
•