GNOME Bugzilla – Bug 694981
Port from gnome-keyring to libsecret
Last modified: 2013-03-25 21:50:25 UTC
Created attachment 237776 [details] [review] Port audioscrobbler and daap plugins Hello, I'll be attaching some patches to port Rhythmbox's plugins from gnome-keyring to libsecret. The first patch ports the C plugins daap and audioscrobbler. I've been using this patch for a month, and it seems to be working great so far. A patch for magnatune will follow soon.
Created attachment 237777 [details] [review] Port audioscrobbler and daap plugins I attached an old patch above. Here's the version of the patch ported to current master.
Created attachment 237779 [details] [review] Port magnatune plugin
Review of attachment 237777 [details] [review]: I'm pretty sure the 'old API' referenced in the audioscrobbler code is no longer supported by last.fm or libre.fm, so that code should just be removed. ::: configure.ac @@ +228,3 @@ +AC_ARG_WITH(keyring, + AC_HELP_STRING([--with-keyring], + [Enable keyring support using libsecret]),, I think this should probably be --with-libsecret etc. rather than --with-keyring
Review of attachment 237779 [details] [review]: ::: plugins/magnatune/MagnatuneAccount.py @@ +26,3 @@ # Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. +from gi.repository import Gio, Secret, SecretUnstable We should really handle the case where libsecret isn't installed or isn't new enough here. I'm not sure exactly what we should do though.
Created attachment 238068 [details] [review] Port magnatune and daap plugins, and remove gnome-keyring from audioscrobbler (In reply to comment #3) > Review of attachment 237777 [details] [review]: > > I'm pretty sure the 'old API' referenced in the audioscrobbler code is no > longer supported by last.fm or libre.fm, so that code should just be removed. > This patch removes that — I'm not sure if I caught all the obsolete code though, could you verify that? > ::: configure.ac > @@ +228,3 @@ > +AC_ARG_WITH(keyring, > + AC_HELP_STRING([--with-keyring], > + [Enable keyring support using libsecret]),, > > I think this should probably be --with-libsecret etc. rather than > --with-keyring Fixed! (In reply to comment #4) > Review of attachment 237779 [details] [review]: > > ::: plugins/magnatune/MagnatuneAccount.py > @@ +26,3 @@ > # Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. > > +from gi.repository import Gio, Secret, SecretUnstable > > We should really handle the case where libsecret isn't installed or isn't new > enough here. I'm not sure exactly what we should do though. The attached version of the patch skips searching/storing if libsecret couldn't be found, and prints errors to the console. However, the plugin continues to work. Maybe this is a decent compromise till libsecret becomes prevalent enough? Also, I tried to find a way to detect the libsecret version through the introspection API, but I could only get at the API version (which is useless). Maybe we should just rely on configure.ac to get the version right for us.
All looks good, pushed as commit 1fac0aa. Thanks for working on this.