Created attachment 234998 [details] [review]
media-keys: Refer key grabbing to the shell

Created attachment 234999 [details] [review]
media-keys: Remove multihead support

Created attachment 235000 [details] [review]
media-keys: Pass appropriate GrabModes to grab_key_unsafe()

Review of attachment 234998 [details] [review]:

::: plugins/media-keys/gsd-media-keys-manager.c
@@ +71,3 @@
 #define GNOME_KEYRING_DBUS_INTERFACE "org.gnome.keyring.Daemon"
 
+

Why the additional linefeed?

@@ +416,3 @@
+
+        if (error) {
+                retry = error->code == G_DBUS_ERROR_UNKNOWN_METHOD;

brackets please:
retry = (error->code == G_DBUS_ERROR_UNKNOWN_METHOD);

@@ +421,3 @@
+                g_error_free (error);
+        } else {
+                retry = key->accel_id == 0;

Ditto.

@@ +425,3 @@
+
+        if (retry)
+                g_timeout_add_seconds (1, retry_grab, data);

Why 1 second? Please move to a #define at the top of the file.

@@ +439,2 @@
+	if (key->accel_id != 0) {
+		shell_key_grabber_call_ungrab_accelerator (manager->priv->key_grabber,

This needs to use a GCancellable.

@@ +446,3 @@
 	tmp = get_key_string (manager, key);
 
+	data = g_new0 (GrabData, 1);

Given the number of those we create, does it make sense to use g_slice_*?

@@ +452,1 @@
+	shell_key_grabber_call_grab_accelerator (manager->priv->key_grabber,

Needs to use a GCancellable as well...

@@ +552,3 @@
                 if (strcmp (key->custom_path, path) == 0) {
                         g_debug ("Removing custom key binding %s", path);
+                        if (key->accel_id)

You already check the value of key->accel_id inside the ungrab_media_key() function.

@@ +623,3 @@
                         continue;
 
+                if (key->accel_id)

Ditto.

@@ +2100,3 @@
+        g_object_get (object, "g-name-owner", &owner, NULL);
+        if (!owner)
+            return;

Indentation.

@@ +2108,2 @@
+                key = g_ptr_array_index (manager->priv->keys, i);
+                grab_media_key (key, manager);

You don't reset key->accel_id so this won't do anything.

@@ +2112,3 @@
 
+static void
+on_accelerator_activated (ShellKeyGrabber     *grabber,

We seem to have lost all the "is this key repeatable" logic.

@@ +2242,3 @@
 
+        manager->priv->key_grabber =
+		shell_key_grabber_proxy_new_for_bus_sync (G_BUS_TYPE_SESSION,

This *needs* to be async, g-s-d and g-s start at the same time, and we don't want to block on gnome-shell being started in the hotpath.

@@ +2337,3 @@
                         key = g_ptr_array_index (manager->priv->keys, i);
 
+                        if (key->accel_id)

Same as before, we check for that value in the ungrab function, and as we're shutting down, is it safe to call async functions? I'd do it async, if at all...

Review of attachment 234999 [details] [review]:

::: plugins/media-keys/gsd-media-keys-manager.c
@@ +176,1 @@
         /* Multihead stuff */

Remove that comment?

@@ +176,2 @@
         /* Multihead stuff */
         GdkScreen       *current_screen;

Renaming to ->screen would be nice as well.

Review of attachment 235000 [details] [review]:

::: plugins/media-keys/gsd-media-keys-manager.c
@@ +511,3 @@
         key = g_new0 (MediaKey, 1);
         key->key_type = CUSTOM_KEY;
+        key->modes = SHELL_KEYBINDING_MODE_NORMAL | SHELL_KEYBINDING_MODE_OVERVIEW;

LAUNCHER_MODES?
Maybe you could rename the define and use it instead of undefining in the header.

Review of attachment 235000 [details] [review]:

::: plugins/media-keys/shortcuts-list.h
@@ +116,3 @@
+        { EMAIL_KEY, "email", NULL, NULL, LAUNCHER_MODES },
+        { SCREENSAVER_KEY, "screensaver", NULL, NULL, SCREENSAVER_MODES },
+        { SCREENSAVER_KEY, NULL, N_("Lock Screen"), "XF86ScreenSaver", SCREENSAVER_MODES },

This needs to work when the shield is down, to lock the screen and require a password.

Created attachment 237679 [details] [review]
media-keys: Refer key grabbing to the shell

(In reply to comment #4)
> Review of attachment 234998 [details] [review]:
> +    data = g_new0 (GrabData, 1);
> 
> Given the number of those we create, does it make sense to use g_slice_*?

We create a lot less now thanks to GrabAccelerators(), but switched to g_slice anyway ...


> @@ +2108,2 @@
> +                key = g_ptr_array_index (manager->priv->keys, i);
> +                grab_media_key (key, manager);
> 
> You don't reset key->accel_id so this won't do anything.

It will, grab_media_key() will try to ungrab first if accel_id is non-zero, and intend a grab in any case.


> @@ +2112,3 @@
> +static void
> +on_accelerator_activated (ShellKeyGrabber     *grabber,
> 
> We seem to have lost all the "is this key repeatable" logic.

Right. This needs to be moved into mutter and exposed in the grab_accelerator() call (e.g. grab(accel, modes) => grab(accel, modes, flags)). Would you be OK with addressing this later? (If not 3.8.0, it should at least make it in 3.8.1)


> Same as before, we check for that value in the ungrab function, and as we're
> shutting down, is it safe to call async functions? I'd do it async, if at
> all...

Good question. As I changed gnome-shell to ungrab all our keys once we leave the bus, it shouldn't matter in practice.

Created attachment 237680 [details] [review]
media-keys: Remove multihead support

Updated according to review.

Created attachment 237681 [details] [review]
media-keys: Pass appropriate GrabModes to grab_accelerator()

Updated according to review.

(In reply to comment #8)
> > @@ +2112,3 @@
> > +static void
> > +on_accelerator_activated (ShellKeyGrabber     *grabber,
> > 
> > We seem to have lost all the "is this key repeatable" logic.
> 
> Right. This needs to be moved into mutter and exposed in the grab_accelerator()
> call (e.g. grab(accel, modes) => grab(accel, modes, flags)). Would you be OK
> with addressing this later? (If not 3.8.0, it should at least make it in 3.8.1)

I should also add that mutter triggers keybindings on KeyPress, so the keybindings that behave differently are the non-repeatable ones ...

Review of attachment 237679 [details] [review]:

::: plugins/media-keys/gsd-media-keys-manager.c
@@ -400,3 @@
-	if (key->key != NULL) {
-		need_flush = TRUE;
-		ungrab_key_unsafe (key->key, manager->priv->screens);

As ->screens won't be used after this patch, could you please remove it in this patch?

@@ +2185,2 @@
+                if (key->key_type == CUSTOM_KEY)
+                        do_custom_action (manager, key, GDK_CURRENT_TIME);

Could you create a new patch that'll pass the device-id to the custom-action keybindings as well?

Review of attachment 237680 [details] [review]:

Ignore my comment in the previous patch.

Review of attachment 237681 [details] [review]:

Looks good.

I would also like another patch to remove all the unused stuff from plugins/common/gsd-keygrab.h

Created attachment 237712 [details] [review]
media-keys: Also pass deviceid for custom actions

(In reply to comment #12)
> Could you create a new patch that'll pass the device-id to the custom-action
> keybindings as well?

Just like this, or is there anything useful I'm supposed to do with it?

Created attachment 237714 [details] [review]
common: Remove now unused key grab functions

(In reply to comment #15)
> I would also like another patch to remove all the unused stuff from
> plugins/common/gsd-keygrab.h

OK - Rui still intends to move the input source switcher into mutter/shell this cycle, at which point everything except for grab_button() will be obsolete. Still, while I was waiting on your feedback on leaving the code removal to this point, the patch was already written ... :)

Review of attachment 237712 [details] [review]:

Looks good, thanks.

Attachment 237679 [details] pushed as b0cee1d - media-keys: Refer key grabbing to the shell
Attachment 237680 [details] pushed as 9e03278 - media-keys: Remove multihead support
Attachment 237681 [details] pushed as 1c9ded7 - media-keys: Pass appropriate GrabModes to grab_accelerator()
Attachment 237712 [details] pushed as 7e6854d - media-keys: Also pass deviceid for custom actions
Attachment 237714 [details] pushed as b7f766d - common: Remove now unused key grab functions

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