After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 347097 - make iradio a plugin
make iradio a plugin
Status: RESOLVED FIXED
Product: rhythmbox
Classification: Other
Component: Internet Radio
HEAD
Other Linux
: Normal normal
: ---
Assigned To: RhythmBox Maintainers
RhythmBox Maintainers
Depends on:
Blocks:
 
 
Reported: 2006-07-10 14:20 UTC by William Jon McCann
Modified: 2006-10-02 11:55 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
unfinished patch (162.57 KB, patch)
2006-07-10 22:56 UTC, William Jon McCann
none Details | Review
same patch with no whitespace changes (58.00 KB, patch)
2006-07-10 23:00 UTC, William Jon McCann
none Details | Review
split up rb_shell_add_uri and rb_shell_guess_type_for_uri (22.35 KB, patch)
2006-07-30 05:18 UTC, Jonathan Matthew
none Details | Review
compile fix (27.23 KB, patch)
2006-07-31 05:51 UTC, James "Doc" Livingston
committed Details | Review
patch (62.51 KB, patch)
2006-09-21 22:40 UTC, Jonathan Matthew
none Details | Review
better patch (58.57 KB, patch)
2006-09-24 08:36 UTC, Jonathan Matthew
committed Details | Review

Description William Jon McCann 2006-07-10 14:20:14 UTC
Breaking bug #323057 into two separate pieces...
Comment 1 William Jon McCann 2006-07-10 16:35:54 UTC
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.
Comment 2 Jonathan Matthew 2006-07-10 22:36:09 UTC
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.
Comment 3 William Jon McCann 2006-07-10 22:56:38 UTC
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.
Comment 4 William Jon McCann 2006-07-10 23:00:17 UTC
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.
Comment 5 Jonathan Matthew 2006-07-30 05:18:25 UTC
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.
Comment 6 James "Doc" Livingston 2006-07-31 05:51:47 UTC
Created attachment 69941 [details] [review]
compile fix

Fixes a compilation issue.

This looks okay to me, and seems to work.
Comment 7 Jonathan Matthew 2006-07-31 12:08:40 UTC
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.
Comment 8 Jonathan Matthew 2006-09-21 22:40:57 UTC
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.
Comment 9 James "Doc" Livingston 2006-09-24 06:23:54 UTC
@@ -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.
Comment 10 Jonathan Matthew 2006-09-24 08:36:38 UTC
Created attachment 73303 [details] [review]
better patch

I hate it when I do that..
Comment 11 James "Doc" Livingston 2006-09-24 12:15:43 UTC
Works fine for me, and could be committed after the upcoming release.
Comment 12 Jonathan Matthew 2006-10-02 11:55:17 UTC
ok, committed.