After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 664075 - iPod Playcount Sync
iPod Playcount Sync
Status: RESOLVED OBSOLETE
Product: rhythmbox
Classification: Other
Component: iPod
HEAD
Other Linux
: Normal normal
: ---
Assigned To: RhythmBox Maintainers
RhythmBox Maintainers
Depends on:
Blocks:
 
 
Reported: 2011-11-14 21:52 UTC by Gareth Foster
Modified: 2018-05-24 17:13 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Code sketching ideas for how to sync metadata (1.08 KB, text/plain)
2012-03-15 22:20 UTC, Gareth Foster
  Details
First draft implementation patch (11.76 KB, patch)
2012-05-20 22:50 UTC, Gareth Foster
none Details | Review
Patch that actually works (22.25 KB, patch)
2012-06-05 18:50 UTC, Gareth Foster
needs-work Details | Review

Description Gareth Foster 2011-11-14 21:52:47 UTC
Hi,

On OSX, I had iTunes set up with an iPod playlist, I'd drag the songs I wanted to that, and when I plugged in again and resynced, the play counts would be amended in my iTunes library.

I've done similar on Fedora 16 with Rhythmbox, set up a playlist, set up the iPod so that only that will synch, but the play counts don't seem to transfer over.

Is this a bug?
Comment 1 Gareth Foster 2011-12-07 21:53:16 UTC
The comments in the code say ...

/* figure out what we want on the device and what's already there */
/* figure out what to add to the device */
/* and what to remove */

Non of that suggests that things like playcounts or ratings would be considered in deciding whether there's anything to actually sync.

So, is it a case of "no wonder it doesn't work"? Is it not meant to work like that?
Comment 2 Gareth Foster 2011-12-07 22:27:40 UTC
Seems related to this bug ...

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

Which was closed. Weird. Did somebody break it?

The new function somebody wrote as part of the patch on there is called here, through g_signal_connect_object, whatever that does.

	load_ipod_playlists (source);
	send_offline_plays_notification (source);

	g_signal_connect_object(G_OBJECT(db), "entry-changed",
				G_CALLBACK (rb_ipod_source_entry_changed_cb),
				source, 0);

Oddly, my last.fm account seems to have had the plays appended, yet I can't sync these play counts to my Rhythmbox library.
Comment 3 Jonathan Matthew 2011-12-07 22:31:27 UTC
The play count is (or at least should be) transferred *when the song is transferred*.  The song doesn't get transferred during sync just because the play count differs between the library and the device, and it shouldn't, because that would be stupid.  What you're looking for is an addition to the sync process that just updates metadata.
Comment 4 Gareth Foster 2011-12-08 08:09:08 UTC
I suppose so ... I took the expectation that it would from seeing that that's what iTunes does. If I press sync in the iPod there, play counts are sync'd.

Interestingly on your first point there, I thought of that as a work around, and dragged a song from my iPod to my library. I then ended up with two copies of said song (one would already have been there) and both seemed to have a play count of 0, even though the one on the iPod had been played showed a play count of 1.
Comment 5 Gareth Foster 2011-12-10 12:32:38 UTC
Jonathan, how hard would it be to implement the metadata sync? Could you give me some pointers as to how to do it if it's not going to be too much of a pig?

I'm a competent C/C++ programmer, but the Rhythmbox code base does seem pretty daunting having tried to work this out.
Comment 6 Jonathan Matthew 2011-12-12 22:57:13 UTC
It doesn't look all that difficult.  Add a map (or something) to the sync state object, mapping library db entries to device db entries (matched by the keys in the itinerary and device maps).  Add a vmethod on RBMediaPlayerSource for syncing metadata, and call it with each of the library:device entry pairs during the sync process.

For the generic player source, the sync method would do nothing.  For ipod and mtp sources, it'd have to decide whether to update the library entry or the device entry or both somehow.  I'm not sure exactly what you want to happen, which is probably why I haven't tried to work on this at all.
Comment 7 Liron Tal 2012-01-20 10:09:26 UTC
Hi guys. 
I've implemented this as a python script which syncs ratings and playcount both-ways (Ipod to Rhythmbox and Rhythmbox to Ipod) 

I've done this almost a year ago, but I remember that anyhow I prefererd syncing music in GTKPod and not Rhythmbox. 
I don't remember for what reason but as far as I recall, there was something really annoying happening when trying to sync a playlist from rhythmbox to the Ipod.

Anyhow, I'd be more than happy to help and implement the Metadata sync and improve the playlists sync if needed.
Comment 8 Gareth Foster 2012-03-15 22:19:49 UTC
> Add a map (or something) to the sync state
> object, mapping library db entries to device db entries (matched by the keys 
> in the itinerary and device maps)

In what method should this be populated?

