Created attachment 94435 [details] [review]
Makes Rhythmbox stream radio directly, rather than through server.

Tested on Rhythmbox 0.10.0 and it fixes the problem for me.

I don't think setting the entry location is the right way to do this, since there's the possibility of collisions.  Entry locations must be unique across all entries.

Instead, the location from the ASUL property should be stored under another property (maybe RHYTHMDB_PROP_MOUNTPOINT, since it's not otherwise used) and there should be a get_playback_uri function for the DAAP entry type that returns the value of the mount point property if present, and the location otherwise.

I think you might be right.

There's actually a secondary problem I'm working on which is the lack of interaction between DAAP radio and the iRadio (I think) plug-in. Because the entry is not in the special Radio playlist, there are no features such as streaming titles in playback.

This could be quite tricky to engineer around. I'll have a think.

Created attachment 94793 [details] [review]
Revision of previous patch, stream radio directly and not through DAAP server.

This is a revised patch based on the above comments.

I was holding off on this until I'd figured out the right way to enable streaming titles on DAAP-shared radio, but I think the two patches will end up being independent. This one fixes the crash anyway.

(I hooked onto RHYTHMDB_PROP_MOUNTPOINT to store the stream URI. Could create a new entry if that's tidier.)

Committed to svn.  Thanks for your work on this.  Since there's still the issue of somehow playing DAAP-served radio streams like local ones, I'm retitling this bug and lowering its severity.

Created attachment 101922 [details] [review]
Introduce streaming title functionality to DAAP-served radio streams

This patch resolves the remaining (lack of) streaming titles issue for radio streams served by a DAAP server.

It duplicates a small amount of code from the iradio plugin, unfortunately, which appears to be a core feature of Rhythmbox now but is still isolated into a dynamic library. The correct design here would have been to make use of rb-streaming-source.c but this does not fulfil all of the functionality that the main DAAP view (rb-browser-source.c) and sub-playlists (rb-static-playlist-source.c) need. Since there does not appear to be a multiple inheritance mechanism available, I have just replicated the required functionality into the DAAP plugin.

The streaming source class probably doesn't actually need to be a class, it could probably be converted into an adjunct object that provided extra-metadata support for any source that wanted it.  It could also convert signals from the player backend (as the iradio source does) into extra-metadata.

I'm more than happy to give this a shot, to get the patch into the best condition possible. I am not terribly familiar with the GLib architecture, however, although I am well-versed in object oriented systems.

Could you elaborate on what such an object would physically look like? E.g. in C++ I could implement this functionality through multiple inheritance (with a streaming superclass) or through an additional member object providing an interface to communicate streaming information (and signals, etc.) through. Is the latter similar to what you're referring?

I'm not entirely sure what it would look like myself.  Probably more like the latter.

Created attachment 102176 [details] [review]
(Revision 2) Introduce streaming title functionality to DAAP-served radio streams

This is a completely revised patch to eliminate the redundancy in 101922.

RBStreamingSource is completely replaced by a helper class shared between plugins/audioscrobbler, plugins/daap and plugins/iradio. Most functionality is tested lightly and working; the streaming metadata rhythmdb_commit functionality is untested as I have no streams providing this information. However, it is a line-by-line copy of the original code.

That's pretty much what I had in mind.

rb-streaming-signal-handler.* should still be in sources/, not lib/, despite not actually being a source.

rb_streaming_signal_handler_free shouldn't exist.  Data allocated by an object should be freed in its dispose or finalize method, and sources that own one of these objects should just unref it when they're done.

I don't really like the additional_sources thing.  Is there a reason that the additional sources can't have their own streaming signal handler object?

Created attachment 102632 [details] [review]
(Revision 3) Introduce streaming title functionality to DAAP-served radio streams

This revision addresses your second and third points.

Placing rb-streaming-signal-handler.c in sources/ does not appear to be possible. The symbols inside must be exported for plugins to link to. This works correctly for librhythmbox-core (itself a shared library) but symbols are lost in the static linking of libsources.a to the rhythmbox binary; -export-dynamic does not export unused symbols from object archives.

Some symbols from libsources.a are, in fact, used by plugins. This only works because they are linked in to resolve unsatisfied dependencies in python/bindings and shell/. Neither approach is appropriate for this patch. I think lib/ is the best place for it.

There are two libraries built out of sources/, libsources.a and libsourcesimpl.a.  libsourcesimpl.a is statically linked into the main binary, and libsources.a goes into librhythmbox-core.so just like librb.a does.  The streaming signal handler can go in libsources.a without any problems.

The python bindings need to be updated.

The dispose method for RBStreamingSignalHandler is wrong in a couple of ways:
- dispose methods can be called multiple times, so if they unref objects, they have to do it like this:
   if (object->priv->ref != NULL) {
        g_object_unref (object->priv->ref);
        object->priv->ref = NULL;
   }
- they also shouldn't free allocated memory; that should be done in the finalize method
- dispose and finalize methods must chain up to the parent class implementation, which looks like this usually: "G_OBJECT_CLASS(rb_streaming_signal_handler_parent_class)->dispose (object);"

The GObject tutorial (http://library.gnome.org/devel/gobject/stable/howto-gobject.html) has more information on this.

The way the construction parameters to rb_streaming_signal_handler_new are handled isn't very GObject-y; they should be GObject properties, which makes them more binding-friendly.  There is a fair bit of cut-and-paste code involved in doing this, though.  Most of the work currently done in _new would be moved to the GObject _constructor method.

It seems backwards, but the comments describing how to use the class actually go in the .c file, rather than the header.  If we then do some magic somewhere else, they get picked up and included in the generated API documentation.

Checking the mountpoint property to determine if something is a stream is a bit awkward.  It's bad enough that we're abusing the property like this within a few sources.  I think a more reasonable way to do this would be to have the streaming signal handler emit a new signal ("is-stream", perhaps) and check the return value.  The default signal handler would return TRUE, but (for instance) DAAP sources could provide a handler that did whatever checks were required for DAAP entries.

It's starting to look like we'll need a new playlist subclass for DAAP playlists.

Created attachment 104945 [details] [review]
updated patch

I've fixed this enough to get it to build, and updated the python bindings (untested).

Review of attachment 104945 [details] [review]:

earlier comments still apply (mostly)

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/rhythmbox/issues/424.