General comments.

First of all, thank for this utility library. I think it will be useful for some applications that want to get access to each playlist entry in a easy way.

Using an API that is almost the same as browsing a Grilo source makes it very easy to integrate the library into the application.

I didn't check carefully the code, but just glanced the general feeling. I have some minor comments (see below) to specific points.

Now, the point for me is how to use the library. I see two ways of using it: either applications use it, or plugins use it to expand the playlists.

And after thinking about it, my preference is for the former: make the applications to use it, but not the plugins.

The rationale is the following. I agree with Bastien that expanding a playlist to browse/get access to each item is something that only some applications want to do. So these applications should use the library to expand the playlists.

We could allow plugins to use the library. Granted. But even doing it, at the end applications still need to use the library, because we can't guarantee that all the plugins will use the library to expand the playlists. Maybe someone writes a plugin that don't expand playlists (because as reminder, most of operations in Grilo are optional to the plugin developers). So at the end, application will need to use the library to ensure that even playlists that come from pugins that do not expand them are properly expanded.

Maybe in the future we can bypass this problem by making the Grilo core expanding automatically the playlists. But meanwhile, my suggestion is to use the library in the application side, instead of plugin side.

Now, some (minor) details.

In core:

GRLPLS_LT_VERSION=1:1:1
I would start with 0:0:0

if BUILD_GRILO_NET
SUBDIRS += net pls
endif
pls should be checked separately

No need to add "Since: " tag. I usually add it when I do the release.

In examples, you need to #include <glib/gprintf.h>.
Running browse-playlist example is causing a segmentation fault in grl_pls_entry_free().

In filesystem plugin:
  if (grl_pls_media_is_playlist (bs->container)) {
    grl_pls_browse_by_spec (source, bs);
    return;
  }
This is the only change I see in filesystem plugin, which happens in the browse() function.

I think that change is not enough. The point is that when you browse a folder, if there's a playlist it will returned as a GrlMedia element. So, if you try to browse that playlist it won't reach the filesystem plugin, because it's not a box. I think there must be also code in the operations that when a playlist is found, ensure it is encoded as a GrlMediaBox, so later user can browse it.

In playlist plugin:
I don't see what's the point of that plugin. I fail to see how we  can use it, or which benefits it provides.

