GNOME Bugzilla – Bug 663428
keybindings: Allow to add/remove keybindings at runtime
Last modified: 2011-11-21 23:46:22 UTC
See patches. This should make it possible to add keybindings at runtime for both gnome-shell and extensions (meta_display_remove_keybinding() in particular targets extensions)
Created attachment 200707 [details] [review] keybindings: Store keybindings dynamically Rather than defining keybindings in static arrays generated at compile time, store them in a hash table initialized in meta_display_init_keys() and filled in init_builtin_keybindings(). This is a prerequisite for allowing to add/remove keybindings at runtime.
Created attachment 200708 [details] [review] keybindings: Allow to add/remove keybindings at runtime Add meta_display_add_keybinding()/meta_display_remove_keybinding(), which allow to add/remove keybindings dynamically at runtime.
Review of attachment 200707 [details] [review]: ::: src/core/prefs.c @@ +1767,3 @@ +meta_key_pref_free (MetaKeyPref *pref) +{ + update_binding (pref, NULL); It seems this will re-set the binding to the default handler. I don't see where we're freeing the GSList* or the MetaKeyCombo*. @@ +2183,3 @@ +/** + * meta_prefs_get_keybindings: + * Return: (element-type MetaKeyPref) (transfer container): Meta.KeyPref is the syntax that gobject-introspection likes, I believe. @@ +2275,1 @@ + if (c->keysym!=0 || c->modifiers!=0) whitespace.
(In reply to comment #3) > Review of attachment 200707 [details] [review]: > > ::: src/core/prefs.c > @@ +1767,3 @@ > +meta_key_pref_free (MetaKeyPref *pref) > +{ > + update_binding (pref, NULL); > > It seems this will re-set the binding to the default handler. I don't see where > we're freeing the GSList* or the MetaKeyCombo*. In update_binding() - it's kind of abusing it, but I actually don't call update_binding() for any other reason than to free pref->bindings. > @@ +2183,3 @@ > +/** > + * meta_prefs_get_keybindings: > + * Return: (element-type MetaKeyPref) (transfer container): > > Meta.KeyPref is the syntax that gobject-introspection likes, I believe. Hmm, apparently it doesn't matter, as introspection does not work with either annotation unless we register a boxed type for MetaKeyPref; I should probably just (skip) it ... > @@ +2275,1 @@ > + if (c->keysym!=0 || c->modifiers!=0) > > whitespace. Fixed locally.
(In reply to comment #4) > (In reply to comment #3) > > Review of attachment 200707 [details] [review] [details]: > > > > ::: src/core/prefs.c > > @@ +1767,3 @@ > > +meta_key_pref_free (MetaKeyPref *pref) > > +{ > > + update_binding (pref, NULL); > > > > It seems this will re-set the binding to the default handler. I don't see where > > we're freeing the GSList* or the MetaKeyCombo*. > > In update_binding() - it's kind of abusing it, but I actually don't call > update_binding() for any other reason than to free pref->bindings. I don't see where, though. The only place I see a g_slist_free is in update_list_binding(). Unless I'm reading this incorrectly, or I missed a patch, I don't see where update_binding() frees anything.
Review of attachment 200708 [details] [review]: Looks fine. ::: src/meta/prefs.h @@ +339,3 @@ gboolean per_window:1; + + /** for core keybindings not added with meta_display_add_keybinding() */ are there any core bindings added with meta_display_add_keybinding?
(In reply to comment #5) > I don't see where, though. The only place I see a g_slist_free is in > update_list_binding(). Unless I'm reading this incorrectly, or I missed a > patch, I don't see where update_binding() frees anything. There's a dependency on bug 635378 for a reason ;-)(In reply to comment #6) > Review of attachment 200708 [details] [review]: > + /** for core keybindings not added with meta_display_add_keybinding() */ > > are there any core bindings added with meta_display_add_keybinding? No, and I wouldn't really expect any. Should be s/core// then I guess.
(In reply to comment #7) > (In reply to comment #5) > > I don't see where, though. The only place I see a g_slist_free is in > > update_list_binding(). Unless I'm reading this incorrectly, or I missed a > > patch, I don't see where update_binding() frees anything. > > There's a dependency on bug 635378 for a reason ;-)(In reply to comment #6) Just an oversight on my part there. Looks fine. > > Review of attachment 200708 [details] [review] [details]: > > + /** for core keybindings not added with meta_display_add_keybinding() */ > > > > are there any core bindings added with meta_display_add_keybinding? > > No, and I wouldn't really expect any. Should be s/core// then I guess. Or just "/* for core keybindings */". Use /*, not /**, as the latter will confuse gtk-doc (I'm working on porting mutter to use gtk-doc in my local tree). > Hmm, apparently it doesn't matter, as introspection does not work with either > annotation unless we register a boxed type for MetaKeyPref; I should probably > just (skip) it ... Is there any reason we aren't registering it as a boxed type? I'd like to have as much APIs for extension authors to deal with out of the box.
(In reply to comment #8) > Use /*, not /**, as the latter will confuse gtk-doc Then we'll fix it up later, but I neither want to break inconsistency with the other comments, nor mix completely unrelated doc fixes with the patch. > Is there any reason we aren't registering it as a boxed type? I'd like to have > as much APIs for extension authors to deal with out of the box. I added one now.
Created attachment 200798 [details] [review] keybindings: Store keybindings dynamically Rebased to latest patch in bug 635378.
Created attachment 200799 [details] [review] keybindings: Allow to add/remove keybindings at runtime Rebased to lastest patch in bug 635378.
*** Bug 651901 has been marked as a duplicate of this bug. ***
Review of attachment 200798 [details] [review]: Looks fine to me.
Review of attachment 200799 [details] [review]: A few minor comments. ::: src/core/prefs.c @@ +491,3 @@ +meta_key_binding_copy (const MetaKeyBinding *binding) +{ + return g_memdup (binding, sizeof(MetaKeyBinding)); sizeof (MetaKeyBindings) @@ +495,3 @@ + +GType +meta_key_binding_get_type (void) Just use: G_DEFINE_BOXED_TYPE (MetaKeyBinding, meta_key_binding, meta_key_binding_copy, g_free); @@ +2155,1 @@ * Return: (element-type MetaKeyPref) (transfer container): Meta.KeyPref, no?
(In reply to comment #14) > ::: src/core/prefs.c > @@ +491,3 @@ > +meta_key_binding_copy (const MetaKeyBinding *binding) > +{ > + return g_memdup (binding, sizeof(MetaKeyBinding)); > > sizeof (MetaKeyBindings) Mmh, I was treating built-ins like macros, but sure ... > @@ +2155,1 @@ > * Return: (element-type MetaKeyPref) (transfer container): > > Meta.KeyPref, no? I didn't test Meta.KeyPref, but MetaKeyPref works - so it's either correct or equivalent :)
Created attachment 201207 [details] [review] keybindings: Store keybindings dynamically Adjust to schema changes.
Created attachment 201208 [details] [review] keybindings: Allow to add/remove keybindings at runtime Rebased patch, updated doc comments.
Created attachment 201862 [details] [review] keybindings: Store keybindings dynamically Hide MetaKeyHandler and MetaKeyBinding structs.
Created attachment 201863 [details] [review] keybindings: Allow to add/remove keybindings at runtime Add accessor functions for useful MetaKeyBinding members.
Review of attachment 201862 [details] [review]: Basically looks good - few fairly superficial comments. ::: src/core/keybindings.c @@ +425,3 @@ &display->key_bindings, &display->n_key_bindings, + prefs); Looks like this leaks prefs? @@ +3032,3 @@ gpointer dummy) { + gint backwards = binding->handler->flags & META_KEYBINDING_FLAGS_IS_REVERSED; Existing problem, but really should be: gboolean backwards = (binding->handler->flags & META_KEYBINDING_FLAGS_IS_REVERSED) != 0 having a random flag value as a "boolean" is ugly. @@ +3046,3 @@ gpointer dummy) { + gint backwards = binding->handler->flags & META_KEYBINDING_FLAGS_IS_REVERSED; same here @@ +3465,3 @@ MetaKeyBinding *binding) { + gint backwards = binding->handler->flags & META_KEYBINDING_FLAGS_IS_REVERSED; And here @@ +3482,3 @@ + META_KEYBINDING_FLAGS_NONE, + META_KEYBINDING_ACTION_WORKSPACE_1, + handle_switch_to_workspace, 0); OK like this. If you did: struct { const char name[big enough value] int ... int .... int ... } you'd have somewhat more efficient code generation, I think in terms of size and relocations. (But then you need to manually maintain the 'big enough value', which is one longer than the max length of the names), so OK like this. ::: src/core/prefs.c @@ +1913,3 @@ + { + meta_warning ("Trying to re-add keybinding \"%s\".\n", name); + return FALSE; This combination of warn-and-return-false is a little sketchy. In general, API should either: - fail in ways that the caller can't anticipate and must handle - these failures should be signaled in the API, usually with a GError and be silent - fail in ways that the caller can anticipate, or has no way of handling - these failures should generally not be signaled in the API and may produce warnings But that being said, I'm not really concerned here and am find leaving the return value. @@ +1931,3 @@ + pref->bindings = NULL; + pref->add_shift = !!(flags & META_KEYBINDING_FLAGS_REVERSES); + pref->per_window = !!(flags & META_KEYBINDING_FLAGS_PER_WINDOW); I'd say style is () != 0, not !!() ::: src/meta/prefs.h @@ +240,3 @@ + META_KEYBINDING_FLAGS_PER_WINDOW = 1 << 0, + META_KEYBINDING_FLAGS_REVERSES = 1 << 1, + META_KEYBINDING_FLAGS_IS_REVERSED = 1 << 2 META_KEY_BINDING_FLAGS, because MetaKeyBinding, but I'd suggest not including _FLAGS_ in the constant names for improved readability and cnsistency. (MetaFrameFlags, MetaMaximizeFlags) META_KEY_BINDING_NONE META_KEY_BINDING_PER_WINDOW @@ +289,3 @@ + const char *schema, + MetaKeyBindingAction action, + MetaKeyBindingFlags flags); As mentioned on IRC, I think it's confusing to have these in a public header - keybindings-private.h would be OK lacking a prefs-private.h
Review of attachment 201863 [details] [review]: Just a few comments ::: src/core/prefs.c @@ +468,3 @@ + +static MetaKeyBinding * +meta_key_binding_copy (const MetaKeyBinding *binding) Maybe this should be ref-counted? Do we want to make a copy of the (immutable?) MetaKeyBinding when passing it to JS code. Also, what about the const char *name, and MetaKeyHandler *handler? do they dangle if we copy the structure and free the original? @@ +1945,3 @@ { settings = g_settings_new (schema); + if (!!(flags & META_KEYBINDING_FLAGS_BUILTIN)) != 0 @@ +1958,3 @@ pref->add_shift = !!(flags & META_KEYBINDING_FLAGS_REVERSES); pref->per_window = !!(flags & META_KEYBINDING_FLAGS_PER_WINDOW); + pref->builtin = !!(flags & META_KEYBINDING_FLAGS_BUILTIN); != 0 ::: src/meta/keybindings.h @@ +27,3 @@ +const char *meta_keybinding_get_name (MetaKeyBinding *binding); +MetaVirtualModifier meta_keybinding_get_modifiers (MetaKeyBinding *binding); +guint meta_keybinding_get_mask (MetaKeyBinding *binding); These should be meta_key_binding since it's KeyBinding
Created attachment 201885 [details] [review] keybindings: Store keybindings dynamically (In reply to comment #20) > ::: src/core/keybindings.c > @@ +425,3 @@ > &display->key_bindings, > &display->n_key_bindings, > + prefs); > > Looks like this leaks prefs? Woops, yeah. > @@ +3032,3 @@ > gpointer dummy) > { > + gint backwards = binding->handler->flags & > META_KEYBINDING_FLAGS_IS_REVERSED; > > Existing problem, but really should be: > > gboolean backwards = (binding->handler->flags & > META_KEYBINDING_FLAGS_IS_REVERSED) != 0 > > having a random flag value as a "boolean" is ugly. Sure. > ::: src/core/prefs.c > @@ +1913,3 @@ > + { > + meta_warning ("Trying to re-add keybinding \"%s\".\n", name); > + return FALSE; > > This combination of warn-and-return-false is a little sketchy. In general, API > should either: > > - fail in ways that the caller can't anticipate and must handle - these > failures should be signaled in the API, usually with a GError and be silent > - fail in ways that the caller can anticipate, or has no way of handling - > these failures should generally not be signaled in the API and may produce > warnings > > But that being said, I'm not really concerned here and am fine leaving the > return value. Also fine with the warning? > @@ +1931,3 @@ > + pref->bindings = NULL; > + pref->add_shift = !!(flags & META_KEYBINDING_FLAGS_REVERSES); > + pref->per_window = !!(flags & META_KEYBINDING_FLAGS_PER_WINDOW); > > I'd say style is () != 0, not !!() Yeah - I only used the !!() syntax because of the existing code, I don't really like it either :) > ::: src/meta/prefs.h > @@ +240,3 @@ > + META_KEYBINDING_FLAGS_PER_WINDOW = 1 << 0, > + META_KEYBINDING_FLAGS_REVERSES = 1 << 1, > + META_KEYBINDING_FLAGS_IS_REVERSED = 1 << 2 > > META_KEY_BINDING_FLAGS, because MetaKeyBinding ... but META_KEYBINDING_ACTION_FOO or meta_prefs_get_keybindings(); the code is awefully inconsistent :( (nevertheless, updated) > _FLAGS_ in the constant names for improved readability and cnsistency. > (MetaFrameFlags, MetaMaximizeFlags) Done. > @@ +289,3 @@ > + const char *schema, > + MetaKeyBindingAction action, > + MetaKeyBindingFlags flags); > > As mentioned on IRC, I think it's confusing to have these in a public header - > keybindings-private.h would be OK lacking a prefs-private.h Done.
(In reply to comment #22) > > ::: src/core/prefs.c > > @@ +1913,3 @@ > > + { > > + meta_warning ("Trying to re-add keybinding \"%s\".\n", name); > > + return FALSE; > > > > This combination of warn-and-return-false is a little sketchy. In general, API > > should either: > > > > - fail in ways that the caller can't anticipate and must handle - these > > failures should be signaled in the API, usually with a GError and be silent > > - fail in ways that the caller can anticipate, or has no way of handling - > > these failures should generally not be signaled in the API and may produce > > warnings > > > > But that being said, I'm not really concerned here and am fine leaving the > > return value. > > Also fine with the warning? Yes. (To me, the warning is expected, the return value is slightly extraneous.)
Created attachment 201886 [details] [review] keybindings: Allow to add/remove keybindings at runtime (In reply to comment #21) > @@ +468,3 @@ > + > +static MetaKeyBinding * > +meta_key_binding_copy (const MetaKeyBinding *binding) > > Maybe this should be ref-counted? Yeah, makes sense. > Also, what about the const char *name, and MetaKeyHandler *handler? do they > dangle if we copy the structure and free the original? Right. Consumers are not expected to take a reference from the signal handler, but it's definitively ugly. > @@ +1945,3 @@ > { > settings = g_settings_new (schema); > + if (!!(flags & META_KEYBINDING_FLAGS_BUILTIN)) > > != 0 > > @@ +1958,3 @@ > pref->add_shift = !!(flags & META_KEYBINDING_FLAGS_REVERSES); > pref->per_window = !!(flags & META_KEYBINDING_FLAGS_PER_WINDOW); > + pref->builtin = !!(flags & META_KEYBINDING_FLAGS_BUILTIN); > > != 0 > > ::: src/meta/keybindings.h > @@ +27,3 @@ > +const char *meta_keybinding_get_name (MetaKeyBinding *binding); > +MetaVirtualModifier meta_keybinding_get_modifiers (MetaKeyBinding *binding); > +guint meta_keybinding_get_mask (MetaKeyBinding *binding); > > These should be meta_key_binding since it's KeyBinding Done, done and done.
Review of attachment 201885 [details] [review]: Looks good
Review of attachment 201886 [details] [review]: Looks mostly good, would like to see all the MetaKeyBinding stuff consolidated into one place, rather than having the type split over two files. ::: src/core/prefs.c @@ +463,2 @@ static void +meta_key_binding_free (MetaKeyBinding *binding) we would normally call these _ref() and _unref() Why don't we just make MetaKeyBinding copy the name() - just less things to think about if we don't have the siutation where saving a binding, then freeing the handler causes get_name() to segfault. [ Could go either way on it - there's a little voice on my shoulder saying "But that's ~100 separate mallocs on startup for No Point. So, if you'd rather not, then that's fine too ] Why are the type and the accessors in different files? I'd expect to fine everything for MetaKeyBinding as a type in keybindings.c.
Created attachment 201888 [details] [review] keybindings: Allow to add/remove keybindings at runtime (In reply to comment #26) > ::: src/core/prefs.c > @@ +463,2 @@ > static void > +meta_key_binding_free (MetaKeyBinding *binding) > > we would normally call these _ref() and _unref() Backed out of recounting as of discussion on IRC. > Why are the type and the accessors in different files? I'd expect to fine > everything for MetaKeyBinding as a type in keybindings.c. Done.
Review of attachment 201888 [details] [review]: Looks good
Attachment 201885 [details] pushed as d42a2a3 - keybindings: Store keybindings dynamically Attachment 201888 [details] pushed as 0e50287 - keybindings: Allow to add/remove keybindings at runtime (In reply to comment #23) > Yes. (To me, the warning is expected, the return value is slightly extraneous.) Ah, OK - I don't feel strongly attached to the return value, it just looked like something extension authors might be interested in.