Created attachment 200886 [details] [review]
First iteration of playlist containers.

The code in this patch is awful (as commit message says), because already existing classes couldn't be reused - they are too specific. For generalization some steps could be done:
1. Make Rygel.Tracker.RootContainer hold an array of all item factories.
2. Refactor Rygel.Tracker.MetadataValues class, so it would take not a key chain, but rather a key tree (maybe just 2D array of strings). Also it would use parent's array of factories instead of just a single factory.
3. Refactor Rygel.Tracker.SearchContainer to use array of factories, check rdf:type of each url it gets and use proper factory to create its children.
4. With the steps above done Music, Videos and other containers should be adapted. Playlists container code could be then very shortened.

Review of attachment 200886 [details] [review]:

I'm not sure if your refactoring suggestion is a bit over-engineering; tbh I didn't think of mixed playlists or even playlists containing other things than music.

Also the PlaylistContainer is rather the "AllPlaylistsContainer". Why didn't you introduce a separate playlist container that deals with its children (and does that lazily) instead of filling the whole tree in one go.

Also listening for update notification (added/removed playlists) is missing (see other "All" containers).

::: src/plugins/tracker/rygel-tracker-playlists.vala
@@ +54,3 @@
+    }
+
+    internal async void fetch_metadata_values () {

This method is way too long and I struggle to understand it.

@@ +142,3 @@
+            child_triplets.add (new QueryTriplet ("?file_uri",
+                                                  "nie:url",
+                                                  "?nfo_entryUrl"));

I'm not sure why you use the on-disk uri here instead of the tracker URN and what you need the ?nfo_listPosition for.

@@ +271,3 @@
+        return value;
+    }
+

I don't like that duplication here, but you already said that. Would have been better to extract a common base class for this and SearchContainer including the DBus stuff. That also would have avoided the need for using the generic MediaContainer in the item factories.

It's not as good as the grand refactoring, but well.

::: src/plugins/tracker/rygel-tracker-root-container.vala
@@ +47,3 @@
         }
+
+        if (this.get_bool_config_without_error ("share-playlists")) {

If you add configuration keys it would be great if you also update the configuration in doc/man/rygel.conf.xml.

-- 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/rygel/issues/5.