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

[Ruben Vermeersch <ruben Lambda1 be>]

On Sat, 2005-09-17 at 21:26 +1000, Jonathan Matthew wrote:
> 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.

Absolutely lovely! Merged in :-)

> 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].

I should have a look at that function, have yet to figure out what it
does & how.

> 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've implemented something like this (basically monitors the appropriate
gconf keys & updates the soup session object with the right proxy
information).

A new patch has been uploaded, still here:

http://files.lambda1.be/linux/patches/rhythmbox-audioscrobbler-libsoup.patch
http://files.lambda1.be/linux/patches/rhythmbox-audioscrobbler-libsoup-new-files.tar.bz2

> 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.

This is quite hard to archieve with the current async model, not sure
how to do this.

> 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.

TODO ;-)

> 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.

Where do you see it appending? It does clear my queue as needed, old
entries are removed correctly (atleast over here).

> 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.

Alex Revo, the original creator of the patch put those there, I'm not
sure we should do anything with them (except maybe for the bad password
case).

> 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.

Agree, it's quite a big hack, and I've only looked at it briefly yet.

> 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.

Regards,
Ruben


--
Ruben Vermeersch (rubenv)
http://www.Lambda1.be/