Created attachment 116893 [details] [review]
patch to enable goear plugin in rhythmbox.

Created attachment 116894 [details]
goear icon

A couple of style things:
- Return statements at the end of void functions look really weird
- The functions in rb-goear-utils.h need much more specific names

and more seriously:
- We really need to avoid synchronous http requests on the main thread
- The source should not construct its own search box.  It should use the RBSource search method instead.
- I'm not sure why get_playback_uri ends up using the metadata reader, but it doesn't seem like a good idea to me.  The basic metadata will be read from the stream during playback, and the other stuff probably isn't very useful here.
- I'd suggest using gio rather than libsoup, maybe

Can you write up an explanation of how this works, preferably as a comment in the code?  It's not particularly clear.

Is this sort of usage acceptable under the site's terms and conditions?

One more thing: the icon should be themeable.  See the visualizer plugin for an example of how to do that.

(In reply to comment #3)
> A couple of style things:
> - Return statements at the end of void functions look really weird

:) they're a personal taste

> - The functions in rb-goear-utils.h need much more specific names

Yeah, you're right. The rational of that file was to move out the helper functions which didn't have hard dependencies with th RBGoearSource object.

> and more seriously:
> - We really need to avoid synchronous http requests on the main thread

I'm agree, but I need to fetch the XML stream descriptor synchronously in order to provide the stream URI when the get_playback_uri callback is called. In that way bandwidth is save because the XML descriptor is called only when the user ask to play a specific song, and not all the available play list. So, I didn't figure out how to fetch the XML descriptor asynchronously in the get_playback_uri callback.

> - The source should not construct its own search box.  It should use the
> RBSource search method instead.

I played with the search box but it didn't work for me: it does incremental searches on a local database. But in this case the database is remote, so incremental search (a search each time a user type a key) is just crazy. And again, I couldn't figure out how to disable the incremental search in the search box, so I wrote my own.

> - I'm not sure why get_playback_uri ends up using the metadata reader, but it
> doesn't seem like a good idea to me.  The basic metadata will be read from the
> stream during playback, and the other stuff probably isn't very useful here.

I'm agree, but I didn't find how to add the metadata info in the RhythmDB during the playback. I don't like the burden added by the metadata reader, but I found quite useful to see the metadata in the browser.

> - I'd suggest using gio rather than libsoup, maybe

What are the advantages to use gio instead of libsoup? Avoid a dependency?

> Can you write up an explanation of how this works, preferably as a comment in
> the code?  It's not particularly clear.

goear.com is a youtube like site for mp3 streams. This plugin searches on that site, parses the html ouput and fills the browser. When the user plays a song, the plugin fetches the xml descriptor of the song to get the stream URI and then playback the stream.
 
> Is this sort of usage acceptable under the site's terms and conditions?

