Created attachment 355178 [details] [review]
media-keys: Fix grabbing of previously disabled shortcuts

If a key is not actually grabbed when we try to ungrab it, we register
the request and ungrab the key after the grab operation has completed.
This avoids leaking bindings when a shortcut is disabled while a grab
operation is in progress, however it means that any grab operation done
after trying to ungrab an unset shortcut will be immediately undone.
This is the currently the case when a shortcut settings changes, which
we handle as a simple combination of ungrab and grab - we need to make
sure to only ungrab a key that has been grabbed previously for the
latter operation to have an effect.

Review of attachment 355178 [details] [review]:

changes to this file should use 8 spaces indentation

::: plugins/media-keys/gsd-media-keys-manager.c
@@ +601,3 @@
                 if (strcmp (settings_key, key->settings_key) == 0) {
+                        if (key->accel_id > 0)
+                          ungrab_media_key (key, manager);

this defeats the purpose. the core issue is that accel_id == 0 means two different things: binding unset and binding set but pending grab.

to fix this we should change the accel_id == 0 in ungrab_media_key() to instead check for the binding presence in keys_pending_grab and the request the ungrab in that case.

(In reply to Rui Matos from comment #2)
> to fix this we should change the accel_id == 0 in ungrab_media_key() to
> instead check for the binding presence in keys_pending_grab and the request
> the ungrab in that case.

Isn't that the same as removing the ungrab_media_key() call altogether? If there's a pending grab, the entry in keys_pending_grab is based on the previous settings value that we no longer have access to, so the lookup for get_key_string() should never find anything in this case ...

(In reply to Florian Müllner from comment #3)
> (In reply to Rui Matos from comment #2)
> > to fix this we should change the accel_id == 0 in ungrab_media_key() to
> > instead check for the binding presence in keys_pending_grab and the request
> > the ungrab in that case.
> 
> Isn't that the same as removing the ungrab_media_key() call altogether? If
> there's a pending grab, the entry in keys_pending_grab is based on the
> previous settings value that we no longer have access to, so the lookup for
> get_key_string() should never find anything in this case ...

ugh. I guess the hash table usage here is wrong then. we're using the binding's combo string as an index but it's written with the assumption that it's the binding's name...

Created attachment 355299 [details] [review]
media-keys: Use hash table keys that uniquely identify a key

We keep track of ongoing and requested grab operations by registering
a key in a corresponding hash table. However currently we use the
key's current binding as hash table key, which is subject to changes.
To not mess up the tracking when a setting changes at the wrong time,
base the hash table keys on immutable properties that uniquely identify
the key.

Created attachment 355300 [details] [review]
media-keys: Fix grabbing of previously disabled shortcuts

If a key is not actually grabbed when we try to ungrab it, we register
the request and ungrab the key after the grab operation has completed.
This avoids leaking bindings when a shortcut is disabled while a grab
operation is in progress, however it means that any grab operation done
after trying to ungrab an unset shortcut will be immediately undone.
This is the currently the case when a shortcut settings changes, which
we handle as a simple combination of ungrab and grab - we need to make
sure to only request a later ungrab if there's a pending grab.

I'm not quite sure about the hash key change - it seems that it can mess up when an existing accelerator is reassigned to a different key?

I wonder whether switching to sync IO is an option here - we don't have any GUI we could block, so off-hand that seems acceptable?

Review of attachment 355299 [details] [review]:

right, this is how we should have been using these hash tables from the beginning

Review of attachment 355300 [details] [review]:

::: plugins/media-keys/gsd-media-keys-manager.c
@@ +586,3 @@
 {
         if (key->accel_id == 0) {
+                key->ungrab_requested = is_pending_grab (key, manager);

in grab_accelerator_complete() we should remove the key from keys_pending_grab before calling ungrab_media_key() otherwise we'll reset ->ungrab_requested erroneously

Created attachment 355960 [details] [review]
media-keys: Fix grabbing of previously disabled shortcuts

If a key is not actually grabbed when we try to ungrab it, we register
the request and ungrab the key after the grab operation has completed.
This avoids leaking bindings when a shortcut is disabled while a grab
operation is in progress, however it means that any grab operation done
after trying to ungrab an unset shortcut will be immediately undone.
This is the currently the case when a shortcut settings changes, which
we handle as a simple combination of ungrab and grab - we need to make
sure to only request a later ungrab if there's a pending grab.

Review of attachment 355960 [details] [review]:

looks good, thanks

Attachment 355299 [details] pushed as 7d73194 - media-keys: Use hash table keys that uniquely identify a key
Attachment 355960 [details] pushed as 19ce2c1 - media-keys: Fix grabbing of previously disabled shortcuts