GNOME Bugzilla – Bug 434813
only display GDM desktop items if using GDM
Last modified: 2007-05-14 03:30:49 UTC
Recently I wrote the attached patch which installs the GDM desktop files for gdmsetup, gdmflexiserver, gdmphotosetup, etc. to /usr/share/gdm/applications instead of to /usr/share/applications. Then GDM sets the XDG_DATA_DIR environment variable to add this directory so that these programs are only available when using GDM. This makes things a bit nicer when the user may be using KDM or CDE login or some other display manager, but might also have GDM installed on the system. The GDM programs are obviously non-functional if GDM isn't running, so its a bit better to not display them. That said, I'm not sure if this is a patch that is of general use or is only useful to us at Sun (where CDE login is still the default login program). So I'll provide my patch for revie and if I get a thumbs up from people, then I'll go ahead and put it in the code. If not, we can close this bug as WILLNOTFIX.
Created attachment 87307 [details] [review] patch adding this feature
Obviously the above patch doesn't actually change the install locations of the desktop files, but adds the code to daemon/slave.c to set the environment variable. If this patch gets approved, then I'll also add the Makefile.am code to change the install location of these files.
One concern might be...what if someone sets XDG_DATA_DIRS in their .profile? It's probably a configuration bug if the user doesn't append to instead of replace XDG_DATA_DIRS though. As far as the patch goes, it seems a little weird to iterate over the system dirs array and rejoin the string rather than just appending to the already joined string provided by g_getenv ("XDG_DATA_DIRS"). I guess if you went that route you'd have to provide the fallback /usr/local/share/:/usr/share/ when XDG_DATA_DIRS isn't set. So it would be something like: const char *old_system_data_dirs; char *new_system_data_dirs; old_system_data_dirs = g_getenv ("XDG_DATA_DIRS")? g_getenv ("XDG_DATA_DIRS") : "/usr/local/share:/usr/share/"; new_system_data_dirs = g_build_path (":", old_system_data_dirs, DATADIR "/gdm", NULL); g_setenv ("XDG_DATA_DIRS", new_system_data_dirs, TRUE); g_free (new_system_data_dirs);
Ray, a few comments: - If a user sets XDG_DATA_DIRS improperly by resetting it instead of appending to it, then this is a user problem. The problem would only cause GDM desktop items to not appear in their application menu, which probably isn't the end of the world. The user could always add the right directories back if they want the menu items. - Note that g_get_system_data_dirs just returns whatever is in the XDG_DATA_DIRS environment variable. Since glib provides a function for accessing this, it seems smart to use it. Though if people think just appending to the environment variable is better, I can modify the patch to work that way.
I definitely agree with your first point. I know what g_get_system_data_dirs() does (I actually wrote the function). I'm just saying that it's a bit weird to loop over and rejoin every element of the array when it's already in the format you care about by doing g_getenv ("XDG_DATA_DIRS"). It doesn't really matter either way though, it's just an implementation detail.
Okay, so do people think this patch is a good idea to integrate?
I haven't gotten any feedback yet about whether people think this is a good feature to add to GDM. Please provide some feedback.
I think it seems reasonable.
Thanks, Ray, for the input. I went ahead and used your code snippit instead of the one in my patch since I think you are right - it is better to do things this way. I did modify your snippit a bit to make sure to always append the "/" to the end of each directory in the path, which is more like how the path is set by default. old_system_data_dirs = g_getenv ("XDG_DATA_DIRS") ? g_getenv ("XDG_DATA_DIRS") : "/usr/local/share/:/usr/share/"; new_system_data_dirs = g_build_path (":", old_system_data_dirs, DATADIR "/gdm/", NULL); Marking as fixed.