Re: [Rhythmbox-devel] Last.fm Integration Patch #3

[Jonathan Matthew <jonathan kaolin hn org>]

On Fri, Sep 16, 2005 at 09:32:20PM +0200, Ruben Vermeersch wrote:
> Hi all,
> 
> I've updated the Last.fm (audioscrobbler) patch for rhythmbox. It now
> contains the patch of James Livingstone, to make it use gnome-vfs. Also
> some minor cleanups here & there, but there's still a lot of cleaning up
> to do (as the original code is kinda messy sometimes).

A small problem with the gnome-vfs changes: it doesn't know how to
create the queue file.  If the file doesn't already exist, gnome_vfs_open
fails, and the gnome vfs handle is never set, but we try to close it
anyway, so rb crashes.  Using gnome_vfs_create instead seems to work.

> If anyone would like to review/test it (applies cleanly to HEAD), it can
> still be found here:
> 
> http://files.lambda1.be/linux/patches/rhythmbox-audioscrobbler-libsoup.patch
> http://files.lambda1.be/linux/patches/rhythmbox-audioscrobbler-libsoup-new-files.tar.bz2
> 
> If someone sees things that should be corrected, please let me know, any
> hints on getting this code in shape would be greatly appreciated. Teuf:
> I just read on IRC you wanted a cleaned version, here it is, largely
> unmodified ;-)

Rather than tracking the active source and watching its entry view to
figure out the playing song, it should just use the RBShellPlayer
playing-entry-changed signal, which does exactly what you want in much
less code.

Rather than comparing album/artist/title strings with "Unknown", I think
you need to use the translated equivalent.

There are a few unnecessary string duplications in rb-audioscrobbler.c,
particularly in rb_audioscrobbler_song_changed_cb.

I've made these changes myself:
http://j.kaolin.hn.org/rhythmbox/rb-audioscrobbler.h
http://j.kaolin.hn.org/rhythmbox/rb-audioscrobbler.c

and I've been happily using it for a while now.  Here ends the list of
things I've fixed myself.

rb_audioscrobbler_parse_response might have some problems with
maliciously formed responses (don't say it won't happen), where it might
read past the end of the 'breaks' array when referencing breaks[i+2].

HTTP proxy support would be good.  I don't think libsoup knows about the
proxy information stored in gconf under /system/http_proxy, but that'd
be the thing to use.  Finding a sensible way to use http-proxy.c from
gnome-vfs (or just stealing it) would do this. 

I think there might need to be a limit on the number of entries
submitted in a single HTTP request -
http://www.audioscrobbler.net/wiki/Protocol1.1 says "If you try to
submit more than 10 tracks at once, some of them may not be accepted.",
so I guess that's a sensible limit.

I think the thread creation could be a bit lazier - most of the checks
done at the start of rb_audioscrobbler_thread_main (except for the
handshake bit) could be done in the main thread, so we'd only create a
thread once per song, rather than once every 15 seconds.  This isn't a
big deal, but threads are a wonderful source of bugs, and so they should
be avoided wherever practical.

I'm not sure the queue handling is right, but I haven't done any testing
to be sure about it.  It seems that rb_audioscrobbler_save_queue only
appends to the queue file, so entries written to the queue file will
never be removed.  I'm not sure it should just overwrite the queue file
either, but I don't know why I think that.

Regarding the "TODO: how to alert user?" comments, I think the import
error dialog box, or something like it, should be used for errors like
this.  It shouldn't automatically display itself, but it could add a
button to the status bar when there are new errors the user hasn't seen
before.  There could be an item in the view menu and also the tray icon
menu to show the dialog box, which would also indicate whether there
were new errors or not.

I don't really like the way the audioscrobbler object is handled, and in
particular the way the preference page is added, but that's not your
fault; rhythmbox doesn't provide a way to do this properly yet.  What I
think we need, rather than a "plugin system", is a way for features like
this to be added without sprinkling code all over rb-shell.c.

Overall I think this is looking pretty good, and most of the issues I
described above shouldn't be hard to fix.  The last two items are things
that need discussion, and others will probably have better ideas.

-jonathan