Created attachment 88871 [details] [review]
Adds iTunes player migration

This patch changes the existing PlayerMigration system a little and adds an iTunes importer. Player importers are now their own ImportSource to the Import source list (see PlayerMigrationCore.cs for more details). The iTunes importer prompts the user to locate their "iTunes Music Library.xml" file. The user may specify if they would like to also import song ratings, playcounts and lastplayed dates, playlists, and smartplaylists.

The import should be able to find any song that is in the same mount as the iTunes library provided it is not within two consecutive directories of the same name (e.g. "iTunes/Music/Music/Artist/song.mp3").

This patch differs from my iTunes import plugin in two notable ways: if "iTunes Music Library.xml" is encountered during the course of a normal import, an iTunes import is not triggered, and a summary widget is not provided upon import completion. I can add either of these features.

Created attachment 99746 [details] [review]
A new patch from the win32 work

This patch changes the stucture of the PlayerMigration namespace and adds iTunes support.

Rather than a single player migration import source which opens the player migration

dialog, each importer is its own import source. All file paths are reletive to

banshee/src/Core/Banshee.Base



* Modified Banshee.PlayerMigration/AmarokPlayerImport.cs: Tweaked the class to conform

  to the new player migration structure.

  

* Added Banshee.PlayerMigration/ItunesImport.cs: Added an iTunes importer to the

  player migration system.

  

* Added Banshee.PlayerMigration/ItunesImportDialog.cs: Supporting UI for the iTunes

  importer.

  

* Added Banshee.PlayerMigration/ItunesSmartPlaylistParser.cs: Supporting class

  for the iTunes importer which handles translation of smart playlists.

  

* Modified Banshee.PlayerMigration/PlayerImport.cs: Tweaked the class to improve

  player migration structure.

  

* Removed Banshee.PlayerMigration/PlayerImportDialog.cs: Removed as part of the new

  player migration structure.

  

* Removed Banshee.PlayerMigration/PlayerImportSource.cs: Removed as part of the new

  player migration structure.



* Added Banshee.PlayerMigration/PlayerMigrationCore.cs: Added as part of the new

  player migration structure.



* Modified Banshee.PlayerMigration/RhythmboxPlayerImport.cs: Tweaked the class to

  improve player migration structure.

  

* Modified Gui/ImportDialog.cs: Tweaked the class to conform to the new player

  migration structure.

Created attachment 99754 [details] [review]
Here is a patch with the makefile changes and a bug fix

Created attachment 101025 [details] [review]
An updated patch with loads of fixes.

After working with a user whose directory structure I had not anticipated, I've significantly improved the importer. This new patch also includes some fixes which are not directly related to the importer, but which fix general stability problems that were particularly manifest during imports. This patch is a candidate for inclusion in the stable branch.

Created attachment 101026 [details]
An image required by the above patch

This is the image referenced in the above patch.

Hey Scott, one thing I noticed while reviewing this patch.  You do:

user_event.Header = Catalog.GetString("Importing Playlist ") + name;

But this should really be

user_event.Header = String.Format (Catalog.GetString ("Importing Playlist {0}"), name);

A more minor thing, properties should look like this:

public string Property {
  get { return "foo"; }
  set { .. }
}

Created attachment 101027 [details] [review]
With the changes Gabe mentioned

And with those changes. Let me know if you spot any other out of style code (some fragments are more than a year old!).

This is still really valuable code and a feature we should have.  Marking as needs-work since the patch was against 0.13.x or before.

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

