GNOME Bugzilla – Bug 697810
If no media client is currently registered try the first mpris interface
Last modified: 2013-08-01 08:01:38 UTC
If the media-keys plugin gets a Play/Pause/other and we don't have any client registered to handle it, look for an mpris interface and try that. This came up as I'm using spotify and it exposes an mpris media interface but is unlikely ever to register it's media keys with g-s-d. I can write a patch for this if it's wanted.
I would even be willing to ditch the custom code we have, if we had MPRIS support builtin to most core GNOME apps. That means porting Totem's MPRIS plugin to C (so it can be enabled by default), and making sure Rhythmbox' MPRIS plugin is enabled by default. It would also mean that we can implement bug 687831.
Been working on a patch for this, hopefully will have something for you to have a look at next week.
Created attachment 246824 [details] [review] WIP for early comments
Review of attachment 246824 [details] [review]: ::: plugins/media-keys/gsd-media-keys-manager.c @@ +188,3 @@ guint start_idle_id; + + MprisController *mpris_controller; You'll need to unref it on stop. @@ +2337,3 @@ manager, NULL); + g_debug ("Start mpris controll"); "Starting MPRIS controller" ::: plugins/media-keys/mpris-controller.c @@ +44,3 @@ + MprisControllerPrivate *priv = MPRIS_CONTROLLER (object)->priv; + + g_cancellable_cancel (priv->cancel); You still need to unref it. @@ +76,3 @@ + return FALSE; + /* FIXME: PlayPause == Play WFM ?! poss do it manually + * i.e. check playing state/pause state and invert ? That would be useful. PlaybackStatus is a property, so you'd just get the cached value of it. @@ +81,3 @@ + key = "PlayPause"; + + /* we may want to do this sync instead to get a return value if Async callback with no error handling is fine by me. Maybe add a callback and print the error through g_debug()? Makes it easier to test apps. @@ +223,3 @@ + } + + freedesktop_dbus_call_list_names (FREEDESKTOP_DBUS (priv->dbus), This is overly complicated, even as an API. IMO, we could either extend g_bus_watch_name to be able to take in a prefix to a name, rather than a full name, or create an internal API that is similar to g_bus_watch_name(). ::: plugins/media-keys/mpris-controller.h @@ +26,3 @@ +#define MPRIS_TYPE_CONTROLLER mpris_controller_get_type() + +#define MPRIS_CONTROLLER(obj) \ On one line please. @@ +66,3 @@ +MprisController *mpris_controller_new (void); + +gboolean One line as well, and align with the other functions.
Created attachment 249569 [details] [review] wip2 mpris control Adds bus_watch_namespace() and use that from MprisController. The most important advantage of it is that it uses a match rule for org.mpris.MediaPlayer2. I didn't have time to test the patch to mpris-controller.c due to some build issues.
Review of attachment 249569 [details] [review]: I've only reviewed bus-watch-namespace.c. ::: plugins/media-keys/bus-watch-namespace.c @@ +3,3 @@ + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by GPL? @@ +12,3 @@ + * General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License LGPL? Probably should all be under LGPL just incase we want to copy it somewhere later. @@ +70,3 @@ + + g_signal_handlers_disconnect_by_func (watcher->connection, namespace_watcher_stop, watcher); + g_object_unref (watcher->connection); watcher->connection may be NULL here if get_bus() failed. @@ +90,3 @@ + if (reply == NULL) + { + if (!g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) same note about the cancellable handling as the other place @@ +94,3 @@ + g_warning ("bus_watch_namespace: error calling org.freedesktop.DBus.GetNameOwner: %s", error->message); + + /* Remove the name so that we don't call 'vanished' on a name This logic seems potentially flaky. We're here because we made an async call to GetNameOwner. That only happens if we have an entry in the table but didn't emit a appeared() call yet. So far so good. But this entry in the table during our pending GetNameOwner call could have been removed by a NameOwnerChanged signal, resulting in a vanished() call for a name that was never appeared() yet. That seems kinda strange. Why not just dispatch the GetNameOwner signals, in bulk, from the original ListNames reply (ignoring the cases that you already know about, since you'd know about them already only in the case of rare races). Then from this function, check for an entry in the hashtable. Alternatively, you could count the number of outstanding GetNameOwner calls pending and note the incoming signals during this time as well. Then you could emit the apeared() callbacks in bulk when the last one returns and free the hashtable at this point. After that, you just naively report signals as they come. @@ +123,3 @@ + gchar *copied_name; + + /* There's a race between NameOwnerChanged signals arriving and good that you thought to deal with this race. @@ +213,3 @@ + if (reply == NULL) + { + if (!g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) small point of style: would be better to rephrase this to make it look like you're dealing with the "was cancelled" condition first and foremost, with a simple 'g_error_free();return;', perhaps even delaying the setting of watcher = user_data until after this check. handle the "unexpected error/gwarning" case completely separately. @@ +244,3 @@ + if (watcher->connection == NULL) + { + namespace_watcher_stop (watcher); This will free the watcher, but it's still in the hashtable. When it gets removed from the hashtable later, it will be freed again. @@ +248,3 @@ + } + + g_signal_connect_swapped (watcher->connection, "closed", swapped is _slightly_ bad form... @@ +291,3 @@ + g_hash_table_insert (watchers, GUINT_TO_POINTER (watcher->id), watcher); + + g_bus_get (bus_type, NULL, got_bus, watcher); No cancellable used here so if the watcher is destroyed just after being created, this will still complete, causing a crash.... @@ +301,3 @@ + if (watchers) + { + g_hash_table_remove (watchers, GUINT_TO_POINTER (id)); This could indirectly end up calling the vanished handler, which you probably don't want to do as a result of the user asking you to stop calling them.
Created attachment 249583 [details] [review] wip2 mpris control Thanks for the review! I've fixed the race by doing what you suggested: only inserting the name when after we get the owner. The hash table now only contains things that 'appeared' was called for. Fixed the other issues as well.
Created attachment 249585 [details] [review] wip2 mpris control Oops, attaching again with the fixes this time.
Review of attachment 249583 [details] [review]: ::: plugins/media-keys/gsd-media-keys-manager.c @@ +1516,2 @@ if (!have_listeners) { + if (!mpris_controller_key (manager->priv->mpris_controller, On one line please, and use braces, as we have a multi-line condition. @@ +2337,3 @@ manager, NULL); + g_debug ("Start mpris controll"); See original review. ::: plugins/media-keys/mpris-controller.c @@ +23,3 @@ +G_DEFINE_TYPE (MprisController, mpris_controller, G_TYPE_OBJECT) + +#define CONTROLLER_PRIVATE(o) \ No need for linefeed here. @@ +28,3 @@ +struct _MprisControllerPrivate +{ + GCancellable *cancel; cancellable. @@ +41,3 @@ + MprisControllerPrivate *priv = MPRIS_CONTROLLER (object)->priv; + + g_cancellable_cancel (priv->cancel); No need to cancel here, just unref it with g_clear_object, it will cancel. @@ +52,3 @@ + if (other_players) + { + g_slist_free_full (other_players, g_free); g_slist_free_full can take a NULL pointer. @@ +68,3 @@ + g_dbus_proxy_call_finish (G_DBUS_PROXY (object), res, &error); + + if (error) Check on the retval of g_dbus_proxy_call_finish() instead, it will need free'ing anyway. @@ +83,3 @@ + return FALSE; + /* FIXME: PlayPause == Play WFM ?! poss do it manually + * i.e. check playing state/pause state and invert ? Ditto original review. @@ +133,3 @@ + +static gboolean +is_mpris_name (MprisController *self, const gchar *dbus_name) You can remove that, it's unused. @@ +161,3 @@ + + if (priv->mpris_client_proxy && + g_strcmp0 (name, g_dbus_proxy_get_name (priv->mpris_client_proxy)) == 0) Reverse the condition and exit early instead, will make the code more readable.
Comment on attachment 249585 [details] [review] wip2 mpris control Marking as needs-work as per previous reviews.
Created attachment 249688 [details] [review] Fix up of review fixes The checking the status of the player didn't really work as the client i'm testing with (spotify) just ignored the request to play if paused and only responded on "PlayPause".
Created attachment 249689 [details] [review] Changes since attachment 249585 [details] [review] These are the changes fyi which were fixed up from attachment 249585 [details] [review]
Review of attachment 249689 [details] [review]: Can you please squash the patch with the previous one so I can review it again? Make sure to remove the PlayPause hacks. ::: plugins/media-keys/mpris-controller.c @@ +85,3 @@ + + /* We get the "Play" key on keyboards even though the graphic is often a + * playpause so call pause if we're playing. If that's the case, it's a bug that needs to be dealt with at the udev level. See /usr/lib/udev/keymaps/
Review of attachment 249688 [details] [review]: + others I mentioned before. ::: plugins/media-keys/mpris-controller.c @@ +52,3 @@ + if (priv->other_players) + { + g_slist_free_full (priv->other_players, g_free); This works with a NULL list. @@ +67,3 @@ + GVariant *ret; + + if (!(ret = g_dbus_proxy_call_finish (G_DBUS_PROXY (object), res, &error))) Split out the assignment and the conditional. @@ +84,3 @@ + return FALSE; + + /* We get the "Play" key on keyboards even though the graphic is often a Remove hack. @@ +106,3 @@ + }*/ + + g_print ("\ncalling %s over dbus to mpris\n", key); This needs to be debug. @@ +107,3 @@ + + g_print ("\ncalling %s over dbus to mpris\n", key); + g_dbus_proxy_call (priv->mpris_client_proxy, Do the keys really match 1 to 1 with the MPRIS ones? @@ +173,3 @@ + + if (priv->mpris_client_proxy && + g_strcmp0 (name, g_dbus_proxy_get_name (priv->mpris_client_proxy)) == 0) Invert the condition and exit early. @@ +196,3 @@ + +static void +mpris_controller_constructed (GObject *object) There's nothing in constructed you can't do in _init().
Review of attachment 249688 [details] [review]: (again, review limited to the namespace watching parts) This patch is much better. The use of the hashtable is __much__ cleaner now. Still some small issues, though. ::: plugins/media-keys/bus-watch-namespace.c @@ +44,3 @@ + +static guint next_id; +static GHashTable *watchers; I generally prefer if global variables are 'more namespaced' than this. When I see these used in the code below I think that they are locals... namespace_watcher_watchers, namespace_watcher_next_id, maybe? @@ +176,3 @@ + if (reply == NULL) + { + g_warning ("bus_watch_namespace: error calling org.freedesktop.DBus.GetNameOwner: %s", error->message); The most likely case of this occurring is if you got a name during the ListNames stage but it was gone by the time you called GetNameOwner on it. That's pretty harmless, so g_warning() is probably too much here. You could explicitly check for org.freedesktop.DBus.Error.NameHasNoOwner (G_DBUS_ERROR_NAME_HAS_NO_OWNER) and only g_warning() in case it's another error.... @@ +224,3 @@ + if (dbus_name_has_namespace (name, watcher->namespace)) + { + GetNameOwnerData *data = g_new (GetNameOwnerData, 1); this is the sort of thing that gslice is particularly good at.... @@ +337,3 @@ + watcher = g_hash_table_lookup (watchers, GUINT_TO_POINTER (id)); + if (watcher) + namespace_watcher_stop (watcher); I still don't like how this will probably call vanished() a bunch of times... Maybe you should move the vanished calls to connection_closed() since that's the only case where we really want to see them.
Created attachment 250127 [details] [review] wip4-mrpis-control.patch Thanks for the review Ryan. I've fixed your issues in this attachement. (For some reason I can't mark the other one as obsolete.)
Created attachment 250584 [details] [review] media-keys: Add bus namespace watching This adds a bus namespace watching helper, mimicking the g_bus_watch_name() API but watching name prefixes instead of bus names.
Created attachment 250588 [details] [review] media-keys: Use MPRIS when no apps use the media-keys API If the media-keys plugin gets a Play or Pause key events and we don't have any clients registered to handle it through the native media-keys API, look for an MPRIS interface using bus namespace watching and try that. This fixes integration of Spotify for example, as it's unlikely to implement a GNOME specific interface.
Attachment 250584 [details] pushed as 0df8545 - media-keys: Add bus namespace watching Attachment 250588 [details] pushed as cae0fc0 - media-keys: Use MPRIS when no apps use the media-keys API