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 613127 - [MetaScreen] Keep num_workspaces key in sync with the actual workspace number
[MetaScreen] Keep num_workspaces key in sync with the actual workspace number
Status: RESOLVED FIXED
Product: mutter
Classification: Core
Component: general
git master
Other Linux
: Normal normal
: ---
Assigned To: mutter-maint
mutter-maint
Depends on:
Blocks:
 
 
Reported: 2010-03-17 10:07 UTC by Tomas Frydrych
Modified: 2010-05-14 11:34 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch to keep prefernces key in sync with actual workspace count (2.53 KB, patch)
2010-03-17 10:07 UTC, Tomas Frydrych
none Details | Review
updated patch for _MUTTER_HINTS (6.67 KB, patch)
2010-04-13 08:40 UTC, Tomas Frydrych
none Details | Review

Description Tomas Frydrych 2010-03-17 10:07:28 UTC
Created attachment 156342 [details] [review]
patch to keep prefernces key in sync with actual workspace count

Changing the number of workspaces via an external pager relies on the gconf
key; if a plugin adds or removes workspaces on the fly, we can get into a
situation when the stale number stored by the preferences matches the new
number requested by the pager, in which case the pager request becomes a nop.

This commit ensures that when the meta_screen_append_new_workspace() or
meta_screen_remove_workspace() functions are called, the stored value is
updated accordingly.
Comment 1 Dan Winship 2010-03-17 14:07:21 UTC
we'd find this slightly annoying since we treat the number of workspaces as being a transient/per-session thing. but we already have to have code to remove the excess workspaces at startup since the gconf default is "4" anyway, so...
Comment 2 Tomas Frydrych 2010-03-17 14:25:12 UTC
The other option here is to decouple the handling of the _NET_NUMBER_OF_DESKTOPS property form the gconf value, and only use the latter to as the initial value. That would also fix the problem of not being able to create an additional workspace.
Comment 3 Owen Taylor 2010-04-12 18:15:12 UTC
Review of attachment 156342 [details] [review]:

Hmm, the problem with this versus gnome-shell is that if you start gnome-shell you are going to lose all your workspaces when you go back to normal GNOME.

One thing I've considered - for themes, button positions, etc, is that we should have a generic way of overriding Metacity preferences from a plugin. So redirecting and saying "for num_workspaces, don't use /apps/metacity/general/num_workspaces, use /apps/gnome-shell/num_workspaces."

I'm not sure how hard that's going to be to implement (here it has the problem too that we probably read the preferences before we initialize the plugins. After all, the plugins to use are one of the GConf keys, though that's a bit busted from the point of view of the way we conceive of plugins now.) I guess I should look into it some.

Leaving that aside, this patch makes a lot of sense to me. It doesn't make sense to sometimes sync the number of workspace to GConf and sometimes not. Arguably it's a bit messy that:

 - When the number of workspaces changes via client message, we adjust the GConf key and then wait for the notification to adjust the workspace count
 - When the number of workspaces changes otherwise, we adjust the workspace count, adjust the GConf key, and count on short-circuiting

We can't make both work like the client message (since we want to do things like add a workspace, then put a window on it immediately). We could switch the property-change code path, but likely not worth it. One minor style comment, otherwise looks good to me, with the big caveat about it messing things up for GNOME Shell testers.

Having the GConf key only be set initially will break the idea that Mutter is a drop-in for Metacity; it means that Mutter no longer works with an external pager in the way that window managers are supposed to in GNOME 2. I'm not really that concerned about keeping the ability to use Mutter with GNOME 2, but it's sort of nice that it does work currently.

::: src/core/screen.c
@@ +1268,3 @@
   GList         *next = NULL;
   int index;
+  gint           new_num;

Shouldn't mix int and gint in successive lines. (Metacity/Mutter is pretty consistent about not using gint and the like, there are some mistakes but it's only a small fraction. I certainly approve of the avoidance of gint - people should realize that glib and gtk+ only use gint because of historical reasons.) More examples of gint usage below.
Comment 4 Owen Taylor 2010-04-12 21:57:59 UTC
GConf key redirection implemented in bug 615586
Comment 5 Tomas Frydrych 2010-04-13 08:40:41 UTC
Created attachment 158579 [details] [review]
updated patch for _MUTTER_HINTS

Update patch based on the review attached.
Comment 6 Tomas Frydrych 2010-05-14 11:24:29 UTC
Comment on attachment 158579 [details] [review]
updated patch for _MUTTER_HINTS

Sorry, that patch was meant for bug 613123.
Comment 7 Tomas Frydrych 2010-05-14 11:34:04 UTC
Patch pushed as fc9488211f689e7f8802345d753d70e5f5cd4025.