> it'd have to decide whether to update the library entry or the
> device entry or both somehow.  I'm not sure exactly what you want to happen,
> which is probably why I haven't tried to work on this at all.

I've attached some pseudo code to hopefully create some discussion on this.
Comment 9 Gareth Foster 2012-03-15 22:20:46 UTC
Created attachment 209894 [details]
Code sketching ideas for how to sync metadata
Comment 10 Gareth Foster 2012-05-20 22:50:18 UTC
Created attachment 214528 [details] [review]
First draft implementation patch
Comment 11 Gareth Foster 2012-05-20 22:51:06 UTC
I've attached a patch, having tried to get this working.

Can anyone code review it for me?
Comment 12 Gareth Foster 2012-06-05 18:47:51 UTC
Okay, this patch actually works. It updated my library play counts with the higher ones on my iPod.
Comment 13 Gareth Foster 2012-06-05 18:50:03 UTC
Created attachment 215676 [details] [review]
Patch that actually works
Comment 14 Jonathan Matthew 2012-07-25 12:59:36 UTC
Review of attachment 215676 [details] [review]:

I've made some comments here but then I noticed that your diff seems to have diffs on top of itself somehow, so I have no idea what I'm supposed to look at. Fix this and I'll have a proper look at it.

::: plugins/ipod/rb-ipod-source.c
@@ +2062,3 @@
+	GHashTableIter iter;
+	gpointer key, value;
+	GValue val = {0, };

use = G_VALUE_INIT now

@@ +2070,3 @@
+	while (g_hash_table_iter_next (&iter, &key, &value)) {
+		RhythmDBEntry * pLibraryEntry = key;		
+		RhythmDBEntry * pDeviceEntry = value;

should be 'RhythmDBEntry *library_entry' and so on.

@@ +2073,3 @@
+
+		int iLibraryPlayCount = rhythmdb_entry_get_ulong (pLibraryEntry, RHYTHMDB_PROP_PLAY_COUNT);
+		int iDevicePlayCount = rhythmdb_entry_get_ulong (pDeviceEntry, RHYTHMDB_PROP_PLAY_COUNT);

these should be ulongs (since the property is also a ulong) and should not have hungarian notation names.

@@ +2076,3 @@
+
+		/* more plays on the ipod? */
+		if(iDevicePlayCount > iLibraryPlayCount) {

shouldn't this also update the play count on the ipod if the library play count is greater?

@@ +2079,3 @@
+			/* update the playcount! */
+			g_value_set_ulong (&val, iDevicePlayCount);
+			rhythmdb_entry_set (source->priv->db, pLibraryEntry, RHYTHMDB_PROP_PLAY_COUNT, &val);

probably need a rhythmdb_commit after this

@@ +2084,3 @@
+			 *
+			 * lastFmScrobbler->scrobblePlay(deviceSong);
+			 */

this comment seems a bit useless

::: plugins/ipod/rb-ipod-source.h
@@ +72,3 @@
 							 GList *entries);
 
+void			rb_ipod_source_sync_metadata (RBMediaPlayerSource *source, GHashTable *metadata_sync_map);

this doesn't appear to exist, so shouldn't be declared

::: sources/rb-media-player-source.c
@@ +822,3 @@
 sync_cmd (GtkAction *action, RBSource *source)
 {
+	rb_media_player_source_sync_metadata (RB_MEDIA_PLAYER_SOURCE (source));

should just call klass->impl_sync_metadata from sync_idle_cb_update_sync, after the sync update (how does this work now? there isn't necessarily a metadata sync map yet)

::: sources/rb-media-player-source.h
@@ +95,3 @@
 void	rb_media_player_source_sync (RBMediaPlayerSource *source);
 
+void	rb_media_player_source_sync_metadata (RBMediaPlayerSource *source);

this shouldn't exist

::: sources/sync/rb-sync-state.c
@@ +284,3 @@
+ *
+ * @param syncItinerary The hash of everything to be synched.
+ * @param fullDbHash The hash of the full Db, or NULL or that's the same as the above.

I don't like the "NULL if that's the same as the above" bit. GHashTable is refcounted, so we can return separate references to the same hash table.

'full db hash' isn't a great name for what it does.

@@ +321,3 @@
+
+		rb_debug ("adding all playlists to the full db hash");
+		itinerary_insert_some_playlists (state, fullDbHash);

how does this work? shouldn't the full db hash always have all songs and podcasts in it so we can match anything in the library against anything on the device?

@@ +357,3 @@
+	rb_debug ("finished building itinerary hash; has %d entries", g_hash_table_size (syncItinerary));
+
+	/* assign return values to pointers */

this comment is useless
Comment 15 GNOME Infrastructure Team 2018-05-24 17:13:35 UTC
-- 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/1129.