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 _______________________________________________ rhythmbox-devel mailing list [email protected] http://mail.gnome.org/mailman/listinfo/rhythmbox-devel
