Created attachment 100799 [details] [review]
Beginnings of playlist support

This patch adds code that loads playlists from the mtp device and makes static playlist sources attached to the mtp source.

The patch also adds code that cleans up the files transferred to /tmp when we play songs 'off the device.'  (i.e. deletes the files we transferred to /tmp)

Created attachment 104758 [details] [review]
updated work....

Here is a patch with playlist support, as well as the beginnings of playlist editing.  There is something buggy in the part of the code that updates the playlist on the device.  but, I'm at a bit of an impasse, so here's what I have for you all to enjoy.

Created attachment 104761 [details] [review]
latest patch

In this patch, adding tracks to playlists works.  RB freezes if you try to remove a track though.

Still need to add playlist creation/deletion

Tempfile cleanups and filesize stat hack do not belong to this bug.

Comment on attachment 104761 [details] [review]
latest patch

needs to be updated for recent changes to the mtp plugin, and to support playlist syncing.

Created attachment 154314 [details] [review]
Updated playlist patch for current rhythmbox.

This is the first version to port the old playlist patch to the current rhythmbox. I remove all none playlist stuff from the old patch.

Created attachment 154317 [details] [review]
MTP playlist support v2.

In the first try the device was wrong referenced. Now compiled and tested with current rhythmbox from repository.

Review of attachment 154317 [details] [review]:

Thanks for updating this patch.  Unfortunately it doesn't really fit in with the way the MTP plugin works now - since libmtp isn't thread safe, we can only access the device directly from a single thread, which is what rb-mtp-thread.{c,h} are for.  For this to work, we'll need device thread functions there to get the list of playlists, get a list of LIBMTP_track_ts for a playlist, and update a playlist on the device.

::: plugins/mtpdevice/rb-mtp-source.c
@@ +1386,3 @@
+		track = LIBMTP_Get_Trackmetadata(priv->device_thread->device,
+						 playlist->tracks[i]);
+		loc = g_strdup_printf ("rb-mtp-%i", track->item_id);

This should be xrbmtp://%i/%s, track->item_id, track->filename .. actually, there should be a single function that formats the location string for an MTP device entry.

You probably need to free the LIBMTP_track_t when you're done with it

@@ +1413,3 @@
+			   "mtp-source", source);
+	g_object_set_data (G_OBJECT (playlist_source),
+			   "mtp-playlist", playlist);

if we're going to attach data to the playlists like this, we should create a subclass of RBStaticPlaylistSource, rather than using GObject data items

@@ +1418,3 @@
+	priv->playlists = g_list_prepend (priv->playlists, playlist_source);
+
+	if (playlist_source != NULL) {

playlist_source cannot be NULL here.

@@ +1435,3 @@
+	GList *ids = NULL, *tmp = NULL;
+
+	g_object_get (G_OBJECT (playlist), "name", &pl->name, NULL);

You don't need the G_OBJECT cast here

@@ +1448,3 @@
+
+	if (ids == NULL) {
+		return FALSE;

Shouldn't we write an empty playlist here, rather than treating this as an error?

@@ +1452,3 @@
+
+	rb_debug ("GList length %d", g_list_length(ids));
+	if (g_list_length (ids) < 1) {

You can't have a non-NULL GList with no entries in it.  This check doesn't do anything.

@@ +1464,3 @@
+	pl->next = oldlist->next;
+
+	if (LIBMTP_Update_Playlist (priv->device_thread->device, pl) != 0) {

again, this needs to be done on the device handling thread, not the main thread

@@ +1489,3 @@
+	}
+
+	if (g_str_has_prefix (rhythmdb_entry_dup_string (entry, RHYTHMDB_PROP_LOCATION), "rb-mtp-")) {

What is this checking for?  How is a different type of entry going to get in here?

Also, this leaks the entry location - rhythmdb_entry_dup_string returns a copy of the string.  You need to use rhythmdb_entry_get_string instead.

@@ +1492,3 @@
+		char *num = strrchr(rhythmdb_entry_dup_string (entry, RHYTHMDB_PROP_LOCATION), '-') + 1;
+		uint32_t track_id = (uint32_t) g_ascii_strtod(num, NULL);
+		*tracks = g_list_append (*tracks, GINT_TO_POINTER(track_id));

This should use the entry_map hashtable instead.  It maps RhythmDBEntry pointers to LIBMTP_track_t pointers, from which you can get the track ID directly.

Thanks for your review. I will follow your advices and send a new patch.

*** Bug 628394 has been marked as a duplicate of this bug. ***

Created attachment 177712 [details] [review]
Recognize mtp playlists

This patch scans for mtp playlists after the initial track scan is done
not all of it it tested because, apparently, my mtp player doesn't
support playlists.  Still lacks ability to add/edit playlists, though
it implements mechanisms for doing so.

-- 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/481.