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

[Jonathan Matthew <jonathan kaolin hn org>]

On Sat, Sep 17, 2005 at 10:08:00PM +0200, Ruben Vermeersch wrote:
> On Sat, 2005-09-17 at 21:26 +1000, Jonathan Matthew wrote:

> > 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 :-)

Thanks.

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

I've fixed this up, as far as I can tell.

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

Great.  I guess the ignore_hosts configuration isn't really important in
this case, but I'm sure someone with a crazy backwards network setup
will appear and complain about it sooner or later..

> > 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've added this by moving the submitted entries to a separate list, and
either freeing them or moving them back to the queue depending on
whether the submission succeeds.

> > 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 ;-)

Actually, I realised that since libsoup uses the glib main loop, threads
aren't necessary at all.  None of the processing is at all intensive,
and the network I/O is done asynchronously.

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

I've since noticed that it does clear the queue properly, so I'm a bit
confused.  After opening the queue file, it seeks to the end of it:

if (result == GNOME_VFS_OK)
	result = gnome_vfs_seek (handle, GNOME_VFS_SEEK_END, 0);

since that doesn't seem to do anything at all, maybe that code should
just be removed.


> > 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 think it's important to provide information like "your profile is not
being updatd because .." even if it's not something the user can fix.
I've noticed it queueing tracks a couple of times, but I have no idea
why, and I don't like that.

My updated copy is here:
http://j.kaolin.hn.org/rhythmbox/rb-audioscrobbler.c
I haven't been using it for all that long (3 songs submitted?), and I've
only checked that it handles the 10 entry limit properly once, but it
does seem to work.

-jonathan