After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 712302 - GtkPlacesSideBar showing "Desktop" or not shouldn't be a by-application setting
GtkPlacesSideBar showing "Desktop" or not shouldn't be a by-application setting
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Widget: GtkFileChooser
3.11.x
Other Linux
: Normal normal
: ---
Assigned To: gtk-bugs
Federico Mena Quintero
: 711040 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2013-11-14 14:36 UTC by Sebastien Bacher
Modified: 2013-11-26 19:32 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add a GtkSetting for 'shell-shows-desktop' (7.85 KB, patch)
2013-11-14 16:55 UTC, Allison Karlitskaya (desrt)
reviewed Details | Review
xsettings: export Gtk/ShellShowsDesktop setting (2.83 KB, patch)
2013-11-14 16:56 UTC, Allison Karlitskaya (desrt)
committed Details | Review
Add a GtkSetting for 'shell-shows-desktop' (3.70 KB, patch)
2013-11-14 20:03 UTC, Allison Karlitskaya (desrt)
committed Details | Review
GtkPlacesSidebar: use shell-shows-desktop setting (4.49 KB, patch)
2013-11-14 20:03 UTC, Allison Karlitskaya (desrt)
committed Details | Review
Change the default for "show-desktop" back to TRUE (3.23 KB, patch)
2013-11-15 14:21 UTC, Allison Karlitskaya (desrt)
committed Details | Review
defaultvalue test: ignore show-desktop (1.34 KB, patch)
2013-11-15 14:35 UTC, Allison Karlitskaya (desrt)
committed Details | Review

Description Sebastien Bacher 2013-11-14 14:36: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?
Comment 1 Allison Karlitskaya (desrt) 2013-11-14 14:48:58 UTC
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...
Comment 2 Sebastien Bacher 2013-11-14 14:55:13 UTC
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? ;-)>
Comment 3 Allison Karlitskaya (desrt) 2013-11-14 16:55:59 UTC
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.
Comment 4 Allison Karlitskaya (desrt) 2013-11-14 16:56:04 UTC
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'.
Comment 5 Bastien Nocera 2013-11-14 17:00:45 UTC
Review of attachment 259825 [details] [review]:

Looks good.
Comment 6 Bastien Nocera 2013-11-14 17:02:35 UTC
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.
Comment 7 Allison Karlitskaya (desrt) 2013-11-14 17:09:11 UTC
(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 8 Allison Karlitskaya (desrt) 2013-11-14 17:09:53 UTC
Comment on attachment 259825 [details] [review]
xsettings: export Gtk/ShellShowsDesktop setting

Attachment 259825 [details] pushed as 3bd705b - xsettings: export Gtk/ShellShowsDesktop setting
Comment 9 Matthias Clasen 2013-11-14 19:59:49 UTC
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
Comment 10 Allison Karlitskaya (desrt) 2013-11-14 20:00:43 UTC
Will do.
Comment 11 Allison Karlitskaya (desrt) 2013-11-14 20:03:21 UTC
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.
Comment 12 Allison Karlitskaya (desrt) 2013-11-14 20:03:26 UTC
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.
Comment 13 Allison Karlitskaya (desrt) 2013-11-14 20:04:12 UTC
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?
Comment 14 Allison Karlitskaya (desrt) 2013-11-14 20:08:44 UTC
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.
Comment 15 Allison Karlitskaya (desrt) 2013-11-14 20:27:40 UTC
Bug 712334 for the "show ~/Desktop if it's non-empty" thing.
Comment 16 Allison Karlitskaya (desrt) 2013-11-14 20:28:00 UTC
*** Bug 711040 has been marked as a duplicate of this bug. ***
Comment 17 Allison Karlitskaya (desrt) 2013-11-15 14:21:12 UTC
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.
Comment 18 Allison Karlitskaya (desrt) 2013-11-15 14:25:19 UTC
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.
Comment 19 Allison Karlitskaya (desrt) 2013-11-15 14:35:08 UTC
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.
Comment 20 Thomas Lange 2013-11-16 15:23:00 UTC
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.
Comment 21 Matthias Clasen 2013-11-23 04:27:02 UTC
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.
Comment 22 Matthias Clasen 2013-11-23 04:29:00 UTC
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.
Comment 23 Allison Karlitskaya (desrt) 2013-11-23 04:51:46 UTC
(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.
Comment 24 Matthias Clasen 2013-11-24 15:20:18 UTC
(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
Comment 25 Matthias Clasen 2013-11-26 19:32:44 UTC
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