GNOME Bugzilla – Bug 164831
Allow a list of alternative keybindings to be specified for each action
Last modified: 2006-11-05 01:59:41 UTC
Currently, Main Menu, Run Application and Show Desktop are bound to Alt-F1, Alt-F2 and Ctrl-Alt-D by default. If we feel it's worth trying to help people migrating from Windows where possible, it might be worth considering binding these to Ctrl-Esc, <Super>R and <Super>D by default instead, as some distros (e.g. JDS) already do. (Expect Vinay to attach our patch for this shortly.)
My take is that we should add support for two keys and bind to both the windows and our defaults. Tons of people will complain if we break our current bindings. (In fact I think some of the current bindings may work on windows too?) There are also keyboards without the Windows key, e.g. IBM Thinkpads are a modern shipping system without one.
Havoc: Currently, the keys (apps/metacity/global_keybindings/panel_main_menu etc) are of the GConf type String and one action is bound to a single key. If we have to add support for two keys, then the GConf type of the keys should be changed to List and they should be handled accordingly in the gnome-keybindings capplet. Before going ahead with a fix wanted to confirm if this is the right approach. kindly let know
I'm not sure of the right approach. You can't just change the keys to a list because it will break old versions of GNOME using the same home directory. See forward/back compat guidelines on http://www.gnome.org/projects/gconf/ We could do various things, such as introduce keyname_1, keyname_2, keyname_3 as extra keybindings; or have a global_keybindings_1/keyname, global_keybindings_2/keyname type of setup. Or we could add keyname_list to go with each keyname. Adding support for this to the control panel is probably quite hard and could have negative UI impacts, so we could say that extra bindings are not editable via the UI. That has problems too though if going into the keybindings dialog to disable a specific binding and then you don't find it there. It's just sort of hard. That's why I haven't done it ;-) I think it will take a fair bit of work. If you come up with a proposed approach maybe post it before you do too much of the work so we can discuss.
Created attachment 36649 [details] Screenshot of keyboard shortcut dialog. Havoc, Calum, We could probably do the following: -> Introduce new gconf keys (as you have suggested) /apps/metacity/extra_keybings/keyname_1 /apps/metacity/extra_keybings/keyname_2 /apps/metacity/extra_keybings/keyname_3 -> In the gnome-keybinding-properties capplet, we could have a new section called 'Extra Keybindings' This can be editable like the other keys. Attached is the screenshot for the same. (will move the Extra Keybindings to the bottom of the list while actually fixing the bug) Kindly comment.
I think it might be a bit nicer if the keybindings dialog had a second column, "Secondary Shortcut" or something, so you can see right in the dialog if there are two shortcuts for a key? I don't know if it works.
Then in the 'Secondary Shortcut' column, only 3 keybindings ie Main menu, Run dialog and Show desktop will be editable (currently). In order that the column is editable for the rest of the keys listed in the capplet, we would have to provide secondary shortcuts for each of them. Is that what we want ?
I think if we're sure we only want a maximum of two shortcuts per command, then an extra column would work okay (provided we could think of a short enough name to go in the header). If you wanted to be more flexible, or even if you didn't, you could add an extra level to the treeview (probably collapsed by default): v Desktop Launch help browser | F1 Log out | Ctrl-Alt-Delete v Show panel menu | <Alt>F2 Alternative | <Mod4>Esc Alternative | <Ctrl><Alt>M Launch web browser | Disabled v Sound etc. Would also then need buttons to add/delete keybindings to the currently-selected command, of course. (BTW, this is rapidly turning into a dup of #108689).
Havoc, Jody, Calum, As suggested above, we can add an extra level to the treeview but instead of having separate keys for each of the alternative keys, have an alternative keyname list. For instance, the apps/metacity/global_keybindings/panel_main_menu key will have apps/metacity/global_keybindings/panel_main_menu_subkeylist containing all the alternative keys. v Desktop Launch help browser | F1 Log out | Ctrl-Alt-Delete v Show panel menu | <Alt>F2 Alternative | [<Mod4>Esc,<Ctrl><Alt>M,<Ctrl><Alt>P] Launch web browser | Disabled v Sound please let know if this is okay to go about.
It's not obvious from this design how a user would edit the list of Alternatives directly from the capplet. E.g. how would I change <ctrl><alt>M to <ctrl><alt>P, or delete <mod4>Esc altogether? The idea of my suggestion in comment #7 was to allow the user to edit alternative shortcuts in exactly the same way as regular shortcuts. How they are actually stored in gconf shouldn't influence the design of the UI.
Calum, I got your point. Will make use of list to store all the alternative but display it as you mentioned in comment #7
Created attachment 47904 [details] Latest screenshot
Created attachment 47905 [details] [review] Patch fixes the control-center part of the bug.
Created attachment 47906 [details] [review] Patch fixes the metacity part of the bug. Below are the functions which i have changed in the following patches to take care of the new alternative keybindings. In control-center patch. -Made an entry in desktop_key_list[] for the alternative keys Main Menu, Run App and Show Desktop. -Changed append_keys_to_tree() to take care of List while creating treeview. -Changed accel_edited_callback() to validate and update the new shotcuts changed from gnome-keybinding-properties capplet in case of list. -Changed accel_cleared_callback() to take care when an element of list has been disabled from gnome-keybinding-properties capplet. In metacity patch. a)In prefs.h file -Add two members i.e 'keysym_alt' and 'modifiers_alt' of type List to structure MetaKeyPref to handle the alternative list of shortcuts. -Defined the keys for all the three. b)In prefs.c file -Intialise the values for these alternative keys in screen_bindings[] array. -Changed init_bindings() in order to take care of list during initial binding. -Changed change_notify() to take care when an element of list has been changed either from gnome-keybinding-properties or gconf-editor. And also added update_list to update the list. c)In keybindings.c -In screen_handlers[], associate an handler with these alternative keys. -Changed rebuild_binding_table() to handle binding of all the list elements. d)In metacity.schemas.in Added schema section for all the three alternatives. Attached is the latest screenshot of the treeview changed slighly. As in the previous screenshot click on the row v Show the panel run application dialog | <Alt>F2 should take care of both collapsing and providing the accelerator, which will confuse the user.
Looks pretty good from the screenshot, except that I'd only expect to see 'alternatives' in the expanded section of the tree. Right now you seem to have the main shortcut there as well. That is, I'd expect to see: v Show the panel run application dialog Alt-F2 Alternative <Mod4>r Alternative Ctrl+Alt+R rather than v Show the panel run application dialog Show the panel run application dialog Alt-F2 Alternative <Mod4>r Alternative Ctrl+Alt+R Did we definitely decide to only allow two alternatives per shortcut? Not that I can imagine needing any more than that, but it makes the UI clunkier than it would be if you could just have as many as you needed. Also, in the screenshot I don't see any way to add an alternative shortcut for (say) Launch Help Browser... how are you proposing to do that?
Calum, a) Implementing as below has some issues v Show the panel run application dialog Alt-F2 Alternative <Mod4>r Alternative Ctrl+Alt+R -When user clicks on row v Show the panel run application dialog Alt-F2 -> he/she can either change the shortcut or collapse the treeview. -> if both actions are provided then, the first click on that row will allow the user to change the shortcut and a second click on that row will expand it. This would mean that in order to expand that row, the user will have to click twice. This might be confusing for the user. b) > Did we definitely decide to only allow two alternatives per shortcut? Presently one can add/edit/delete as many alternative shortcuts to the Run application, Main menu and show desktop as required through gconf-editor. c) > don't see any way to add an alternative shortcut for > (say) Launch Help Browser... - Providing alternative shortcut to each and every key dynamically would mean that Alternate list for each of the keys in the metacity schemas need to be added And with current changes as in the attached screenshots above capplet may look like this even if there are no alternative shortcut is assigned initialy. v Desktop v Launch help browser Launch help browser |F1 Alternative |disabled v Log out Log out |Ctrl-Alt-Delete Alternative |disabled v Show the panel run application dialog Show the panel run application dialog |Alt-F2 Alternative |<Mod4>r Alternative |<Alt>r
> When user clicks on row > v Show the panel run application dialog Alt-F2 > -> he/she can either change the shortcut or collapse the treeview. > -> if both actions are provided then, the first click on that row will allow the user to change the shortcut and a second click on that row will expand it. I would expect clicking on the expander arrow to expand/collapse that row, and clicking on any item of text to select that shortcut for editing. There shouldn't be any need to overload the click operation. (Double-clicking doesn't currently expand an unexpanded row AFAICT, at least not in Ubuntu Hoary, which is the latest version I have access to right now.) > Presently one can add/edit/delete as many alternative shortcuts to the Run > application, Main menu and show desktop as required through gconf-editor. I appreciate the technical reason, but special cases give usability people nightmares :/ I can think of other shortcuts that it would be useful to allow alternatives for, so I'd like to see this work for any shortcut. But I guess the maintainer should have the final decision on whether the extra work is worth it.
Created attachment 50508 [details] [review] Created the metacity patch against latest source
Created attachment 50509 [details] [review] Control-center patch which takes care of comment#16
Ok, I've tried the latest patch(es), and I think interaction-wise it works pretty nicely. The only problem I found was that if you try to replace an existing shortcut with itself (e.g. try to replace Ctrl-Space with Ctrl-Space), you get an irritating alert popping up telling you that the shortcut is already in use. It should really just silently accept the 'change'. I don't know if this behaviour is directly related to the patch or not, but it doesn't happen on the older 2.11 build I have here. My concerns about the extensability of the overall solution (see comment #16) remain, but I'm happy enough that the UI itself seems to work well, and that it could easily be extended to allow alternative shortcuts for more items in future, if required. (And on reflection, I think only being able to add one alternative shortcut for any given action is plenty, and that there's no need to clutter the UI further with Add/Remove Shortcut buttons that I sort of suggested in comment #7.)
Created attachment 51362 [details] [review] Control-center Patch This control-center patch fixes the problem of replacing an existing shortcut with itself as mentioned in comment#19. Metacity patch as not been changed, so one can use patch attached in comment#17.
I can confirm that things seem to work as expected with this new patch.
I'd like to test it out quickly too, and then give Havoc a ping. Which are the patches that need to be applied? (it looks like there's old ones hanging around that haven't been marked obsolete)
Elijah, You can use the patches id 50508(metacity patch) and 51362(control-center patch) for testing purpose. Since i don't have permission to mark other patches (id's 47905, 47906, 50509) as obsolete, could you please mark it as obsolete ?
If you can create cool patches like these, you ought to be able to edit bugs. So I gave you permissions to do so. ;-) But yeah, I'll mark the others as obsolete now and try to get to test the others soon.
Tested; works for me too. Havoc: that's to WFM's (Calum specified that too in comment 21). Time for you to take a look. :) (I'm going to cc control-center-maint because of the control center patch)
doh, s/to WFM's/two WFM's/. I'm obviously no English major. :)
Havoc and control-center maintainers please review my patch.
*** Bug 157824 has been marked as a duplicate of this bug. ***
Comment on attachment 50508 [details] [review] Created the metacity patch against latest source Thanks for the patch. Some of my comments here also apply to the control-center patch. On a high level I think the get_gconfvalue_type stuff is not ideal. Instead, you should just treat the list keys and the string keys separately, the same way the difference between int/bool/string is already handled. Alternatively, genericize the patch such that *any* keybinding "foobar" can have "foobar_alt" which is always a list. So don't put "foobar_alt" in the various tables of keybindings separately; instead, for each keybinding, also concat the _alt and then look up the list. With either of these approaches you simply know whether you're expecting a list of string, and as always you need to check that the GConfValue you get is the right type, but you don't need to lookup what to expect. I would name the gconf keys here something like _additional or _list rather than _alt (_alt sounds like it might relate to the alt key) gconf-client.h should not be put in prefs.h, it should remain confined to prefs.c. IS_STRING/IS_LIST should be an enum or bool and not #define, but per my earlier comment I would just get rid of this. Why did you move the screen_bindings/window_bindings arrays? I would say that keysym_alt/modifiers_alt lists should be merged into a single list of structs. In update_list you are leaking the GError from gconf_client_get_list I think. The return value should not have parentheses in code like: return (GCONF_VALUE_INVALID) In rebuild_binding_table(), the identical code for the string and list case should be factored out instead of cut-and-pasted.
Control center patches might be better off filed against the control center product and then the bugs linked. Anyway, I'm guessing from Havoc's comment that the control center patch should also be marked as needs work.
Kicking off the 2.14.x milestone given the small amount of recent activity.
Anyone else interested in picking this up? The Wipro guys have moved on to other work within Sun for the time being, but we'd still like to see this finished off.
I'll take a look at it today and tomorrow and see if I can figure anything out.
Still working on this. Maybe it'll be working by Monday.
*** Bug 108689 has been marked as a duplicate of this bug. ***
*** Bug 91538 has been marked as a duplicate of this bug. ***
Created attachment 62560 [details] [review] Allow any keybinding to be set both from "foo", a string, and "foo_list", a list Here's my first draft of a solution attempting to address the concerns Havoc raised above. Any keybinding gconf can now optionally have a foo_list which sets alternative key combinations. Currently, when you specify some kinds of bad key combinations (like <Shift>Tab for window cycling), metacity reverts your gconf setting. I've just made it ignore strings within lists which are bad, but maybe we should revert there, too. Let me know what you think-- I think this is my biggest metacity patch yet, so I'm sure it's open to improvement.
Comment on attachment 62560 [details] [review] Allow any keybinding to be set both from "foo", a string, and "foo_list", a list I wasn't able to find any large problems, and overall I like the organization. Just a couple smaller issues: >- meta_topic (META_DEBUG_PREFS, "Queueing change of pref %s\n", >+ meta_topic (META_DEBUG_PREFS, "Queueing change of pref %s\n", The old spacing was right. ;) >+ list_val = gconf_client_get_list (default_client, key, GCONF_VALUE_STRING, &err); >+ cleanup_error (&err); >+ >+ update_list_binding (&window_bindings[i], list_val, TRUE); >+ >+ g_slist_free (list_val); Isn't this a memory leak? Perhaps I have misunderstood the gconf docs, but don't you have to free the individual strings in the list as well? >+ list_val = gconf_client_get_list (default_client, key, GCONF_VALUE_STRING, &err); >+ cleanup_error (&err); >+ >+ update_list_binding (&screen_bindings[i], list_val, TRUE); >+ >+ g_slist_free (list_val); Same here. >+static gboolean >+update_list_binding (MetaKeyPref *binding, >+ GSList *value, >+ gboolean list_of_strings) <snip> >+ if (list_of_strings) >+ pref_string = pref_iterator->data; >+ else >+ pref_string = gconf_value_get_string (pref_iterator->data); Using a gboolean to specify the type of the list in value makes it hard to follow the code elsewhere. list_of_strings as a name also makes me thing 'well, what is it when it's false?' and forces me to read the function to find out. I'd prefer if you introduced a new enum with two values -- something like META_LIST_OF_STRINGS and META_LIST_OF_GCONFVALUE_STRINGS. Then, instead of passing TRUE and FALSE elsewhere in the code it becomes self-documenting by using these enum values.
Created attachment 63533 [details] [review] 62560: Allow any keybinding to be set both from "foo", a string, and "foo_list", a list: Version 2 Oh, well spotted. Here's a version which should address those problems. Thanks.
Okay, committed. Do we need to patch gnome-control-center before we can close this bug?
Oh! I just thought: do we need to set defaults for e.g. panel_main_menu_list and show_desktop_list so the Windows shortcuts work, or shall we leave that to the distros?
(In reply to comment #40) > Okay, committed. I just updated cvs and didn't see the changes. Are you sure you committed it? > Do we need to patch gnome-control-center before we can close this bug? No, let's open a separate bug for that (if one hasn't been opened already). (In reply to comment #41) > Oh! I just thought: do we need to set defaults for e.g. panel_main_menu_list > and show_desktop_list so the Windows shortcuts work, or shall we leave that > to the distros? Let's ping the usability folk (Calum, Bryan, Seth, Matthew) and get an opinion from them on what should be the defaults and do that. Do we have other bugs open for that already? If not, we can leave this bug open for that and retitle it.
(In reply to comment #42) > I just updated cvs and didn't see the changes. Are you sure you committed it? Errr, no. But I have now. :) > > Do we need to patch gnome-control-center before we can close this bug? > > No, let's open a separate bug for that (if one hasn't been opened already). Okay, bug 338805 it is. > Let's ping the usability folk (Calum, Bryan, Seth, Matthew) How should I go about doing that-- contacting them on IRC or something?
Apologies for spam-- marking AP4 to reflect accessibility impact.
Apologies for spam... ensuring Sun a11y folks are cc'ed on all current accessibility bugs.
Should we be closing this one now?
Yeah, we probably shouldn't have kept it open after you had added the functionality anyway. Do you want the honors?
Sure! Thank you! :)
I thought all this stuff went back, but in 2.16 I still don't see any UI for adding a secondary shortcut in gnome-keybindings-properties?
I did think about adding a UI for it, but 1) I couldn't see an obvious way to do so, 2) that's not really a metacity bug, and 3) it's not really the sort of thing that users want to do anyway. I think users will generally associate one keystroke with one action; this fix is useful in that a distro can set things up so multiple keystrokes provide one action (to accomodate users who expect either), but an individual user probably wouldn't want to for themselves.
What about users who also have to use different GUIs, like those of KDE, Apple or Windows? Dual key bindings also help for keyboards that have shortcut keys. I actually have ctrl+insert, ctrl+c and a 'dedicated multimedia button' shortcut for the copy action. 'New tab' or 'New window' is another binding that would be handy to be able to set several shortcuts for, since every environment insists on using their own. It is extremely annoying when simple shortcuts are different than expected, because it's not something you can re-learn in a few minutes. KDE's shortcut configuration dialog is excellent, as are their 'Input Actions'... I feel this is an important area where flexibility is required for many people and that is being overlooked by Gnome. The keyboard is the main peripheral, after all. It's not a luxury to be able to set it up to one's liking.
Wouter: That's not really relevant here. None of your examples have anything to do with metacity. Copy and new tab/window actions are the responsibility of the application; that could be handled by application authors directly or by the toolkit, but either way it's not the job of the WM. A shortcut key GUI editor would belong in control center, which again isn't part of the WM. This particular bug report is merely about having metacity allow its keybinding actions to each be bound to more than one key, which has been fixed by Thomas and Vinay. I think your arguments sound reasonable, they just don't make sense in a metacity bug report.
I must have misunderstood the reference in bug 322542, then. My apologies. What I said was not meant to apply to metacity only. I think that any shortcut, whether handled by metacity, control center or any of the gnome daemons (granted I lost track of what is handled where) could benefit from having an easy GUI for setting up multiple keys. Any normal user who wants to set up different keys, could also want to set up additional keys for one action, for instance when they also use different environments and window managers, or because they are migrating from another platform, or want to set up a multimedia keyboard with special buttons for window manager actions.