GNOME Bugzilla – Bug 682069
keyboard: Add a shortcuts section for common XKB options
Last modified: 2013-01-20 14:55:29 UTC
probably be improved. The Good: I couldn't make the combos pop up when just cliking in the row or activating it. Looking at the various APIs it feels like there's a way to do it but I wasn't able to find it so far. The Bad: There's no detection of conflicting options, i.e. you can end up with both options assigned to, say, right ctrl. It doesn't help that the XKB id isn't the same for the same key in different options like compose:rctrl and lv3:switch for the right ctrl key... The Ugly: The description strings from xkeyboard-config are full of crazy. I'll try to come up with a patch for that soon. and more Ugly: The cell combos look a bit strange, flashing the combobox just before and after the popup menu.
Created attachment 221543 [details] [review] keyboard: Introduce a BINDING_GROUP_SEPARATOR type for the separator This way we can control the shortcut section separator position in case we introduce other types.
Created attachment 221544 [details] [review] keyboard: Add a shortcuts section for common XKB options Both the compose key and the 3rd level chooser are common and useful enough to expose in the control center. This patch adds them to a new shortcuts section that looks just like the others but uses combo cell renderers to allow users to choose one of the several options that XKB presents us.
(In reply to comment #0) Fail *sigh*. It should read: This works well in my testing. Yet, there are some things which can probably be improved. ...
Review of attachment 221543 [details] [review]: Looks fine to me.
Review of attachment 221544 [details] [review]: And most of the gruesome code in keyboard-shortcuts.c should actually live in cc-keyboard-item.[ch] where you would add a new type and handle all the goo in there. This also means that you can do the hacky conversions (normalising the Ctrl key for example). ::: panels/keyboard/cc-keyboard-item.h @@ +36,3 @@ { BINDING_GROUP_SYSTEM, + BINDING_GROUP_XKB_OPTIONS, No, it's under system too.
(In reply to comment #0) > probably be improved. > > The Good: > I couldn't make the combos pop up when just cliking in the row or > activating it. Looking at the various APIs it feels like there's a > way to do it but I wasn't able to find it so far. Look for start_editing_cb() in keyboard-shortcuts.c, should be possible to trigger the popup that way. > The Bad: > There's no detection of conflicting options, i.e. you can end up > with both options assigned to, say, right ctrl. It doesn't help that > the XKB id isn't the same for the same key in different options like > compose:rctrl and lv3:switch for the right ctrl key... That should be possible if the cc-keyboard-item.[ch] will take care of marshalling and de-marshalling the xkb option into a string compatible with what GTK+ uses. > The Ugly: > The description strings from xkeyboard-config are full of > crazy. I'll try to come up with a patch for that soon. Cool. A patch to gnome-desktop right? You might want to make sure that the gnome-desktop changes include both a unique identifier and a translatable string. > and more Ugly: > The cell combos look a bit strange, flashing the combobox just > before and after the popup menu. Might want to discuss this with ofourdan, he has some experience with the popups in treeview too.
(In reply to comment #6) > Look for start_editing_cb() in keyboard-shortcuts.c, should be possible to > trigger the popup that way. Yeah, I experimented with it. What I wasn't able to find yet is a way to trigger the popups, but it certainly seems doable. > > The Bad: > > There's no detection of conflicting options, i.e. you can end up > > with both options assigned to, say, right ctrl. It doesn't help that > > the XKB id isn't the same for the same key in different options like > > compose:rctrl and lv3:switch for the right ctrl key... > > That should be possible if the cc-keyboard-item.[ch] will take care of > marshalling and de-marshalling the xkb option into a string compatible with > what GTK+ uses. I'll look into this. But I'm not getting the marshalling bit here. Where would the marshalling happen? Between which entities? You mean that we should save something like <Ctrl> in the setting? Does GTK+ even distinguishes between Right Ctrl and Left Ctrl, etc. ? > > The Ugly: > > The description strings from xkeyboard-config are full of > > crazy. I'll try to come up with a patch for that soon. > > Cool. A patch to gnome-desktop right? You might want to make sure that the > gnome-desktop changes include both a unique identifier and a translatable > string. I mean a patch to xkeyboard-config. You can see the translatable strings in http://cgit.freedesktop.org/xkeyboard-config/tree/rules/base.xml.in . But yes, I think maintaining a different set of description strings on GNOME's side would be more flexible for us.
> But yes, I think maintaining a different set of description strings on GNOME's side would be more flexible for us. Please don't. Let's not multiply the entities. What's the issue with flexibility in xk-c case?
(In reply to comment #8) > > But yes, I think maintaining a different set of description strings on GNOME's > side would be more flexible for us. > > Please don't. Let's not multiply the entities. What's the issue with > flexibility in xk-c case? Well, or maybe not, seeing as Sergey is here with us :-) BTW, is it considered a string freeze break if translatable strings like these change in an external module like xkeyboard-config?
Why would it be a break??? xk-c has its own translation rules.
*** Bug 678172 has been marked as a duplicate of this bug. ***
*** Bug 681685 has been marked as a duplicate of this bug. ***
*** Bug 678155 has been marked as a duplicate of this bug. ***
Created attachment 221764 [details] [review] keyboard: Rename the Input Sources shortcuts section to Typing This way we can add more typing related keybindings here.
Created attachment 221765 [details] [review] keyboard: Add common XKB options to the Typing shortcuts section -- I've moved the shortcuts into the new Typing section (see previous patch) and added a white list so that we only allow sane single modifier keys from the various things that XKB supports for both shortcuts. This patch also moves the bulk of the code into a new CcKeyboardOption class to keep things tidier.
Review of attachment 221764 [details] [review]: Looks fine
Review of attachment 221765 [details] [review]: Much nicer. ::: panels/keyboard/keyboard-shortcuts.c @@ +60,3 @@ + DETAIL_TYPE_KEY_ENTRY, + DETAIL_TYPE_XKB_OPTION, +} DetailType; I don't really like using "detail" here, maybe "shortcut type" is more fitting. @@ +857,3 @@ + + if (g_str_equal (description, _("Typing"))) + fill_xkb_options_shortcuts (shortcut_model); I don't think you should only be adding things to the model when switching sections. Add them from the start, and on reload.
Created attachment 221826 [details] [review] keyboard: Add common XKB options to the Typing shortcuts section -- > ::: panels/keyboard/keyboard-shortcuts.c > @@ +60,3 @@ > + DETAIL_TYPE_KEY_ENTRY, > + DETAIL_TYPE_XKB_OPTION, > +} DetailType; > > I don't really like using "detail" here, maybe "shortcut type" is more fitting. Indeed, fixed. > @@ +857,3 @@ > + > + if (g_str_equal (description, _("Typing"))) > + fill_xkb_options_shortcuts (shortcut_model); > > I don't think you should only be adding things to the model when switching > sections. Add them from the start, and on reload. Hmmm, but this is how the code works for the other keybindings as well. You can see that the shortcuts model is cleared a few lines up in this function and then the section's CcKeyboardItems are loaded into it just before I do this. The reload thing just gets the data out of the xml files and gsettings to create and store the CcKeyboardItem objects. For the CcKeyboardOption objects I do something similar, except that the objects are created and stored in cc-keyboard-option.c since you can't really create arbitrary numbers of them.
(In reply to comment #18) > Created an attachment (id=221826) [details] [review] > keyboard: Add common XKB options to the Typing shortcuts section > > -- > > ::: panels/keyboard/keyboard-shortcuts.c > > @@ +60,3 @@ > > + DETAIL_TYPE_KEY_ENTRY, > > + DETAIL_TYPE_XKB_OPTION, > > +} DetailType; > > > > I don't really like using "detail" here, maybe "shortcut type" is more fitting. > > Indeed, fixed. Cool > > @@ +857,3 @@ > > + > > + if (g_str_equal (description, _("Typing"))) > > + fill_xkb_options_shortcuts (shortcut_model); > > > > I don't think you should only be adding things to the model when switching > > sections. Add them from the start, and on reload. > > Hmmm, but this is how the code works for the other keybindings as > well. You can see that the shortcuts model is cleared a few lines up > in this function and then the section's CcKeyboardItems are loaded > into it just before I do this. > > The reload thing just gets the data out of the xml files and gsettings > to create and store the CcKeyboardItem objects. For the > CcKeyboardOption objects I do something similar, except that the > objects are created and stored in cc-keyboard-option.c since you can't > really create arbitrary numbers of them. Yep, that would be me confusing it with the GNOME 2 code...
Attachment 221543 [details] pushed as a24f221 - keyboard: Introduce a BINDING_GROUP_SEPARATOR type for the separator Attachment 221764 [details] pushed as 4e0bc86 - keyboard: Rename the Input Sources shortcuts section to Typing Attachment 221826 [details] pushed as 978ab40 - keyboard: Add common XKB options to the Typing shortcuts section