GNOME Bugzilla – Bug 329676
Setting cycle_windows to a single key results in incorrect behavior.
Last modified: 2006-02-27 22:46:00 UTC
Please describe the problem: When the cycle_windows key combination is set to a single key (e.g., "F3", "b", etc.), rather than a modified key (e.g., "<Alt>Escape", "<Alt>Tab", etc.), window cycling does not proceed correctly. Steps to reproduce: 1. In gconf-editor, set apps->metacity->global_keybindings->cycle_windows to "F3" 2. Press "F3" twice on a desktop with more than two windows. 3. Note that only two windows have been cycled through. Actual results: Only two windows are cycled through. Expected results: All windows are visited in a cycle. Does this happen every time? Yes. Other information: Although this is less critical, the same problem affects switch_windows.
Confirmed. How bizarre. *investigates*
We should probably disallow setting cycle_windows and switch_windows to keybindings without a primary modifier. The reason the bug occurs is - The windows are listed in MRU (most recently used) order - Switching to a window makes it the most recently used - When using Alt-Tab or Alt-Esc or something with some modifier like Alt that can be held down, the window isn't considered switched to until that modifier is let go - When not using a modifier, e.g. just F3, the window is switched to instantly, thereby juggling the MRU order instantly. Unfortunately, and hacks to attempt to fix this without disallowing such keybindings will probably merely be ugly not-fully-working hacks. How exactly does on distinguish the following two cases: - User hits F3 to cycle windows - User hits F3 to cycle windows again and - User hits F3 to cycle windows - User works with the app for a while doing what they want - User wants to go back to previous app, so - User hits F3 to cycle windows In the first case, without working in the app inbetween, we should not switch to the original window but pick the 3rd window in the original MRU list. In the second case, we should go back to the original window. So, in summary, and as stated above, I think the fix is to disallow a keybinding like F3 for an action like this.
That makes sense. We can't disable them being able to enter "F3" or whatever in gconf, though (or can we?) So maybe, when we pick up META_KEYBINDING_SWITCH_WINDOWS, we need to check whether Ctrl or Alt is being held down, and meta_warning and ignore if it's not?
We don't really need to disable it in gconf, we could just check for notification of a change to something like this and when we detect it, reset it back to what it was before. We have other code in prefs.c for checking if gconf values get set to invalid things (integers where strings are expected, values out of range when they are integers, etc.) so we'd just need to add a little more code. It'd probably be good to double check that it'll work nicely by opening up the keyboard shortcuts dialog from the preferences menu item and then running e.g. gconftool-2 --set /apps/metacity/global_keybindings/switch_windows \ --type string "<Alt>F3" in a terminal and verifying that the keyboard shortcuts dialog updates the "Move between windows with popup" item when you hit enter.
Yes, that does work. So I'm thinking that what we need is for update_binding() in prefs.c to know when we're dealing with a binding of this kind, and reject any attempt to set it to a keybinding where neither META_VIRTUAL_ALT_MASK and META_VIRTUAL_CONTROL_MASK are set. Then there's the question of how update_binding() can tell it's "a binding of this kind". Fortunately, all the relevant ones seem to be exactly the ones which have the add_shift member of their MetaKeyPref set. So we could probably just check that.
Created attachment 59152 [details] [review] Require that bindings which can be reversed with shift contain Ctrl or Alt Something like this is what I was thinking.
(But that doesn't yet reset it to whatever it was before, just disables it.)
- I don't particularly like the binding->add_shift thing, but I don't see any other reasonable solution. So, that might be fine. I'll probably ask Havoc to comment once the other issues are cleaned out. - We shouldn't limit to Alt or Ctrl. Super, Meta, etc. should be valid too. I think we just want mods to not be 0 or META_VIRTUAL_SHIFT_MASK, right? - I don't see why you have the keysym != 0 check. - Disabling the binding altogether isn't what we want. If the keybinding specified was totally invalid (i.e. some gibberish like "iofdsjls") then we should just ignore the change and return FALSE. If it would be valid other than a missing modifier then we should manually change it back to what it was before. This is slightly tricker, but to do so we basically need a new wrapper function in ui.c similiar to meta_ui_parse_accelerator() but which wraps egg_virtual_accelerator_name() instead of egg_accelerator_parse_virtual(). You can then use the return result of that new function in a call to gconf_client_set_string().
- I thought I could add another field for this, but then I realised it would entirely duplicate add_shift. I'm fine with doing that if you think it's justified, though. - META_VIRTUAL_SHIFT_MASK: good point. - keysym != 0 is there because the string can be set to "disabled", in which case you get keysym == mods == 0 back from the parser. But you don't want to complain about that, or you get a whole slew of warnings coming up every time you start the program. So we only want to check where keysym != 0. - Okay, I'll read up on that and figure something out.
Ah, so the keysym != 0 basically checks for either "disabled" or something totally bogus. Makes sense. You may want to make your comment a little longer and explain this and maybe the add_shift thing too. Just leave the add_shift thing for now. I really don't know what'd be better so I'll have Havoc take a look. Thanks for working on this. :)
Created attachment 59386 [details] [review] Forbid shiftable key bindings from having no modifiers or Shift alone No problem :) Here's another attempt. How's this?
Oh, reading over your comments again I find that this > If the keybinding > specified was totally invalid (i.e. some gibberish like "iofdsjls") then > we should just ignore the change and return FALSE. doesn't match up with the patch; it reverts back to the previous binding if the new binding is totally invalid. I'll try and make a new one now.
Created attachment 59387 [details] [review] Forbid shiftable key bindings from having no modifiers or Shift alone, and ignore nonsense values Another try, where we return FALSE as soon as invalid entries are detected. Is this better?
The second patch looks good. Just two minor nitpicks: The new patch makes the must_revert var unnecessary, and you should probably document that the caller is responsible for freeing the string returned by meta_ui_accelerator_name().
I fixed up the last two things (since they were just trivial changes) in order to get into the release today. Thanks for catching this David, and thanks for fixing it Thomas!