(In reply to comment #1)
<snip>
> In playlist plugin:
> I don't see what's the point of that plugin. I fail to see how we  can use it,
> or which benefits it provides.

It's something which I can subclass for watching TV on laptops in my home :)

Free.fr, my ISP, provides a playlist of of streams to watch TV on the local network.

$ gvfs-cat http://mafreebox.freebox.fr/freeboxtv/playlist.m3u | head
#EXTM3U
#EXTINF:0,2 - France 2 (bas débit)
rtsp://mafreebox.freebox.fr/fbxtv_pub/stream?namespace=1&service=201&flavour=ld
#EXTINF:0,2 - France 2 (HD)
rtsp://mafreebox.freebox.fr/fbxtv_pub/stream?namespace=1&service=201&flavour=hd
#EXTINF:0,2 - France 2 (standard)
rtsp://mafreebox.freebox.fr/fbxtv_pub/stream?namespace=1&service=201&flavour=sd
#EXTINF:0,2 - France 2 (auto)
rtsp://mafreebox.freebox.fr/fbxtv_pub/stream?namespace=1&service=201
#EXTINF:0,3 - France 3 (standard)

I might add that the current seems to only support paths for the playlists, or the config option and variables are misnamed. I would really want it to allow remote playlists.

Another use is for Podcasts/Vodcasts, as totem-pl-parser supports RSS and Atom feeds.

Thanks Juan for the feedback, see comments inline

(In reply to comment #1)
> General comments.
> 
> First of all, thank for this utility library. I think it will be useful for
> some applications that want to get access to each playlist entry in a easy way.
> 
> Using an API that is almost the same as browsing a Grilo source makes it very
> easy to integrate the library into the application.
> 
> I didn't check carefully the code, but just glanced the general feeling. I have
> some minor comments (see below) to specific points.
> 
> Now, the point for me is how to use the library. I see two ways of using it:
> either applications use it, or plugins use it to expand the playlists.
> 
> And after thinking about it, my preference is for the former: make the
> applications to use it, but not the plugins.
> 
> The rationale is the following. I agree with Bastien that expanding a playlist
> to browse/get access to each item is something that only some applications want
> to do. So these applications should use the library to expand the playlists.
> 
> We could allow plugins to use the library. Granted. But even doing it, at the
> end applications still need to use the library, because we can't guarantee that
> all the plugins will use the library to expand the playlists. Maybe someone
> writes a plugin that don't expand playlists (because as reminder, most of
> operations in Grilo are optional to the plugin developers). So at the end,
> application will need to use the library to ensure that even playlists that
> come from pugins that do not expand them are properly expanded.

Yeah, I think we can find examples of either way. For example, some applications just want to work with a very limited and controlled set of plugins, but I see what you mean.

> Maybe in the future we can bypass this problem by making the Grilo core
> expanding automatically the playlists. But meanwhile, my suggestion is to use
> the library in the application side, instead of plugin side.

I agree, it's better to wait for this feature/library to mature a bit more and see what use cases we have.

> 
> Now, some (minor) details.
> 
> In core:
> 
> GRLPLS_LT_VERSION=1:1:1
> I would start with 0:0:0

fixed
 
> if BUILD_GRILO_NET
> SUBDIRS += net pls
> endif
> pls should be checked separately

fixed

> No need to add "Since: " tag. I usually add it when I do the release.

fixed

> In examples, you need to #include <glib/gprintf.h>.

fixed

> Running browse-playlist example is causing a segmentation fault in
> grl_pls_entry_free().

fixed, good catch :)

> In filesystem plugin:
>   if (grl_pls_media_is_playlist (bs->container)) {
>     grl_pls_browse_by_spec (source, bs);
>     return;
>   }
> This is the only change I see in filesystem plugin, which happens in the
> browse() function.
> 
> I think that change is not enough. The point is that when you browse a folder,
> if there's a playlist it will returned as a GrlMedia element. So, if you try to
> browse that playlist it won't reach the filesystem plugin, because it's not a
> box. I think there must be also code in the operations that when a playlist is
> found, ensure it is encoded as a GrlMediaBox, so later user can browse it.

You're right, and it's weird since I'm sure I have tried it, I guess I made some mistake. I will fix it.

> In playlist plugin:
> I don't see what's the point of that plugin. I fail to see how we  can use it,
> or which benefits it provides.

It was a request from Bastien, I saw it more a sample of usage for an autonomous minimalistic plugin than a useful plugin as it is. But it seems Bastien has some ideas to expand it to be more useful.

You can find the fixes I did in

http://cgit.collabora.com/git/user/mbatle/grilo.git/log/?h=playlist-lib

