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 441093 - Import / MIgrate from iTunes
Import / MIgrate from iTunes
Status: RESOLVED FIXED
Product: banshee
Classification: Other
Component: Importing
git master
Other Linux
: Normal enhancement
: 1.6
Assigned To: Alexander Kojevnikov
Banshee Maintainers
: 547925 (view as bug list)
Depends on: 559013
Blocks:
 
 
Reported: 2007-05-25 02:46 UTC by Scott Peterson
Modified: 2009-12-06 08:18 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Adds iTunes player migration (79.62 KB, patch)
2007-05-27 04:52 UTC, Scott Peterson
none Details | Review
A new patch from the win32 work (84.75 KB, patch)
2007-11-27 23:01 UTC, Scott Peterson
none Details | Review
Here is a patch with the makefile changes and a bug fix (83.93 KB, patch)
2007-11-28 05:37 UTC, Scott Peterson
none Details | Review
An updated patch with loads of fixes. (156.17 KB, patch)
2007-12-15 19:44 UTC, Scott Peterson
none Details | Review
An image required by the above patch (15.49 KB, image/png)
2007-12-15 19:44 UTC, Scott Peterson
  Details
With the changes Gabe mentioned (156.11 KB, patch)
2007-12-15 21:03 UTC, Scott Peterson
needs-work Details | Review
Updated to git master + fixes (50.51 KB, patch)
2009-05-14 11:56 UTC, Alexander Kojevnikov
needs-work Details | Review
Updated to comment #12 (50.19 KB, patch)
2009-05-15 03:30 UTC, Alexander Kojevnikov
none Details | Review
Case-insensitive file and directory names (52.29 KB, patch)
2009-05-15 08:59 UTC, Alexander Kojevnikov
none Details | Review
Updated to git master (106.69 KB, patch)
2009-06-02 00:17 UTC, Alexander Kojevnikov
committed Details | Review

Description Scott Peterson 2007-05-25 02:46:34 UTC
Here's a bug for work and discussion on merging my iTunes importer in the PlayerMigration namespace: http://bugzilla.gnome.org/show_bug.cgi?id=378430

First, some thoughts on the current player migration setup:

I find the play migration interface a bit obtuse. "Alternate Media Players" could mean portable media devices. Also, I don't find the "Migrate From Other Players" dialog useful. Most all people are migrating from only one player. As it happens, I only have one other player installed: Rhytmbox. That's probably the case for all other Ubuntu users. A check list with only one option isn't great UI.

I think available player migration sources should be listed as import sources in the Import dialog. I'm going to try out some different things with PlayerMigration and put them up for folks to critique, but in the mean time I'd love to hear people's thoughts on this.
Comment 1 Scott Peterson 2007-05-27 04:52:36 UTC
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.
Comment 2 Scott Peterson 2007-11-27 23:01:42 UTC
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.
Comment 3 Scott Peterson 2007-11-28 05:37:03 UTC
Created attachment 99754 [details] [review]
Here is a patch with the makefile changes and a bug fix
Comment 4 Scott Peterson 2007-12-15 19:44:02 UTC
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.
Comment 5 Scott Peterson 2007-12-15 19:44:56 UTC
Created attachment 101026 [details]
An image required by the above patch

This is the image referenced in the above patch.
Comment 6 Gabriel Burt 2007-12-15 20:46:09 UTC
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 { .. }
}
Comment 7 Scott Peterson 2007-12-15 21:03:37 UTC
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!).
Comment 8 Gabriel Burt 2009-01-13 18:47:33 UTC
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.
Comment 9 Alexander Kojevnikov 2009-03-09 07:54:40 UTC
*** Bug 547925 has been marked as a duplicate of this bug. ***
Comment 10 Gabriel Burt 2009-03-12 00:56:11 UTC
Looks like Amarok 2 does some sort of iTunes importing as well (under 'Configure Amarok' -> Collection -> Import Collection
Comment 11 Alexander Kojevnikov 2009-05-14 11:56:07 UTC
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.
Comment 12 Gabriel Burt 2009-05-14 15:37:42 UTC
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!
Comment 13 Alexander Kojevnikov 2009-05-15 03:30:13 UTC
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
Comment 14 Alexander Kojevnikov 2009-05-15 08:59:35 UTC
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.
Comment 15 Gabriel Burt 2009-05-18 20:06:43 UTC
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.
Comment 16 Alexander Kojevnikov 2009-05-19 00:18:23 UTC
(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
Comment 17 Alexander Kojevnikov 2009-06-02 00:17:57 UTC
Created attachment 135780 [details] [review]
Updated to git master
Comment 18 Gabriel Burt 2009-06-04 19:07:14 UTC
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.
Comment 19 Gabriel Burt 2009-10-27 20:19:12 UTC
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 20 Gabriel Burt 2009-11-06 04:34:13 UTC
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.
Comment 21 Alexander Kojevnikov 2009-11-11 05:22:23 UTC
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.
Comment 22 Gabriel Burt 2009-11-12 04:13:27 UTC
(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.
Comment 23 Alexander Kojevnikov 2009-12-06 08:18:48 UTC
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.