399500 - support XDG user dirs in the directory service

Status: RESOLVED FIXED

Description

[Christian Persch (GNOME) (away; not receiving bug mail)]

The XDG user dirs (see URL) are used by GNOME to find, and allow the user to change, common directories with localised names. To enable using the Download directory in the toolkit's download manager (bug 399498), the directory service needs to support these directories.

Comment 1

[Christian Persch (GNOME) (away; not receiving bug mail)]

Created attachment 284519 [details] [diff] [review]
proposed implementation. The included xdg_user_dir_lookup code is provided by freedesktop.org under a BSD-style licence which is compatible with mozilla's tri-licence.

Comment 2

[Christian Persch (GNOME) (away; not receiving bug mail)]

Comment on attachment 284519 [details] [diff] [review]
proposed implementation. The included xdg_user_dir_lookup code is provided by freedesktop.org under a BSD-style licence which is compatible with mozilla's tri-licence.

I guess it may be too late for 1.9 for this; but asking doesn't cost anything :)

Comment 3

[Christian Persch (GNOME) (away; not receiving bug mail)]

Created attachment 286149 [details] [diff] [review]
updated patch, to also provide the NS_UNIX_DEFAULT_DOWNLOAD_DIR key

Comment 4

[Damon Sicore (:damons)]

Won't block on this, but if reviewed, ask for approval.

Comment 5

[Rimas Kudelis]

Christian, the patch doesn't apply cleanly anymore. Mind to update it (and also the one on Bug 399498)?

Comment 6

[Christian Persch (GNOME) (away; not receiving bug mail)]

How does it fail? Probably just because surrounding lines have changed. I could re-diff, but I have to edit out unrelated changed in my tree, so it's not a zero-cost operation. I would do it if there was any chance this patch could get review and land in 1.9, but that doesn't seem likely.

Comment 7

[Rimas Kudelis]

(In reply to comment #6)
> How does it fail? Probably just because surrounding lines have changed.

Probably.

> I could re-diff, but I have to edit out unrelated changed in my tree, so it's
> not a zero-cost operation. I would do it if there was any chance this patch
> could get review and land in 1.9, but that doesn't seem likely.

Well, as long as it doesn't apply, this seems even more unlikely... OTOH, if that's too much of a hassle for you, OK then...

Comment 8

[Christopher Aillon (sabbatical, not receiving bugmail)]

It doesn't need to apply for someone to provide comments on the patch.

For example:
chpe, does it really make sense for us to copy the xdg_user_dir_lookup function?  glib 2.14 added g_get_user_special_dir () and I don't believe there are any systems below that which would actually have translated directories, so we can simply return the untranslated ones without the need for the extra code if not on glib 2.14.  Also, do we really need to care about things other than Desktop?

Comment 9

[Benjamin Smedberg]

Comment on attachment 286149 [details] [diff] [review]
updated patch, to also provide the NS_UNIX_DEFAULT_DOWNLOAD_DIR key

I'll delegate review to caillon, who seems to know more about it than I.

I do worry that we're adding a bunch of XDG-specific directory keys, when we should just have a single "downloads directory", "desktop directory",  "music directory" key to which each platform responds with the platform download directory.

Comment 10

[Christian Persch (GNOME) (away; not receiving bug mail)]

(In reply to comment #8)
> chpe, does it really make sense for us to copy the xdg_user_dir_lookup
> function?  glib 2.14 added g_get_user_special_dir () and I don't believe there
> are any systems below that which would actually have translated directories, so
> we can simply return the untranslated ones without the need for the extra code
> if not on glib 2.14.  Also, do we really need to care about things other than
> Desktop?

I didn't use the glib functions since I wasn't sure a hard dep on glib >= 2.14 was acceptable, and I dislike the dlsym hackery used otherwise in these cases to load the symbols at runtime... 

(In reply to comment #9)
> (From update of attachment 286149 [details] [diff] [review])
> I do worry that we're adding a bunch of XDG-specific directory keys, when we
> should just have a single "downloads directory", "desktop directory",  "music
> directory" key to which each platform responds with the platform download
> directory.

I see a bunch of OSX specific keys in nsDirectoryServiceDefs.h that are similar, (NS_OSX_PICTURE_DOCUMENTS_DIR, NS_OSX_MOVIE_DOCUMENTS_DIR, NS_OSX_MUSIC_DOCUMENTS_DIR), but we don't seem to have these for windows, so I added them in the NS_UNIX_XDG_ namespace. Also the one I started with this work for, the downloads folder, is defined separately for each major platform too...

Comment 11

[Rimas Kudelis]

Hm, I just marked bug 259356 as dependent on this one, but I'm not sure about it anymore. Feel free to remove it from dependent bugs list if that's not the case.

Comment 12

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 286149 [details] [diff] [review]
updated patch, to also provide the NS_UNIX_DEFAULT_DOWNLOAD_DIR key

r=caillon.  The code is good, but bsmedberg gets the last word on the naming.

Comment 14

[Reed Loden [:reed] (use needinfo?)]

Comment on attachment 286149 [details] [diff] [review]
updated patch, to also provide the NS_UNIX_DEFAULT_DOWNLOAD_DIR key

This fixes the core backend to allow for parity with Mac and Windows on automatically selecting the proper directory for downloads. Bug 399498 will use this backend code to actually support selecting the proper directory.

Comment 15

[Damon Sicore (:damons)]

Comment on attachment 286149 [details] [diff] [review]
updated patch, to also provide the NS_UNIX_DEFAULT_DOWNLOAD_DIR key

a1.9+=damons

Comment 16

[Reed Loden [:reed] (use needinfo?)]

Checking in xpcom/io/SpecialSystemDirectory.cpp;
/cvsroot/mozilla/xpcom/io/SpecialSystemDirectory.cpp,v  <--  SpecialSystemDirectory.cpp
new revision: 1.39; previous revision: 1.38
done
Checking in xpcom/io/SpecialSystemDirectory.h;
/cvsroot/mozilla/xpcom/io/SpecialSystemDirectory.h,v  <--  SpecialSystemDirectory.h
new revision: 1.10; previous revision: 1.9
done
Checking in xpcom/io/nsDirectoryService.cpp;
/cvsroot/mozilla/xpcom/io/nsDirectoryService.cpp,v  <--  nsDirectoryService.cpp
new revision: 1.95; previous revision: 1.94
done
Checking in xpcom/io/nsDirectoryService.h;
/cvsroot/mozilla/xpcom/io/nsDirectoryService.h,v  <--  nsDirectoryService.h
new revision: 1.35; previous revision: 1.34
done
Checking in xpcom/io/nsDirectoryServiceDefs.h;
/cvsroot/mozilla/xpcom/io/nsDirectoryServiceDefs.h,v  <--  nsDirectoryServiceDefs.h
new revision: 1.31; previous revision: 1.30
done

Comment 17

[Reed Loden [:reed] (use needinfo?)]

gerv, do we need to update about:license to include the license concerning the xdg_user_dir_lookup function?

Comment 18

[Gervase Markham [:gerv]]

Reed: Yes. Please file a bug to that effect.

Gerv

Comment 19

[Rimas Kudelis]

Should I also verify this if I have just verified bug 399498?