(In reply to comment #2)
> (In reply to comment #1)
> <snip>
> > In playlist plugin:
> > I don't see what's the point of that plugin. I fail to see how we  can use it,
> > or which benefits it provides.
> 
> It's something which I can subclass for watching TV on laptops in my home :)

Great !

> Free.fr, my ISP, provides a playlist of of streams to watch TV on the local
> network.
> 
> $ gvfs-cat http://mafreebox.freebox.fr/freeboxtv/playlist.m3u | head
> #EXTM3U
> #EXTINF:0,2 - France 2 (bas débit)
> rtsp://mafreebox.freebox.fr/fbxtv_pub/stream?namespace=1&service=201&flavour=ld
> #EXTINF:0,2 - France 2 (HD)
> rtsp://mafreebox.freebox.fr/fbxtv_pub/stream?namespace=1&service=201&flavour=hd
> #EXTINF:0,2 - France 2 (standard)
> rtsp://mafreebox.freebox.fr/fbxtv_pub/stream?namespace=1&service=201&flavour=sd
> #EXTINF:0,2 - France 2 (auto)
> rtsp://mafreebox.freebox.fr/fbxtv_pub/stream?namespace=1&service=201
> #EXTINF:0,3 - France 3 (standard)
> 
> I might add that the current seems to only support paths for the playlists, or
> the config option and variables are misnamed. I would really want it to allow
> remote playlists.

That is correct, I restricted initially all remote stuff. I wasn't sure if we should allow unrestricted remote access by default, how do you think we should do this ? with some configuration API ?

> Another use is for Podcasts/Vodcasts, as totem-pl-parser supports RSS and Atom
> feeds.

that's very useful for gnome-music, is there any plans for editing playlists?

(In reply to comment #5)
> that's very useful for gnome-music, is there any plans for editing playlists?

This really isn't for non-static playlists. You should create your own provider for that instead.

It doesn't block the implementation of playlist views in gnome-music, because you shouldn't be using this to implement playlists in gnome-music. This is for static playlists, transforming a static playlist (such as a list of radios) into a source.

(In reply to comment #7)
> It doesn't block the implementation of playlist views in gnome-music, because
> you shouldn't be using this to implement playlists in gnome-music. This is for
> static playlists, transforming a static playlist (such as a list of radios)
> into a source.

I've talked to Juan A. Suarez Romero on IRC in #gnome-music and we agreed that grl-pls should add support for adding/removing items (ala podcasts/bookmarks). The tracking bug for that is in https://bugzilla.gnome.org/show_bug.cgi?id=702562

As far as I know, right now grl-pls is able to read a pls and offer the content as if it were obtained from a source. So this makes easier to integrate grl-pls in applications using grilo.

Now, adding some kind of support for creating pls using grl-pls, providing that we use a similar api to what other sources offer, would be great.

I understand that all this stuff is for gnome-music able to read and write pls files on disk. And that "static playlists" means exactly that, read/write playlists from/to disk.

If we are talking about in-memory playlists that gnome-music creates for its internal use to play, I think also that gnome-music should implement his own stuff.

A static playlist is completely ill-suited to using in a music application. Say your playlist had 2000 songs. Do you really want to write about 6000 lines of text (conservative estimate) for each change in the playlist?

Given that you're using Tracker to provide you with the main music library, you would probably use a list of references to the ID used in the tracker source (a custom source similar to bookmarks or podcasts). This would avoid extraneous metadata being set in the playlist, and create something more flexible (queries on presence in playlists).

At the same time, if you still want to implement it using grl-pls (which is based on totem-pl-parser, which I would know quite well ;), you'll see soon enough the limitation of doing that.

(In reply to comment #10)
> A static playlist is completely ill-suited to using in a music application. Say
> your playlist had 2000 songs. Do you really want to write about 6000 lines of
> text (conservative estimate) for each change in the playlist?
> 
> Given that you're using Tracker to provide you with the main music library, you
> would probably use a list of references to the ID used in the tracker source (a
> custom source similar to bookmarks or podcasts). This would avoid extraneous
> metadata being set in the playlist, and create something more flexible (queries
> on presence in playlists).
> 

For some reason I think we are talking about different playlists ideas.

What I mean is the typical "Save as ... " / "Load playlist...", where you can export/import your playlists in PLS or whatever other format. 

For the case of using a playlist internally in gnome-music for the playing queue or whatever, I agree that this is not suited for it.

> At the same time, if you still want to implement it using grl-pls (which is
> based on totem-pl-parser, which I would know quite well ;), you'll see soon
> enough the limitation of doing that.

Well, I just told "would like to" :) Of course, after trying, we will see if we can or not do it. It would be great, but not a must feature.

(In reply to comment #11)
> (In reply to comment #10)
> > A static playlist is completely ill-suited to using in a music application. Say
> > your playlist had 2000 songs. Do you really want to write about 6000 lines of
> > text (conservative estimate) for each change in the playlist?
> > 
> > Given that you're using Tracker to provide you with the main music library, you
> > would probably use a list of references to the ID used in the tracker source (a
> > custom source similar to bookmarks or podcasts). This would avoid extraneous
> > metadata being set in the playlist, and create something more flexible (queries
> > on presence in playlists).
> > 
> 
> For some reason I think we are talking about different playlists ideas.
> 
> What I mean is the typical "Save as ... " / "Load playlist...", where you can
> export/import your playlists in PLS or whatever other format. 

That's not what bug 702562 looks like.

> For the case of using a playlist internally in gnome-music for the playing
> queue or whatever, I agree that this is not suited for it.
> 
> > At the same time, if you still want to implement it using grl-pls (which is
> > based on totem-pl-parser, which I would know quite well ;), you'll see soon
> > enough the limitation of doing that.
> 
> Well, I just told "would like to" :) Of course, after trying, we will see if we
> can or not do it. It would be great, but not a must feature.

I'm not sure it brings much to the table besides avoiding a direct totem-pl-parser dependendy.

Created attachment 254797 [details] [review]
pls: Add new Grilo Playlist lib

I've squashed and rebased the playlist-lib branch against latest master version.

I've also fixed some bugs, and removed some code that shouldn't be pushed upstream (like the example of a source called grl-playlist).

There are some changes I would like to do in the library, but I don't want to do them right now to avoid delaying more the release of Grilo (I'd like to include the library in the next release).

Still, there's a change I'd like to do in the API: get rid of browse_by_spec() function. After thinking about it, I don't see any advantage of having such function exposed publicly.

I would like to have an API similar to what we have to browse sources: browse() and browse_sync()[1], and we already have.

But we don't have a browse_by_spec() function in the sources. I understand that you added that for the case of using the GrlPls directly from a source, so you reuse the spec directly. Well, I think that we really save nothing doing that way and we could be passing directly the parameters with the pls_browse(), as you can get them directly from the spec. And we could take the risk that core frees the spec when the GrlPls is still using it.

So that's my proposal: get rid of grl_pls_browse_by_spec() and move the code directly to the grl_pls_browse().

[1] The changes I mentioned at the beginning are precisely to make the browse to behave even more like the sources work. But this can be done later, because it wouldn't mean a change in the API.

Quite a few problems, shown by my new plugin in bug 720436.

1) I have to call this:
g_object_set_data (G_OBJECT (priv->media), "priv:pls:is_playlist", GUINT_TO_POINTER(1));
because grl_pls_media_is_playlist() only handles local files. I'm happy to add the necessary API to totem-pl-parser though.

2) Without the above call, grilo-test-ui crashes:

+ Trace 232922

3) grl_media_new_from_uri() doesn't support non-file URLs inside the playlist.
totem-pl-parser returns a URI, and a title, which should be enough to show it in the list. The test parser shows:
$ ./parser http://mafreebox.freebox.fr/freeboxtv/playlist.m3u
** Message: Started playlist "http://mafreebox.freebox.fr/freeboxtv/playlist.m3u"...
** Message: 	content-type = 'audio/x-mpegurl'
** Message: Added URI "rtsp://mafreebox.freebox.fr/fbxtv_pub/stream?namespace=1&service=201&flavour=hd"...
** Message: 	title = '2 - France 2 (HD)'
** Message: Added URI "rtsp://mafreebox.freebox.fr/fbxtv_pub/stream?namespace=1&service=201&flavour=sd"...
** Message: 	title = '2 - France 2 (standard)'
** Message: Added URI "rtsp://mafreebox.freebox.fr/fbxtv_pub/stream?namespace=1&service=201&flavour=ld"...
** Message: 	title = '2 - France 2 (bas débit)'
** Message: Added URI "rtsp://mafreebox.freebox.fr/fbxtv_pub/stream?namespace=1&service=201"...
** Message: 	title = '2 - France 2 (auto)'
** Message: Added URI "rtsp://mafreebox.freebox.fr/fbxtv_pub/stream?namespace=1&service=202&flavour=ld"...
** Message: 	title = '3 - France 3 (bas débit)'
[etc.]

There's also no easy way to filter entries on the plugin side.
For example, you can see above 4 different URLs for the "France 2" TV channel, I would trim those and only show the "auto" one. I would also change the title (removing "(auto)") and add logos for the various TV channels.

Definitely, allowing non-file URLs in the playlist would be a good idea.

Created attachment 264495 [details] [review]
pls: Add new Grilo Playlist lib

This utility library allows both plugins and applications to read
playlists as if they were containers, using an API similar to the
Sources.

With additional fixes from Juan A. Suarez Romero and
Bastien Nocera.

Signed-off-by: Juan A. Suarez Romero <jasuarez@igalia.com>

Created attachment 264502 [details] [review]
pls: Add new Grilo Playlist lib

This utility library allows both plugins and applications to read
playlists as if they were containers, using an API similar to the
Sources.

With additional fixes from Juan A. Suarez Romero and
Bastien Nocera.

Signed-off-by: Juan A. Suarez Romero <jasuarez@igalia.com>

Created attachment 264510 [details] [review]
pls: Add new Grilo Playlist lib

This utility library allows both plugins and applications to read
playlists as if they were containers, using an API similar to the
Sources.

With additional fixes from Juan A. Suarez Romero and
Bastien Nocera.

Signed-off-by: Juan A. Suarez Romero <jasuarez@igalia.com>

Created attachment 264545 [details] [review]
pls: Add new Grilo Playlist lib

This utility library allows both plugins and applications to read
playlists as if they were containers, using an API similar to the
Sources.

With additional fixes from Juan A. Suarez Romero and
Bastien Nocera.

Signed-off-by: Juan A. Suarez Romero <jasuarez@igalia.com>

I've fixed the crasher (wrong user_data being passed to the error handler), made use of some helper functions (in particular GError related ones), but there's still some things I'm not happy with.

1) The guess work around what and what isn't a playlist mime-type. There's a number of file types that can be either playlists or video files, as can be seen in totem-pl-parser:
https://git.gnome.org/browse/totem-pl-parser/tree/plparse/totem-pl-parser.c#n222