Looks like Amarok 2 does some sort of iTunes importing as well (under 'Configure Amarok' -> Collection -> Import Collection

Created attachment 134641 [details] [review]
Updated to git master + fixes

I updated Scott's patch to apply to git master. In the process a few thing had to go:

 * Smart playlists import: playlists in 1.x are quite different from those in 0.13.x and also the handling of datetime expressions is a bit off. If someone wants to tackle this - you are more than welcome, in the meantime I think we can proceed without smart playlists. Rhythmbox and Amarok importers don't support them and also it's quite easy to create your smart playlists by hand.

 * Automatic import: this feature allowed to see when one of the files being imported (during the usual file or folder import) is iTunes Music Library.xml and processed it after the usual import finished. It looks like the new importer never triggers on xml files, probably because it's not a known media file extension. Another thing that prevented this to be ported is the fact that we cannot re-import files (see the next item).

 * Re-import. The previous patch added two fields to the Tracks table. The fields allowed to see if the file was already imported and handle this appropriately. I decided to remove this feature (along with the additional table fields) because I think this should be handled at the DatabaseImportManager level. See also the TODO in DatabaseImportManager.ImportTrack()

On the upside, the are a few imporvements:

 * Much more fields are being imported, for example: title, artist, album, genre, all *sort fields, year, date_added, grouping, etc
 * Fixed a bug in ProcessMusicFolderPath() that didn't allow to import from Vista partitions.
 * Skipping pre-defined playlists, e.g. Genius, TV Shows, Party Shuffle, etc
 * HACKING-compatible

There were quite a few changes compared to the previous patch. I would appreciate if Scott or anyone else would check the patch and test it on your iTunes libraries.

Wow, great work Alexander.  I agree with what you said about smart playlists.  They add a ton of complexity for marginal benefit.

"SELECT Uri FROM Tracks WHERE lower(Uri)

should not use lower() since it only works for ASCII.  Instead add a custom cmd in src/Libraries/Hyena/Hyena.Data.Sqlite/SqliteUtils.cs that uses String.ToLower (InvariantCulture), maybe?

Oh - also, why is that using the Tracks table?  That is from 0.13.x and before; in 1.x we use CoreTracks.

You can remove the commented out SmartPlaylist unmap loop.

Thanks!

Created attachment 134678 [details] [review]
Updated to comment #12

(In reply to comment #12)
> "SELECT Uri FROM Tracks WHERE lower(Uri)
> should not use lower() since it only works for ASCII.  Instead add a custom cmd
> in src/Libraries/Hyena/Hyena.Data.Sqlite/SqliteUtils.cs that uses
> String.ToLower (InvariantCulture), maybe?

Aren't the URIs encoded and contain only ASCII characters? For example I have this in my database for a song with a Cyrillic file name:

file:///home/alex/Music/Autumn%20Rain%20Melancholy/Seven%20Steps%20To%20Infinity/03.%20%D0%A1%D0%BD%D0%B5%D0%B3.mp3

I changed raw_uri.ToLower() to use the invariant culture.

> Oh - also, why is that using the Tracks table?  That is from 0.13.x and before;
> in 1.x we use CoreTracks.

Fixed. The funny thing is that I updated all other queries, which later got deleted :)

> You can remove the commented out SmartPlaylist unmap loop.

Done

Created attachment 134691 [details] [review]
Case-insensitive file and directory names

This patch works around a problem in the iTunes database. Some URIs cannot be resolved by the importer because iTunes relies on case-insensitive file system. For example, the iTunes database can have ../Foo/bar.mp3 while the real file name is ../foo/Bar.mp3.

The fix reduces the number of import errors on my iTunes library containing 5,200 songs from about 130 to zero.

Is it possible for people to name their itunes db files something else?  If so, wouldn't want to be too strict and filter those out.

(In reply to comment #15)
> Is it possible for people to name their itunes db files something else?  If so,
> wouldn't want to be too strict and filter those out.

AFAIK, no: http://support.apple.com/kb/HT1660

Created attachment 135780 [details] [review]
Updated to git master

The main thing I'm not keen on with this patch is not so much a fault with it, but our existing import dialog, and its confusing "Import Source" button that really goes on to step 2 of the process in some cases, and not in others.  This is bug #559013.  I'm going to mark this as blocked by it.

Bulk changing the assignee to banshee-maint@gnome.bugs to make it easier for people to get updated on all banshee bugs by following that address.  It's usually quite apparent who is working on a given bug by the comments and/or patches attached.

Comment on attachment 135780 [details] [review]
Updated to git master

Ok, sorry for blocking this so long.  Even though the import issue isn't fully resolved, let's get this in.

Thanks Gabriel, I committed the patch.

I'm keeping the bug open because we still need decide if we want to integrate the iTunes importer's settings into the main import dialog. If we do, some bits and pieces of patch 140872 from bug 559013 can be used.

(In reply to bug 559013, comment 25)
> If the iTunes migrator only has that one dialog with those couple prefs, then
> maybe we should put them into the import dialog itself.  I haven't tried it in
> some time; doesn't it have another dialog or two that can popup depending on
> circumstances?

There are two dialogues:

 1. ItunesImportDialog with the library location chooser button and 3 check boxes (what to import)
 2. ItunesMusicDirectoryDialog which is started only if we couldn't infer the location of the media files

They can (and probably should) be merged. We could eventually check if we can find the media files from the ItunesImportDialog and enable/show the corresponding FileChooserButton only when it's needed. The widgets could then be placed to the main ImportDialog.

Let me know if you think that's a good idea, otherwise I'm fine with closing the bug.

(In reply to comment #21)
> I'm keeping the bug open because we still need decide if we want to integrate
> the iTunes importer's settings into the main import dialog. If we do, some bits
> and pieces of patch 140872 [details] from bug 559013 can be used.

Yes, I think we should bring that into the import dialog.  The main issue with that is the sizing of the dialog.  Adding a Gtk.Notebook to the dialog is problematic, since it is always as tall as the tallest page it contains; we don't want there to be whitespace for every import source except iTunes just b/c it has some options.

OTOH, not having it be the height of the tallest member means changing the size of the dialog in front of the user.  I'd suggest we do this in the same way the bottom-right "Import" button changes - it can grow bigger if needed, but it will never shrink.  That way we avoid flip/flop resizing of the dialog.

I'm closing this bug. The original issue is resolved and I don't want to keep such an old bug open forever.

Besides, I'm not sure that including iTunes importer's dialogues into the main ImportDialog is a better solution than what we have now. In my opinion, changing it just for the iTunes importer is not worth it. Feel free to open a new bug if you disagree.