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