Created attachment 83328 [details] [review]
73ipod-dont-make-duplicates.patch

As above.

Tested; seems to work OK.

Filters iPod additions if a match exists with title, artist and album.

Mechanism: 
gboolean RBRemovableMediaSourceClass->impl_should_paste (
                                  RBRemovableMediaSource *source,
                                  RhythmDBEntry *entry);
Initialised to rb_true_function, so other removable media sources that wish to avoid duplicates will need to copy RBIpodSourceClass->impl_should_paste().  

I've done it this way because what is/is not a duplicate is fairly difficult to tell (see bug 133109) so whether or not a track counts as a duplicate really depends on the UI of the media player device.

Yeah, it was intentional not to handle duplicate in the ipod source, I think I even mentioned it in the mails I sent when ipod write support first landed.
The original plan was to have some kind of uid for songs to prevent them from being put twice on the ipod, but only if they were perfectly identical. Imo if the 2 files aren't perfectly identical, we should ask the user which one he wants (eg, when trying to replace a low bitrate song with a newer one or vice versa).

But this patch is probably much better than the current behaviour, though it could be a bit confusing if it doesn't tell why it ignored some files.

Agreed, this patch seems to work much better than the current behaviour (filling a users iPod unnecessarily) which is especially annoying to shuffle/nano users.

 

A couple of minor things:

- instead of string comparisons, we could compare the results of rhythmdb_entry_get_refstring().  If the strings are the same, the returned pointers will be equal.

- the entry_type->category check could be moved to a default should_paste implementation that subclasses can chain up to.  It's a useful approximation (stops you trying to copy radio streams to your ipod), but it may not always make sense.

Created attachment 95480 [details] [review]
73ipod-dont-make-duplicates.1.patch

1. OK
2. OK

Thanks.

Created attachment 95483 [details] [review]
73ipod-dont-make-duplicates.2.patch

Oops, sorry.

Created attachment 95499 [details] [review]
73ipod-dont-make-duplicates.3.patch

Seems the cb signature of rhythmdb_entry_foreach_by_type changed.

This patch still seems important, still applies cleanly to current trunk (5531) and still seems to work (tested on a 2G nano and a 8G video nano).

Can someone (teuf, maybe) have a look and update or commit this, putting four identical tracks on a 2G iPod is annoying for the non-technical user.

Teuf, Jonathan?

I will test this patch within the next 24 hours, as I have also been annoyed with placing duplicate songs on my iPod.  I have a 1st Gen Mini, 1st Gen Nano, and 3rd Gen Nano Video).  Which should I test on?

John

The patch doesn't seem to work for me.  I tried on a 1st Gen 4Gb Nano, and a 3rd Gen 4Gb Nano Video.  It seems as if it will never transfer a track, regardless of whether or not it is already on the ipod.

A question I just thought of... do I need "Portable Player - MTP" plugin enabled alongside the "Portable Players - iPod"?

John

Created attachment 107947 [details] [review]
73ipod-dont-make-duplicates.patch

Updated for 0.11.5.

This patch doesn't seem to work against HEAD (5670), dragging and dropping a file onto any class of iPod doesn't seem to do any transcoding or transfer?

Removing the patch and re-compiling seems to do the right thing -- does this patch need to be updated for any recent code changes?

Actually, scratch that -- it does.

After manually updating the mtime on my music directory with touch, and re-plugging in any of the four ipod's i've been testing this with (mini 1G, mini 3G, video 5G or classic 6G) -- it does transcode and transfer files that are already on the iPod, and leaves duplicates alone.

(Even though, i'd guess the timestamps on the iPod and my music folder no longer match -- which is a nice feature, because doing that same thing without this patch creates a (1) file on the iPod and duplicates it in the database.)

People who have had trouble with this before might want to try running touch on both the media files and the directories in their music collection -- then try this patch again.

I am using 0.11.6 and this is still an annoying problem

Created attachment 115802 [details] [review]
Updated patch against SVN (5834)

Seems to work with a GIO enabled SVN snapshot.

I've tested this patch against latest SVN (5974) and it works like a charm.

I didn't need to touch any files, either.  I just plugged in my 2g Mini, selected all the tracks in my 'Top Rated' auto-playlist in RB (3 stars or higher), and drag+drop onto my iPod.
Of the 180 Top Rated songs, only the 50 that weren't already on my iPod were transferred.  Artist, Title, Album, Rating, Year, etc all transferred perfectly.

My hats off to Ed and Paul for creating and updating this patch, respectively.

'Works for Me!" (tm)

JD

Created attachment 120862 [details] [review]
Improve RbIpodSource::should_paste

Iterating over all ipod tracks to check if one of those tracks matches the one we are trying to copy is probably quite inefficient (think about what happens when you copy 10k songs from your library to an ipod, then add 5k songs to the library and try to do another copy of the whole library to the ipod). This patch uses the query functionality of rhythmdb which should be more efficient.

Can't we move the implementation from the ipod source to the removable media source, as a helper function?

The ipod, generic player and mtp sources would be using the helper function, the default would still be false.

I committed the latest patch. Bastien, I remembered your feedback after committing, sorry for not addressing it before the commit :( I'll look at moving the impl this week.

Created attachment 122028 [details] [review]
Patch moving the should_paste_no_duplicate implementation to RBRemovableMediaSource

Ok, here is the patch I promised in comment #20. It moves the code to check if the new track duplicates an existing one in a helper function in RBRemovableMediaSource. The name might be a bit misleading, it's named rb_removable_media_source_should_paste_no_duplicate but is meant to be used as a vfunc callback, while there is a rb_removable_media_source_should_paste which is meant to be called as a regular function, so maybe the naming should be improved.
I only patched the ipod source, not the mtp source, though the change is probably trivial.

Looks fine to me.  I'll take care of the mtp and generic player sources, since I can test those.

Created attachment 122566 [details] [review]
patch for the MTP and generic player sources

I guess there is only this simple change to do to both source. I only compile-tested it, I'll let you test it some more if you have time for that :)

Patch works for both generic and mtp players.

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

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

All the patches in this bug are in, the latest complaints have been added to bug #570600, let's close it as a duplicate.

*** This bug has been marked as a duplicate of bug 570600 ***