I would remove all this, and always try to parse the container passed to grl_pls_browse() as a playlist.

2) After this code has landed, I would move the code to populate a GrlMedia from a local file in a helper function, so that the code is shared with the filesystem plugin

3) I'm still trying to understand the use cases behind the library. I'm seeing this as a very easy way to create mostly static sources. Like the Freebox TV source, or other ones I'm interested in writing to show BBC and French public radios. I don't see a use in browsing (recursively!) inside playlists stored on the local filesystem.

Created attachment 264559 [details] [review]
pls: Add new Grilo Playlist lib

This utility library allows both plugins and applications to read
playlists as if they were containers, using an API similar to the
Sources.

With additional fixes from Juan A. Suarez Romero and
Bastien Nocera.

Signed-off-by: Juan A. Suarez Romero <jasuarez@igalia.com>

Created attachment 264560 [details] [review]
filesystem: Use new grl-pls library

The library now handles creating GrlMedia from GFileInfos for us.
We're also using the library to handle playlists files as containers
if the "handle-pls" configuration option is set to TRUE.

Created attachment 264561 [details] [review]
ui-test: Handle playlists in the "/" filesystem

Created attachment 266141 [details] [review]
pls: Add new Grilo Playlist lib

This utility library allows both plugins and applications to read
playlists as if they were containers, using an API similar to the
Sources.

