GNOME Bugzilla – Bug 732385
Make gnome-control-center aware of -backward keybindings
Last modified: 2014-08-17 17:33:03 UTC
Currently the keybindings for -backward actions are implicitly created as being <shift>+the non-backward binding. This causes issues with gnome-control-center keyboard panel as it does not know about these implicit keybindings and cannot warn in case of conflicts. This patch series (together with more changes in gsettings-desktop-schemas and gnome-control-center) aims to improve that.
Created attachment 279475 [details] [review] Don't automatically add bindings for -backward actions Currently the bindings for {switch,cycle}.* actions are created with the META_KEY_BINDING_REVERSES flag so that <shift>+binding triggers the reverse action. However, gnome-control-center does not know about this kind of implicit bindings, and, for example, cannot warn when the user tries to setup a conflicting <shift>+xxx binding. These backward <shift> bindings are being explicitly set in gsettings-desktop-schemas, so the META_KEY_BINDING_REVERSES annotation can be removed for them from mutter.
Created attachment 279476 [details] [review] Add hidden -backward bindings to 50-mutter-navigation.xml This makes the gnome-control-center keyboard panel aware of these bindings so that it can warn about conflicting bindings if the user tries to use one of these bindings for a different action.
Can you please also remove META_KEY_BINDING_REVERSES?
Review of attachment 279475 [details] [review]: OK
Review of attachment 279476 [details] [review]: Looks correct.
Review of attachment 279476 [details] [review]: ::: data/50-mutter-navigation.xml.in @@ +52,3 @@ + reverse-entry="switch-applications" + hidden="true" + _description="Switch to previous application"/> Unless I'm overlooking something, isn't adding a description to a hidden entry a bit silly? More so as it is marked for translation ...
Review of attachment 279476 [details] [review]: ::: data/50-mutter-navigation.xml.in @@ +52,3 @@ + reverse-entry="switch-applications" + hidden="true" + _description="Switch to previous application"/> When trying to set a key binding which conflicts with a hidden one, gnome-control-center will show a dialog saying something like (paraphrasing) "The binding you are trying to set for XXX is conflicting with the binding set with "Switch to previous application", if you move forward, the latter binding will be disabled", so the translated version of the hidden entry name is user-visible when this dialog is shown.
(In reply to comment #3) > Can you please also remove META_KEY_BINDING_REVERSES? Is this possible? It's defined in src/meta/prefs.h which is listed in Makefile.am in libmutterinclude_headers. This variable goes with this comment: # Headers installed for plugins; introspected information will # be extracted into Mutter-<version>.gir If some external plugins can be using this, I guess we can only mark this as deprecated (dunno if the logic that goes with it should be kept or removed at the time of deprecation).
(In reply to comment #9) > Is this possible? Yes, mutter's API is not stable. > If some external plugins can be using this Only outside of GNOME - there can only be one plugin at a time, so under GNOME that slot is taken by gnome-shell. So far we've occasionally had requests for adding API for other environments, but I'm not aware of any complaints for removing/renaming existing API (and we've done that quite a lot).
Created attachment 282724 [details] [review] Remove use of META_KEY_BINDING_REVERSES Now that the internal mutter bindings and gnome-shell stopped using META_KEY_BINDING_REVERSES, and after moving the 'adding shift reverses the keybinding action' logic to gnome-control-center, we can remove META_KEY_BINDING_REVERSES from mutter. Plugin API is broken as this constant is removed from the exported headers. ABI is broken as using this flag is now a noop. A runtime warning warns about this when something tries to use the old value at runtime.
How do these patches look with the additional explanations? I'd like to get all of this in before the freeze
Review of attachment 282724 [details] [review]: ::: src/core/prefs.c @@ +2182,3 @@ + if ((flags & META_KEY_BINDING_REVERSES) != 0) + g_warning ("META_KEY_BINDING_REVERSES is deprecated" + " and no longer does anything"); In GNOME, this is pretty useless - all WM keybindings defined outside of mutter itself are added from JS, so maintaining ABI here does not help ... ::: src/meta/prefs.h @@ +376,3 @@ META_KEY_BINDING_PER_WINDOW = 1 << 0, META_KEY_BINDING_BUILTIN = 1 << 1, +/* 1 << 2 used to be META_KEY_BINDING_REVERSES which is deprecated */ ... when you remove the enum entry from the GIR. Seriously, just remove it altogether - just take a look at all the API/ABI breaks just during this cycle in "git log 3.12.0...3.13.4 -- src/meta" (here[0] is an example of removing a whole bunch of values from a public enum). [0] https://git.gnome.org/browse/mutter/commit?id=ebb6847bd11a
Created attachment 283205 [details] [review] Remove use of META_KEY_BINDING_REVERSES Now that the internal mutter bindings and gnome-shell stopped using META_KEY_BINDING_REVERSES, and after moving the 'adding shift reverses the keybinding action' logic to gnome-control-center, we can remove META_KEY_BINDING_REVERSES from mutter. Plugin API is broken as this constant is removed from the exported headers. ABI is broken as using this flag is now a noop.
Review of attachment 283205 [details] [review]: ::: src/meta/prefs.h @@ +376,3 @@ META_KEY_BINDING_PER_WINDOW = 1 << 0, META_KEY_BINDING_BUILTIN = 1 << 1, +/* 1 << 2 used to be META_KEY_BINDING_REVERSES which is deprecated */ Let me know if I should remove this comment, or if I should even change this to META_KEY_BINDING_IS_REVERSED = 1 << 2
Comment on attachment 283205 [details] [review] Remove use of META_KEY_BINDING_REVERSES (In reply to comment #15) > +/* 1 << 2 used to be META_KEY_BINDING_REVERSES which is deprecated */ > > Let me know if I should remove this comment, or if I should even change this to > META_KEY_BINDING_IS_REVERSED = 1 << 2 That's what I'd personally do, but there are precedents for leaving "holes" in enums, so it's just a matter of preference.
Attachment 279475 [details] pushed as bb59b8c - Don't automatically add bindings for -backward actions Attachment 279476 [details] pushed as 679edac - Add hidden -backward bindings to 50-mutter-navigation.xml Attachment 283205 [details] pushed as 20a6243 - Remove use of META_KEY_BINDING_REVERSES