The terms and conditions are extremely ambiguous (http://www.goear.com/pages/terms_and_conditions.html), but the last paragraph (in Spanish) seems to imply that this type of usage is not acceptable :S 

Thank you very much for your input.

(In reply to comment #5)
 
> > - The source should not construct its own search box.  It should use the
> > RBSource search method instead.
> 
> I played with the search box but it didn't work for me: it does incremental
> searches on a local database. But in this case the database is remote, so
> incremental search (a search each time a user type a key) is just crazy. And
> again, I couldn't figure out how to disable the incremental search in the
> search box, so I wrote my own.

That's the wrong way to go about it.  If the search box doesn't work for you, then we need to find a way to extend it to make it usable for more types of source.  It's a pretty safe bet that someone else is going to make a similar plugin for some other site.
 
> > - I'm not sure why get_playback_uri ends up using the metadata reader, but it
> > doesn't seem like a good idea to me.  The basic metadata will be read from the
> > stream during playback, and the other stuff probably isn't very useful here.
> 
> I'm agree, but I didn't find how to add the metadata info in the RhythmDB
> during the playback. I don't like the burden added by the metadata reader, but
> I found quite useful to see the metadata in the browser.

You should be able to use the RBPlayer 'info' signal instead.
 
> > - I'd suggest using gio rather than libsoup, maybe
> 
> What are the advantages to use gio instead of libsoup? Avoid a dependency?

The interface you've defined in rb-goear-http isn't much different to GFile, and gio handles the proxy configuration stuff by itself.

> > Can you write up an explanation of how this works, preferably as a comment in
> > the code?  It's not particularly clear.
> 
> goear.com is a youtube like site for mp3 streams. This plugin searches on that
> site, parses the html ouput and fills the browser. When the user plays a song,
> the plugin fetches the xml descriptor of the song to get the stream URI and
> then playback the stream.

I was after more of a technical description - which documents does it fetch, what do they look like, and how are the URIs constructed?  If anyone else is going to touch this code, it's better that they have some information to work with, rather than having to piece it together by reading the code and looking at
the various html and xml files.  This sort of detail belongs in the code.

> > Is this sort of usage acceptable under the site's terms and conditions?
> 
> The terms and conditions are extremely ambiguous
> (http://www.goear.com/pages/terms_and_conditions.html), but the last paragraph
> (in Spanish) seems to imply that this type of usage is not acceptable :S 

If the site doesn't allow this sort of use, then I don't think we should distribute the plugin.

Hi Jonathan,

Sorry for this so delayed reply :(

> > > - The source should not construct its own search box.  It should use the
> > > RBSource search method instead.
> > 
> > I played with the search box but it didn't work for me: it does incremental
> > searches on a local database. But in this case the database is remote, so
> > incremental search (a search each time a user type a key) is just crazy. And
> > again, I couldn't figure out how to disable the incremental search in the
> > search box, so I wrote my own.
> 
> That's the wrong way to go about it.  If the search box doesn't work for you,
> then we need to find a way to extend it to make it usable for more types of
> source.  It's a pretty safe bet that someone else is going to make a similar
> plugin for some other site.

Well, it means basically I'll need to patch it in order to add a property to disable the time-out signal and just attend the activate signal. Is it that ok for you?
 
> > > - I'm not sure why get_playback_uri ends up using the metadata reader, but it
> > > doesn't seem like a good idea to me.  The basic metadata will be read from the
> > > stream during playback, and the other stuff probably isn't very useful here.
> > 
> > I'm agree, but I didn't find how to add the metadata info in the RhythmDB
> > during the playback. I don't like the burden added by the metadata reader, but
> > I found quite useful to see the metadata in the browser.
> 
> You should be able to use the RBPlayer 'info' signal instead.

I did some test, and the 'info' signal is not enough in this case, because I need to know the duration of the stream before goes to PAUSE state, otherwise the seeking will be disabled. Furthermore, the info signal doesn't provide all the data provided by the stream tags.
 
> > > - I'd suggest using gio rather than libsoup, maybe
> > 
> > What are the advantages to use gio instead of libsoup? Avoid a dependency?
> 
> The interface you've defined in rb-goear-http isn't much different to GFile,
> and gio handles the proxy configuration stuff by itself.

Also checked on this, and gio use libsoup beneath of it in http operations. So I don't feel necessary add a new layer for this.
 
> > > Is this sort of usage acceptable under the site's terms and conditions?
> > 
> > The terms and conditions are extremely ambiguous
> > (http://www.goear.com/pages/terms_and_conditions.html), but the last paragraph
> > (in Spanish) seems to imply that this type of usage is not acceptable :S 
> 
> If the site doesn't allow this sort of use, then I don't think we should
> distribute the plugin.

Yeah :( anyway, has been a nice exercise :)

Thank you very much.

(In reply to comment #7)
> > That's the wrong way to go about it.  If the search box doesn't work for you,
> > then we need to find a way to extend it to make it usable for more types of
> > source.  It's a pretty safe bet that someone else is going to make a similar
> > plugin for some other site.
> 
> Well, it means basically I'll need to patch it in order to add a property to
> disable the time-out signal and just attend the activate signal. Is it that ok
> for you?

Off hand, I'd say that extending the 'can_search' source method would be the way to do it.  Currently, the source indicates whether it supports searching at all.  Perhaps it should return 'no search' - hide the search bar; 'instant search' - current behaviour; or 'explicit search' - only search when the search box is activated.  We'd need some visible indicator of the search type, but I don't have any specific ideas.



> > You should be able to use the RBPlayer 'info' signal instead.
> 
> I did some test, and the 'info' signal is not enough in this case, because I
> need to know the duration of the stream before goes to PAUSE state, otherwise
> the seeking will be disabled. Furthermore, the info signal doesn't provide all
> the data provided by the stream tags.

Again, if something in rhythmbox is insufficient for your needs, then we should fix that, rather than work around it some other way.  I'm not sure what to do about the duration, though.
 
> > > > - I'd suggest using gio rather than libsoup, maybe
> > > 
> > > What are the advantages to use gio instead of libsoup? Avoid a dependency?
> > 
> > The interface you've defined in rb-goear-http isn't much different to GFile,
> > and gio handles the proxy configuration stuff by itself.
> 
> Also checked on this, and gio use libsoup beneath of it in http operations. So
> I don't feel necessary add a new layer for this.

You're adding another layer either way.  If you use gio, you don't have to write and maintain that layer yourself.

> > > > Is this sort of usage acceptable under the site's terms and conditions?
> > > 
> > > The terms and conditions are extremely ambiguous
> > > (http://www.goear.com/pages/terms_and_conditions.html), but the last paragraph
> > > (in Spanish) seems to imply that this type of usage is not acceptable :S 
> > 
> > If the site doesn't allow this sort of use, then I don't think we should
> > distribute the plugin.
> 
> Yeah :( anyway, has been a nice exercise :)

It's not really possible to distribute plugins written in C separately from rhythmbox, but that could change.

Let's close this bug, since it should belong to grilo now