GNOME Bugzilla – Bug 347097
make iradio a plugin
Last modified: 2006-10-02 11:55:17 UTC
Breaking bug #323057 into two separate pieces...
I'm working on this now. It turns out this is a little more challenging than some of the other plugins because how we handle a) entry-type registration b) uri handling. From a high level I think we need to have a new API: rb_shell_register_source (shell, source); rb_shell_unregister_source (shell, source); This would automatically handle rb_shell_register_entry_type_for_source as well as setting up uri scheme and mime-type handlers. For the type registration we might be able to add a property on RBSource for a list of all provided entry-types. The playlist source has an entry-type property but some sources like Podcast supply more than one entry-type. This property can then be inspected by rb_shell_register_source() and all entry types can be registered. When a source is going to be unloaded (via plugin) all types can be unregistered similarly. We can probably have a similar registry for scheme/mime-type handlers. It might work like: entrytype = rb_shell_guess_type_for_uri (shell, uri); source = rb_shell_get_source_by_entry_type (shell, entrytype); handled = rb_source_add_uri (source, uri, title, genre); Where rb_shell_guess_type_for_uri would perform the lookup in the scheme/mime registry. rb_shell_get_source_by_entry_type would perform the lookup in the entry-type registry. And rb_source_add_uri would be implemented by each source independently.
I've already got a mostly working iradio plugin. What I did was: - add the concept of 'unknown entry types' to rhythmdb-tree, so it doesn't destroy iradio entries if the db is loaded without the iradio plugin active (bug 330226 has a patch for this) - make 'guess_type_for_uri' a vfunc on the entry type, returning a match strength. rb_shell_guess_type_for_uri calls the vfunc on all registered entry types and takes the one that returns the highest value. We can't just do this based on URI scheme, since both podcast feeds and iradio stations use HTTP. Basing it on mime types would probably run into problems with iradio, since mime type checking on shoutcast streams usually just fails. Perhaps we could do a mime type check (ignoring errors) and pass the result into the guess_type_for_uri vfunc. - add a property on RBShellPlayer providing access to the RBPlayer backend (this is also used elsewhere) - move buffering_cb and info_available_cb to the iradio source - I'd started on making 'add_uri' a vfunc on the source, but I don't know how far I got with it. I haven't figured out what to do about the playlist parsing stuff in rb_shell_load_uri. The main tricky case it needs to handle is a radio stream playlist downloaded to the local filesystem, as happens when you click on the playlist link in a browser and have it run rhythmbox.
Created attachment 68756 [details] [review] unfinished patch Oh. Well, here's as far as I got... My approach was to scrap guess_type_for_uri and replace it with guess_source_for_uri. My idea was to let the source be responsible for uri handling. I think this might be better, especially when the uri doesn't map to cleanly to an entry type. As for the scheme/mime registry, I think it might be hard to do weighting correctly without sniffing. Yeah, the playlist parsing stuff is kinda messy.
Created attachment 68757 [details] [review] same patch with no whitespace changes Here's the same patch without whitespace changes. We really should fix bug #344293.
Created attachment 69898 [details] [review] split up rb_shell_add_uri and rb_shell_guess_type_for_uri finally getting back to this.. This removes most of the iradio- and podcast-specific code from rb-shell.c, replacing it with two source vfuncs, want_uri and add_uri. want_uri returns a match strength for a uri for that source, and add_uri adds it. This also makes the playlist handling in rb_shell_load_uri generic. If any source wants all the URIs in the playlist, and also returns TRUE from try_playlist, the URI of the playlist is added rather than its contents. For the iradio source, if the playlist URI is local, we'll then go and parse the playlist again and add its contents.
Created attachment 69941 [details] [review] compile fix Fixes a compilation issue. This looks okay to me, and seems to work.
Committed to cvs. Next step is providing a means to supply extra data for the song display area, such as the streaming song title for iradio or the station name for last.fm. With this, we can move the buffering and info callbacks into the iradio source. After that, there are a few references to RHYTHMDB_ENTRY_TYPE_IRADIO_STATION for audioscrobbler and track transfer. Maybe adding an 'is_stream' flag to the entry type would work here. Or maybe we'd have a few general types of entry - streams (iradio, last.fm), virtual entries (import errors, podcast feeds, ..), and 'regular' entries, for want of a better term.
Created attachment 73176 [details] [review] patch This reimplements all (as far as I know) existing iradio functionality as a plugin. Relies on the extra-metadata patch from bug 345592. I changed some of the song info formatting, mostly to make the stream name smaller than the song name.
@@ -3744,6 +3815,7 @@ rhythmdb_register_core_entry_types (Rhyt podcast_feed_type = rhythmdb_entry_register_type (db, "podcast-feed"); podcast_feed_type->entry_type_data_size = sizeof (RhythmDBPodcastFields); podcast_feed_type->save_to_disk = TRUE; + podcast_post_type->category = RHYTHMDB_ENTRY_VIRTUAL; podcast_feed_type->pre_entry_destroy = (RhythmDBEntryActionFunc) podcast_data_destroy; } This should be "podcast_feed_type->category = RHYTHMDB_ENTRY_VIRTUAL;", right? or _CONTAINER given the comment in the description of that. The patch is also missing plugins/iradio/*. It looks okay to me, although I obviously haven't tested it yet.
Created attachment 73303 [details] [review] better patch I hate it when I do that..
Works fine for me, and could be committed after the upcoming release.
ok, committed.