On Thu, 2005-09-01 at 11:35 -0300, Renato Araujo wrote: > Hi all, > > I finally had time to finish the initial podcast support for > rhythmbox. Attached there are the diff against the CVS HEAD and a > tarball with the new files. To test it just apply the patch and > decompress the tarball inside your rhythmbox devel src dir. > Please let me know what you think about it. Any doubt and suggestions > are welcome. I will be glad to fix the bugs and add new features as > needed. Ok cool...some more comments: > + a_val = rhythmdb_entry_get_string (a, data->propid); > + strptime(a_val, "%d/%m/%Y %H:%M", &a_tm); > + a_tm.tm_sec = 0; > + a_time = mktime(&a_tm); This is going to be slow. You should store the date as a regular gulong too, and hook into the rhythmdb property mirroring to use the string as a cache of the gulong property. Look at how mirroring from RHYTHMDB_PROP_LAST_PLAYED to RHYTHMDB_PROP_LAST_PLAYED_STR works. Now on to the new files: rb-podcast-parse.c rb_podcast_parse_date: Need to handle errors from strftime. rb_podcast_parse_load_feed: Couldn't you use gnome_vfs_read_entire_file? Also why not use SAX like rhythmdb-tree.c does instead of reading the whole thing in at once? rb-podcast-download.c rb_podcast_download_copy_file: Calling the rhythmdb functions here from the thread might work, but it's not recommended anymore. Doing the GDK_THREADS_ENTER and g_signal_emit is *definitely* not recommended. We went down the route before of having lots of threads entering the UI, it was just too crazy. I suggest creating a thread which just grabs the data from the network or whatever, and hands it off to the main UI. Actually it looks like you could just use the gnome_vfs async operations here. Alternatively you might look at lib/rb-thread.[ch], which I wrote a while ago as a generic way to run a function in another thread, and ship its result back to the mainloop. It hasn't been heavily tested yet though =) rb-new-podcast-dialog.c: rb_new_podcast_dialog_response_cb Should use gnome_vfs_make_uri_from_input. Also on an overall stylistic note, the patch and new code has a lot of missing spaces before parens; e.g. gtk_widget_hide(GTK_WIDGET(gtkdialog)); should be: gtk_widget_hide (GTK_WIDGET (gtkdialog)); Yeah, it's annoying, but that's been the coding style since before I was involved, and it's important to keep it consistent. Now...onto the user interface. You'll have to bear with me because up until 30 seconds ago when I tried your patch I'd never actually never listened to a podcast. It seems like from browsing the web, the "typical" way is that a web site gives a link, and tells you to copy it to your music software. Is that right? Just giving the user a URL entry box seems kind of unfriendly. Is there anything we can do to make this better? It looks like some web sites support dragging an image from the site into iTunes. I wonder how that works exactly? Maybe metadata embedded in the image data? Could we parse that? It'd be nice at least to at least structure the import podcast dialog like: Podcast information Link (RSS): [ entrybox here ] Updates [X] Download updates for this podcast
Attachment:
signature.asc
Description: This is a digitally signed message part