I don't see a way around that. Florian?

(In reply to comment #1)
> I don't see a way around that. Florian?

uh, well, if we're going to have functions bound to multiple keys by default, then the bindings panel should have a UI that can represent that, right?


At the very least, it's clearly a bug that when you delete the visible binding, g-c-c changes the setting from:

  [ '<Super>Down', '<Alt>F5' ]

to:

  [ '', '<Alt>F5' ]

It should remove the first item entirely, not change it to ''.


Oh, and noticed belatedly that this applies to toggle-maximized too (<Super>Up and <Alt>F10).

I don't want to display 2 shortcuts in the list. In that particular case, I'd remove the 2nd binding when deleting the binding, or changing it from the default.

In 3.8, switch-applications is bound to [ '<Super>Tab', '<Alt>Tab' ], and there's no way to get rid of the '<Alt>Tab' binding from the control center, or even to see that it's there. The control center will let you bind '<Alt>Tab' to switch-windows, but it doesn't actually work, because it's really secretly still bound to switch-applications.

I had opened bug 703977 reporting the toggle-maximized issue Dan reported in comment 2. For some users, this is a significant hassle. I have been using the top four function keys (alt+F9-12) for various shortcuts for a decade now. This forces me to stop using alt+F10 for the terminal because I can't unbind it from maximize.

Generally speaking, if we intend to allow multiple shortcuts for the same action, then the UI should reflect that. Personally, I think wiping out multiple bindings when the user only changes one as suggested above is cruel and unusual and will only lead to more bugs.

Where are these defaults defined? I've searched the values in gconf and dconf and do not find F10 anywhere but menubar accelerator and my old terminal command. The window maximize only lists my setting of Alt+PageUp.

Created attachment 253199 [details] [review]
keyboard: Clear additional bindings when changing a shortcut

Some shortcuts allow multiple bindings for the same actions, which
we mainly use to keep supporting old settings when changing defaults.
Multiple bindings are not exposed in the interface though, so when
changing a shortcut with multiple bindings, we previously only updated
the first one and left additional bindings untouched. This is confusing
however, precisely because additional bindings are not shown in the
UI - for instance, this makes it impossible to actually disable
such a shortcut completely. The less confusing option is to clear
any additional bindings when changing a shortcut.

Review of attachment 253199 [details] [review]:

I think we should make it popup the conflict dialogue before actually removing the extra keybindings. The patch in itself looks good.

Yes, I've some WIP patches locally for doing this (and generally including hidden keybindings in conflict resolution). I also managed to speak with Allan about the desired behavior before he went on vacation, so I should be able to attach something in the next couple of days ...

*** Bug 701324 has been marked as a duplicate of this bug. ***

*** Bug 710245 has been marked as a duplicate of this bug. ***

*** Bug 712228 has been marked as a duplicate of this bug. ***

What about deleting doubly-bound shortcuts from the default config? If you can make a change with the UI and not get back to the default, that's confusing and frustrating.

I assume no work has been done on this at all?

(In reply to comment #14)
> I assume no work has been done on this at all?

Nope. We need to re-do the UI, and there were a bunch of related fixes going in last cycle, which is why it didn't move forward.

Thanks for taking the time to report this.
This particular bug has already been reported into our bug tracking system, but please feel free to report any further bugs you find.

*** This bug has been marked as a duplicate of bug 373934 ***

I'll reopen the bug to attach updated patches I picked up from my old local branch. Bug 373934 is clearly related, but:

 - handling existing actions with multiple shortcuts
   more gracefully doesn't imply exposing those shortcuts
   in the UI

 - the current handling is pretty broken and needs fixing,
   while it's still unclear whether we want the feature
   proposed in the other bug

 - fixing this bug still leaves plenty of work on the other
   bug (UI changes, settings migration)

Created attachment 355187 [details] [review]
keyboard: Clear additional bindings when changing a shortcut

Some shortcuts allow multiple bindings for the same actions, which
we mainly use to keep supporting old settings when changing defaults.
Multiple bindings are not exposed in the interface though, so when
changing a shortcut with multiple bindings, we previously only updated
the first one and left additional bindings untouched. This is confusing
however, precisely because additional bindings are not shown in the
UI - for instance, this makes it impossible to actually disable
such a shortcut completely. The less confusing option is to clear
any additional bindings when changing a shortcut.

Created attachment 355188 [details] [review]
keyboard: Add dedicated key combo type

We currently store keyval, keycode and mask that make up a particular
key combo separately. However as we want to consider multiple bindings
for a single item, it makes more sense to combine them in a dedicated
struct type.

Created attachment 355189 [details] [review]
keyboard: Track all key combos

For shortcuts that support more than one binding, we currently simply
ignore all but the first. This makes some sense as we only expose a
single binding in the UI anyway, however it means that any extra
bindings are not taken into account for conflict resolution. In order
to address this in the future, start tracking all bindings of an item.

Created attachment 355190 [details] [review]
keyboard: Refactor finding of conflicting items

When comparing keys for uniqueness, we currently apply various tests to
check whether shortcuts are different, until we decide that we found a
conflict if none of the tests passed. That approach is a bit weird for
shortcuts that have a reverse item - when comparing a binding to two
different shortcuts, it should always be different from at least one of
them, so there should never be a conflict for any reversible shortcuts.
The reason it does work anyway is that reverse items usually only differ
in modifiers, which is_shortcut_different() currently doesn't consider at
all. We are about to change that however, so refactor the code to set the
conflicting item as soon as we find a match rather than as fall-through.

Created attachment 355191 [details] [review]
keyboard: Don't handle mask separately from "rest" binding

Comparing masks is currently part of the early checks we perform to
determine that two bindings are different. There's some convenience
in that, however logically the mask is part of the binding, and
separating the mask check from comparing the "rest" of the binding
makes it harder to extend the comparison to consider multiple bindings.

Created attachment 355192 [details] [review]
keyboard: Consider additional bindings in uniqueness checks

We now have everything in place to extend the uniqueness check to
consider all bindings of an item rather than just the first one.
With this it is finally possible to set Alt+Tab as binding for
"Switch windows" without keeping the hidden Alt+Tab binding of the
"Switch applications" shortcut ...

Created attachment 355193 [details] [review]
keyboard: Consider multiple bindings when resetting

While we are now able to find conflicts for a particular key combo in
non-primary bindings, in the case of resetting a shortcut we need to
check all key combos in case the shortcut itself has multiple bindings
by default.

Review of attachment 355187 [details] [review]:

this means that we might lose some user configuration (manually set with gsettings/dconf) but I agree that this is the lesser of the two evils

Review of attachment 355188 [details] [review]:

looks good

Review of attachment 355189 [details] [review]:

ok

Review of attachment 355190 [details] [review]:

lgtm

Review of attachment 355191 [details] [review]:

code looks good

summary: ... from _the_ "rest" of the binding? or just drop everything after 'separately'

Review of attachment 355192 [details] [review]:

++

Review of attachment 355193 [details] [review]:

this looks fine

shouldn't cc_keyboard_item_is_value_default() be changed to compare all combos too?

(In reply to Rui Matos from comment #25)
> this means that we might lose some user configuration (manually set with
> gsettings/dconf)

Right, that's why the current behavior was chosen originally. Of course that was when multiple bindings were just a hidden feature that users had to set manually rather than something we'd use in default shortcuts ...


(In reply to Rui Matos from comment #31)
> shouldn't cc_keyboard_item_is_value_default() be changed to compare all
> combos too?

I'm not sure. I have a local commit that does that (and with +1/-21 it's quite a nice cleanup), but it is a bit confusing that the reset button may behave differently from setting the value to the (visible part of the) default shortcut. So my conclusion was to leave it out of the patch series, and put it up for discussion afterwards (also gave me an excuse to leave out the commit message for now :-) )

Attachment 355187 [details] pushed as bd54cd1 - keyboard: Clear additional bindings when changing a shortcut
Attachment 355188 [details] pushed as dccf794 - keyboard: Add dedicated key combo type
Attachment 355189 [details] pushed as 5e4fa7b - keyboard: Track all key combos
Attachment 355190 [details] pushed as ab35359 - keyboard: Refactor finding of conflicting items
Attachment 355192 [details] pushed as dd024ae - keyboard: Consider additional bindings in uniqueness checks
Attachment 355193 [details] pushed as 30c36d4 - keyboard: Consider multiple bindings when resetting