GNOME Bugzilla – Bug 712302
GtkPlacesSideBar showing "Desktop" or not shouldn't be a by-application setting
Last modified: 2013-11-26 19:32:51 UTC
Currently 'whether the placebar should show a "Desktop" entry or not' is a "by application option", that seems wrong. GNOME3 doesn't use a Desktop but most other Linux desktops and outside platforms do... Reference: https://git.gnome.org/browse/gtk+/tree/gtk/gtkplacessidebar.c#n4515 " * Sets whether the @sidebar should show an item for the Desktop folder; this is off by default. * An application may want to turn this on if the desktop environment actually supports the * notion of a desktop." How is an application supposed to query if the current desktop has a notion of "desktop"? It also means every software need to be patched to have that logic, which seems wrong... The change was made in that commit https://git.gnome.org/browse/gtk+/commit/?h=places-sidebar&id=28f44a3b715c1287117222cd166fa8ee0fbb1732 Federico wrote as commit message "Function to set whether to show the Desktop item It sucks to have this as an app-settable option. Maybe we should make this a standard GSetting thing and be done with it." That would be better since it would let vendors set a different default, it wouldn't address the case of a distribution shipping different desktops though (or we would need a way to set different gsettings default by desktop environment, e.g turn that key on for GNOME3 and off for e.G Unity or XFCE) Thoughs?
I think it makes quite a lot of sense to tie this to org.gnome.desktop.background show-desktop-icons. Only problem with that is that this setting is in gsettings-desktop-scheams, on which Gtk does not depend. We're probably going to have to deal with this larger issue as we start using GSettings for more of the functionalities that we're pulling out of XSETTINGS...
Well, gsettings doesn't really make it easy to do what we need. Let's say - $user installs Debian with GNOME3, gets a default schemas that says "show-desktop=false" - after some time the user feels like trying XFCE and install that ... what should happen? * XFCE has no "Desktop" entry and requires the user to manually go change the configuration? or * the XFCE packages pull in a gsettings override that change that key (what happens to the GNOME3 session then, does it get buggy in the reverse way?) or * <better solution ... suggestion? ;-)>
Created attachment 259824 [details] [review] Add a GtkSetting for 'shell-shows-desktop' Add a GtkSetting for whether the desktop shell is showing the desktop folder icons. We use this to determine if we should also show the "Desktop" folder in the places sidebar. This is on by default because most desktop shells do show the icons on the desktop. We will be landing a patch in gnome-settings-daemon to bind this to the org.gnome.desktop.background show-desktop-icons GSettings key which is off by default on GNOME.
Created attachment 259825 [details] [review] xsettings: export Gtk/ShellShowsDesktop setting Export a property for whether the desktop icons are shown or not. This will allow Gtk to decide if we should show the 'Desktop' item in the places sidebar, according to if the user has enabled their desktop icons or not. We use the value of the GSettings key 'show-desktop-icons' from 'org.gnome.desktop.background'.
Review of attachment 259825 [details] [review]: Looks good.
Review of attachment 259824 [details] [review]: The gtkplacessidebar.c patch doesn't look at whether ~/Desktop is empty or not. I'd do those changes separately.
(In reply to comment #6) > Review of attachment 259824 [details] [review]: > > The gtkplacessidebar.c patch doesn't look at whether ~/Desktop is empty or not. > I'd do those changes separately. Agreed. They're non-trivial because we'd need to setup a file monitor for it. I'm still not sure if we want them and it's definitely a separate issue.
Comment on attachment 259825 [details] [review] xsettings: export Gtk/ShellShowsDesktop setting Attachment 259825 [details] pushed as 3bd705b - xsettings: export Gtk/ShellShowsDesktop setting
Review of attachment 259824 [details] [review]: Looks ok for what it does, but I'd commit this in two pieces - first commit adds the setting, second commit makes gtkplacessidebar use it
Will do.
Created attachment 259839 [details] [review] Add a GtkSetting for 'shell-shows-desktop' Add a GtkSetting for whether the desktop shell is showing the desktop folder icons. This is on by default because most desktop shells do show the icons on the desktop. We already have a patch in gnome-settings-daemon to bind this to the org.gnome.desktop.background show-desktop-icons GSettings key which is off by default on GNOME.
Created attachment 259840 [details] [review] GtkPlacesSidebar: use shell-shows-desktop setting Use the just-added shell-shows-desktop GtkSetting to determine if we should also show the "Desktop" folder in the places sidebar.
Attachment 259839 [details] pushed as a90bb7d - Add a GtkSetting for 'shell-shows-desktop' Attachment 259840 [details] pushed as c4141a2 - GtkPlacesSidebar: use shell-shows-desktop setting I'd like to backport this to the respective 3.10 branches. Is everyone OK with that?
15:05 < desrt> this violates my normal rule of 'no user visible changes for backports' but we might make an exception because it restores the behaviour to what it was before 15:05 <@mclasen_> I've already bent the rule for the places sidebar - added the local-only property 15:06 <@mclasen_> there were just too many regressions in the filechooser otherwise 15:06 <@mclasen_> so I'm ok with it Backported to gtk-3-10 and gnome-3-10 branches.
Bug 712334 for the "show ~/Desktop if it's non-empty" thing.
*** Bug 711040 has been marked as a duplicate of this bug. ***
Created attachment 259880 [details] [review] Change the default for "show-desktop" back to TRUE Change the GtkSettings default for "shell-shows-desktop" back to TRUE and also change the default value of the "show-desktop" property on GtkPlacesSidebar so that the defaultvalue test passes.
I think we really should try to take an approach of functionality changes being on an opt-in basis. As such, if the user has no XSETTINGS provider (or has one that doesn't set the until-yesterday-nonexistent Gtk/ShellShowsDesktop setting), let's try to keep our old behaviour. This way, GNOME can get the experience that it wants (since it has such an XSETTINGS provider) and we don't get the hate on our mailing lists. fwiw, Ubuntu is shipping an XSETTINGS provider that sets this property, so this is of no consequence to us. My concern is about not regressing the behaviour of Gtk on other desktops (KDE, XFCE, etc). I really think we should be trying to do better here -- particularly because it is so easy.
Created attachment 259898 [details] [review] defaultvalue test: ignore show-desktop Ignore the "show-desktop" property on GtkPlacesSidebar for the defaultvalue test. Currently, "make check" is passing because it runs the test under a xvfb with no XSETTINGS provider, so we see the Gtk default value. No matter what we set the default value to in Gtk, however, there will be some desktop environment in which someone running the installed test outside of an xvfb will get the wrong result. Best to ignore it.
First of all thanks for the patches. I had to set "gtk-shell-shows-desktop" manually for Xfce with GTK+ 3.10.4. But at least this is configurable now. One problem though is the different icon for the desktop entry. It's the only coloured one. Probably gnome-themes-standard does not contain a grey icon.
Review of attachment 259880 [details] [review]: Why do you remove the quartz and win32 settings ? I think it is a good practice to make the backends set the platform settings explicitly, so I'd like to keep that code.
Review of attachment 259898 [details] [review]: With this argument, we should probably exclude all the platform settings here. At the very least, that would include all the 'shell shows' settings.
(In reply to comment #21) > Why do you remove the quartz and win32 settings ? I think it is a good practice > to make the backends set the platform settings explicitly, so I'd like to keep > that code. They don't need to be explicitly set because TRUE is already the default value of the property. settings_install_property_parser consults the default value in the paramspec and sets it as the default for the created settings object -- no need to explicitly override it in the constructor. (In reply to comment #22) > With this argument, we should probably exclude all the platform settings here. > At the very least, that would include all the 'shell shows' settings. Because the shell-shows-appmenu/menubar settings aren't wired through to any widgets in particular -- only to GtkSettings itself which has a blanket exception in the test.
(In reply to comment #23) > (In reply to comment #21) > > Why do you remove the quartz and win32 settings ? I think it is a good practice > > to make the backends set the platform settings explicitly, so I'd like to keep > > that code. > > They don't need to be explicitly set because TRUE is already the default value > of the property. settings_install_property_parser consults the default value > in the paramspec and sets it as the default for the created settings object -- > no need to explicitly override it in the constructor. > I know that. I still like to keep them explicit. > (In reply to comment #22) > > With this argument, we should probably exclude all the platform settings here. > > At the very least, that would include all the 'shell shows' settings. > > Because the shell-shows-appmenu/menubar settings aren't wired through to any > widgets in particular -- only to GtkSettings itself which has a blanket > exception in the test. Ah, indeed. Ignore that comment then
Attachment 259880 [details] pushed as 2111407 - Change the default for "show-desktop" back to TRUE Attachment 259898 [details] pushed as ce2d9b4 - defaultvalue test: ignore show-desktop