With additional fixes from Juan A. Suarez Romero and
Bastien Nocera.

Signed-off-by: Juan A. Suarez Romero <jasuarez@igalia.com>

Created attachment 267323 [details] [review]
pls: Add new Grilo Playlist lib

This utility library allows both plugins and applications to read
playlists as if they were containers, using an API similar to the
Sources.

With additional fixes from Juan A. Suarez Romero and
Bastien Nocera.

Signed-off-by: Juan A. Suarez Romero <jasuarez@igalia.com>

Created attachment 268573 [details] [review]
pls: Include src/ in introspection

Needed to find Grl-0.2.gir

Created attachment 268601 [details] [review]
filesystem: Use new grl-pls library

The library now handles creating GrlMedia from GFileInfos for us.
We're also using the library to handle playlists files as containers
if the "handle-pls" configuration option is set to TRUE.

https://bugzilla.gnome.org/show_bug.cgi?id=695303

Signed-off-by: Juan A. Suarez Romero <jasuarez@igalia.com>

commit e55699b8574fdf64795df862c7536f1a012fc47d
Author: Bastien Nocera <hadess@hadess.net>
Date:   Thu Dec 19 19:10:11 2013 +0100

    ui-test: Handle playlists in the "/" filesystem
    
    https://bugzilla.gnome.org/show_bug.cgi?id=695303

 tools/grilo-test-ui/main.c | 1 +
 1 file changed, 1 insertion(+)

