Created attachment 279339 [details] [review]
Use new meta_key_binding_is_reversed() method

Now that mutter gives a way to check if a MetaKeyBinding was marked as
'reversed' or not, gnome-shell does not have to hardcode that a
MetaKeyBinding using a shift modifier is reversed, it can directly check
if the appropriate flag is set.

Created attachment 279340 [details] [review]
Stop using Meta.KeyBindingFlags.REVERSES for IM switch keybinding

When this flag is set on a MetaKeyBinding, mutter will know that
the keybinding has an associated reverse keybinding triggered with
the shift modifier. However, an undesirable side-effect is that
gnome-control-center keyboard panel does not know that this 'shift'
is reserved for these reverse keybindings and cannot detect
conflicting bindings in this case.
This 'reverse' logic can now be handled at a higher level (in gcc keyboard
panel) so this commit removes it from gnome-shell so that they do not
conflict.

Review of attachment 279339 [details] [review]:

Why only for the input-source switcher?

Review of attachment 279340 [details] [review]:

I understand that <shift>space is a popular keyboard shortcut for input source switching, but I don't like this approach much - it is true that gnome-control-center does not know about the shift-magic, but the same applies just as well to app- and window switcher. So maybe a better approach would be to make the magic smarter and allow <shift> in reversable keybindings, but only actually reverse it when the original shortcut does not already contain <shift>?

Review of attachment 279339 [details] [review]:

For no good reason, I started with this one to see how far I could go with what I had in mind, and didn't go back yet to look at the other similar bindings. I'll look at the others :)

Review of attachment 279340 [details] [review]:

I agree that the same needs to be done for app- and window- switchers, it's just I did not get to that yet. The goal of this patch is not to allow <shift>+space to switch input methods, but more to give more visibility/control to gnome-control-center over these shortcuts rather than having shift handling hardcoded deep inside mutter.

With the patches from bug #731618, gnome-control-center now knows about the reverse shortcuts and can warn about conflicts when you try to use shift+meta+space as a keybinding. It will also suggest to set automatically the reverse binding to <shift>+xxxx when you set the switch input method binding to xxxx (and if this already contains shift, it will suggest a shortcut without shift.

(In reply to comment #6)
> give more visibility/control to gnome-control-center over these shortcuts 
> rather than having shift handling hardcoded deep inside mutter.

OK, only making the change for switch-input-source threw me off then - actually removing the hardcoded shift-logic inside mutter (not just disabling it for selected keybindings) sounds good to me.

(In reply to comment #7)
> (In reply to comment #6)
> > give more visibility/control to gnome-control-center over these shortcuts 
> > rather than having shift handling hardcoded deep inside mutter.
> 
> OK, only making the change for switch-input-source threw me off then - actually
> removing the hardcoded shift-logic inside mutter (not just disabling it for
> selected keybindings) sounds good to me.

Just for the record for people reading this bug in the future, this part is tracked in bug #732385

How do these patches look with the additional explanations? I'd like to get all of this in before the freeze

Review of attachment 279339 [details] [review]:

I'd say with the auto-shift magic removed, we really want this to apply for all the remaining switchers as well (apps, windows and a11y) ...

Review of attachment 279340 [details] [review]:

LGTM

Review of attachment 279339 [details] [review]:

Apps/Windows should be handled by the patches in bug #732385 and bug #732293. I haven't looked at all at a11y shortcuts, will check that now.

(In reply to comment #12)
> Apps/Windows should be handled by the patches in bug #732385 and bug #732293.

No, the alt-tab popup implementation in gnome-shell still checks explicitly for SHIFT_MASK - see _start*Switcher in js/ui/windowManager.js and _keyPressEvent in js/ui/switcherPopup.js

(In reply to comment #13)
> (In reply to comment #12)
> > Apps/Windows should be handled by the patches in bug #732385 and bug #732293.
> 
> No, the alt-tab popup implementation in gnome-shell still checks explicitly for
> SHIFT_MASK - see _start*Switcher in js/ui/windowManager.js and _keyPressEvent
> in js/ui/switcherPopup.js

Oh right, I totally missed/forgot those, thanks! I'll post some new patches, hopefully tomorrow.

Created attachment 283256 [details] [review]
Stop using Meta.KeyBindingFlags.REVERSES for IM switch keybinding

When this flag is set on a MetaKeyBinding, mutter will know that
the keybinding has an associated reverse keybinding triggered with
the shift modifier. However, an undesirable side-effect is that
gnome-control-center keyboard panel does not know that this 'shift'
is reserved for these reverse keybindings and cannot detect
conflicting bindings in this case.
This 'reverse' logic can now be handled at a higher level (in gcc keyboard
panel) so this commit removes it from gnome-shell so that they do not
conflict.

Created attachment 283257 [details] [review]
Remove 'backwards' argument from SwitcherPopup:_keyPressHandler

All derived classes are already checking explicitly for action names
(FOO and FOO_BACKWARDS). mutter used to have a META_KEY_BINDING_REVERSES
flag for keybindings which required special handling of "shift"+FOO as
FOO_BACKWARDS, but this has been removed now, so this special handling
is no longer necessary.

Created attachment 283258 [details] [review]
Use new meta_key_binding_is_reversed() method

Now that mutter gives a way to check if a MetaKeyBinding was marked as
'reversed' or not, gnome-shell does not have to hardcode that a
MetaKeyBinding using a shift modifier is reversed, it can directly check
if the appropriate flag is set.

Review of attachment 283258 [details] [review]:

Untested, but code looks good to me

Review of attachment 283257 [details] [review]:

Does this work as expected? For instance, when you hit <alt>tab to bring up the popup, can you then still add <shift> to reverse the direction? Assuming yes, this patch looks good to me as well ...

Review of attachment 283257 [details] [review]:

With the schema changes from bug #732293 this works as expected (I've tested alt+tab and alt+key-above-tab)

Attachment 283256 [details] pushed as e8fa2b6 - Stop using Meta.KeyBindingFlags.REVERSES for IM switch keybinding
Attachment 283257 [details] pushed as d450b74 - Remove 'backwards' argument from SwitcherPopup:_keyPressHandler
Attachment 283258 [details] pushed as c459ef6 - Use new meta_key_binding_is_reversed() method