This seems to have been broken by commit 2d57b1b4703eb660aa398dfd84b52119f1e58ab4 in mutter.

Reverting it fixes the issue.

Probably add the binding to the mutter schema for now; I can't think of a reason why that wouldn't work.

Created attachment 153731 [details] [review]
Add recording keybinding to gconf schema

The recording keybinding is not part of the gconf schemas because it is not
supported by metacity (which shares the keys with mutter).

Created attachment 153736 [details] [review]
Resurrect schema-bindings.c

This is the source of a small helper program to generate gconf schemas for
key bindings. As mutter uses the schemas provided by metacity, this file
was deleted when the project forked.
Get the file back from the current metacity tree to generate schemas for
mutter-only key bindings.

Created attachment 153737 [details] [review]
Generate schemas for mutter-only key bindings

Recover the schemas generation for key bindings from metacity with a minor
modification:
If __MUTTER_ONLY__ is defined when including all-keybindings.h, bindings
inherited from metacity are ignored.

Generate schemas for key bindings using that define to avoid conflicts with
the schemas installed by metacity.

This is an alternative approach, which uses a modified version of the schemas generation in metacity.

(In reply to comment #6)
> This is an alternative approach, which uses a modified version of the schemas
> generation in metacity.

Yeah that might be better than having to add them manually. 
The question is if we intend to add more mutter specific keybindings in the future but having the infrastructure in place wouldn't hurt.

> +  fprintf (target_file, "      <owner>metacity</owner>\n");

Shouldn't this be mutter?

(In reply to comment #1)
> This seems to have been broken by commit
> 2d57b1b4703eb660aa398dfd84b52119f1e58ab4 in mutter.
> 
> Reverting it fixes the issue.

Weird, will debug.

(In reply to comment #4)
> Created an attachment (id=153736) [details] [review]
> Resurrect schema-bindings.c
> 
> This is the source of a small helper program to generate gconf schemas for
> key bindings. As mutter uses the schemas provided by metacity, this file
> was deleted when the project forked.
> Get the file back from the current metacity tree to generate schemas for
> mutter-only key bindings.

I don't really want to go with either of these approaches; the screensaver keybinding into Mutter was a hack to begin with. We shouldn't keep piling on the hack.

The right way to go here is to allow for plugins like gnome-shell to supply their own keybindings that get integrated into the Mutter keybindings.

But first we should fix the hard-coded defaults to work right. (May just be a mismerge between my change to use the hard-coded defaults as a fallback to GConf and Matthias's patch)

Created attachment 153962 [details] [review]
Fix fallback to builtin defaults for key bindings

The change to reduce GConf trips by using
gconf_client_all_entries() broke the fallback to builtin values
because update_binding() was no longer called for bindings not
found in GConf. Fix this by keeping track of the bindings we
find from GConf in a hash table, then looping through and setting
all bindings at the end.

There's also an efficiency increase here from avoiding a linear
scan for each binding in GConf, though in practice the difference
is probably small.

This patch also corrects another minor problem introduced by the GConf
round-trip patch where list key bindings were no longer taking
reliably taking preference over scalar key bindings - the precendence
was instead based on the order GConf returned entries.

Comment on attachment 153962 [details] [review]
Fix fallback to builtin defaults for key bindings

>round-trip patch where list key bindings were no longer taking
>reliably taking preference over scalar key bindings - the precendence

s/taking reliably taking/reliably taking/
s/precendence/precedence/

>+  bindings_to_update = g_hash_table_new_full (g_str_hash, g_str_equal,
>+                                              (GDestroyNotify) g_free,
>+                                              (GDestroyNotify) g_free);

(FWIW, the casts are unnecessary; g_free already is a GDestroyNotify (as is g_object_unref).)

>+  if (*gconf_key == '/')
>+    start = strrchr (gconf_key, '/');
>+  else
>+    start = gconf_key;

you want a "+ 1" at the end of the "then" case don't you?


In general it looks like it should work, though it's a little weird how list-key entries are processed in the first loop, and string-key entries in the second one. Another way would be to just save all the GConfEntries into the hash table in the first loop (but still not allowing string-key entries to override list-key entries), and then do all the setting in the second loop.

But if you like the current code, it's fine like that too.

(In reply to comment #9)
> I don't really want to go with either of these approaches; the screensaver
> keybinding into Mutter was a hack to begin with. We shouldn't keep piling on
> the hack.

Marking patches obsolete then ...

Created attachment 154167 [details] [review]
Fix fallback to builtin defaults for key bindings

Dan: Good catch for the + 1 in binding_name(). You were right too that
storing the GConfEntry in the hash table is cleaner but after I went
ahead and implemented that, I decided to test thoroughly and found
that my understanding of the interaction of list and 'scalar' bindings
was wrong; they are supposed to combine together rather than having
the scalar bindings override the list bindings. So, I rewrote things
again and came up with this patch. (Which goes back to doing the list
bindings immediately and the scalar bindings deferred, but with a
bit better reason - if we wanted to defer both than we'd have to
keep a hash table of lists, which would be painful. While doing
both immediately would lose the optimization here of not linearly
scanning for each scalar binding. And there are quite a few scalar
bindings, so that optimization isn't necessarily trivial.)

Attachment 154167 [details] pushed as 8fa83e1 - Fix fallback to builtin defaults for key bindings