commit a60fc34bc8c9bbbb92d36782a9834a5f152055b4
Author: Juan A. Suarez Romero <jasuarez@igalia.com>
Date:   Sun Feb 9 14:02:55 2014 +0000

    pls: Include src/ in introspection
    
    Needed to find Grl-0.2.gir
    
    https://bugzilla.gnome.org/show_bug.cgi?id=695303

 libs/pls/Makefile.am | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

commit 50e231075122405bad866191c829470c2f100467
Author: Mateu Batle <mateu.batle@collabora.com>
Date:   Thu Sep 12 19:01:35 2013 +0200

    pls: Add new Grilo Playlist lib
    
    This utility library allows both plugins and applications to read
    playlists as if they were containers, using an API similar to the
    Sources.
    
    With additional fixes from Juan A. Suarez Romero and
    Bastien Nocera.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=695303
    
    Signed-off-by: Bastien Nocera <hadess@hadess.net>
    Signed-off-by: Juan A. Suarez Romero <jasuarez@igalia.com>

 Makefile.am                  |    4 +
 configure.ac                 |   37 +++++++
 doc/grilo/grilo-docs.sgml    |    5 +
 doc/grilo/grilo-sections.txt |   10 ++
 examples/Makefile.am         |    5 +-
 examples/browsing-pls.c      |  246 ++++++++++++++++++++++++++++++++++++++++++
 grilo-pls-0.2.pc.in          |   15 +++
 grilo-pls-uninstalled.pc.in  |   15 +++
 libs/Makefile.am             |    6 +-
 libs/pls/Makefile.am         |   70 ++++++++++++
 libs/pls/grl-pls.c           | 1372 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 libs/pls/grl-pls.h           |   79 ++++++++++++++
 po/POTFILES.in               |    1 +
 13 files changed, 1863 insertions(+), 2 deletions(-)

commit a3b6bafb420336b27ebd69fb73e8213bd3252ca8
Author: Bastien Nocera <hadess@hadess.net>
Date:   Thu Dec 19 19:18:33 2013 +0100

    filesystem: Use new grl-pls library
    
    The library now handles creating GrlMedia from GFileInfos for us.
    We're also using the library to handle playlists files as containers
    if the "handle-pls" configuration option is set to TRUE.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=695303
    
    Signed-off-by: Juan A. Suarez Romero <jasuarez@igalia.com>

 configure.ac                    |  20 ++++++--
 src/filesystem/grl-filesystem.c | 290 ++++++++++++++++----------------------------------------------------------------------------------------------
 src/filesystem/grl-filesystem.h |   1 +
 3 files changed, 60 insertions(+), 251 deletions(-)