I notice there is one for amarok. Having one for rhythmbox would be a useful tool for encouraging distros to switch from rhythmbox to banshee as their default music player/

Created attachment 119746 [details] [review]
update of the old 0.13.2 code

A first patch (mainly an updated version of the 0.13.2 code).

Hey Paul, this is looking good.  A couple changes would be good:

1. Your copyright should be 2008 - you can put it right below the current 2006 line
2. Go ahead and make rhythmbox_db_path a SafeUri instead of making a new SafeUri in several spots

Answering questions from your e-mail:
> RhythmboxPlayerImportSource.cs) and imported rated songs multiple (6x)
> times. But there's also an "Importing Media" Job running so I don't know
> what is causing this.

Do you have playlists in RB?  It may be importing them once for each playlist they're in.  Though it shouldn't import duplicates (detected based on URI).

> Another thing I noticed it that in ImportSourceManager there now are calls
> for every PlayerImporter. Maybe it would be clever to create an extension
> point therefore.

Yeah, that'd be nice.  If you're interested in doing that, let's do it in two phases, one for this patch/fix and one for that.

> A last question: The importer currently only imports songs. How  should it
> handle Podcasts/Radio-Streams. Is there a way to only import them if the
> corresponding extensions are enabled?\

You're already using the LibraryImportManager as your import_manager, so importing podcasts should work (it should put them under Podcasts if they have Genre = Podcast).  Importing radio stations...would have to be added to the LibraryImportManager - which should probably be made extendable by extensions to hook into the import process.

> ps. Do I need to class make clean; ./autogen.sh, make, make run after every
> change or is there a simple way?

You should just be able to "make" and then "make run" - should rarely have to make clean or ./autogen.sh (usually after svn up'ing and somebody else committed new files).

Would probably be a good idea to use a XmlTextReader instead of a XmlDocument too, as I understand it it's lighter weight.

Created attachment 119834 [details] [review]
now with podcast import

Hey, an updated patch is ready.
It solves points 1 and 2 from Gabriel.

The multiple imports were a stupid copy-and-paste mistake. It is solved now. Furthermore all downloaded podcast episodes are imported now.
Currently I set the AlbumTitle and TrackTitle properties of each podcast to the according rhythmbox data. Is this ok?

Back to songs: I read out information like title, album, artist, genre.... import_manager.ImportTrack reads out meta-data from the file. I think the best would be only to write the rhythmbox based information if no metadata is available.

I created a new bug for the extension point: http://bugzilla.gnome.org/show_bug.cgi?id=554783

Looking forward to feedback.

Hey Paul, looking good.

Please read HACKING for our full style guide - specifically please use curleys around if statements even if only one line, and use class names not C#-type-names when doing things like Int32.Parse (instead of int.parse).

Would you break the ImportCore function up into ImportMusic and ImportPodcasts methods, to make the code more readable?  I'm also slightly worried the ImportPodcasts code will throw an exception, so maybe within the ImportCore where it calls ImportPodcasts surround it with a try/catch/Log.Exception?

Did RB get rid of "first-seen"?  That's an important field to migrate (to our DateAdded).  Are there other RB fields we should be importing?  Looks like you have it parsing quite a few, but you only actually set a couple - why aren't we setting all of those?  What if the user edited the info in RB but it didn't write that back to file?  The metadata in RB's db should "win" over the metadata in the file.

Oh, I also don't like the Mono.Unix.Native.NativeConvert.ToDateTime call or the parsing of the "date" field (why is it different than the other dates, first of all?).  If the values in RB are stored as seconds-since-epoch than we should be using Hyena.DateTimeUtil.ToDateTime.

Thanks!  This is coming along nicely.

Created attachment 119911 [details] [review]
splitted in smaller methods

I splitted the ImportCore method up. The code not longer throws any Exception but logs errors therefore. The first-seen filed is now migrated properly and all other fields are set to their RB-library data. Furthermore Hyena.DateTimeUtil replaced Mono.Unix...

I also started playlist migration but need some hints. 
When creating a new PlaylistSource (is this the right class to create a static playlist?) the constructor wants a PrimarySource parameter. Where do I get this from?

Hey Paul, good progress here.  Some nitpicky things:

1) Make sure to use spaces between method names and their args, eg method (args) not method(args)
2) In the "if (track == null) { LogError..." check it should "continue" after the LogError to avoid unnecessarily throwing a NRE a few lines later (which is caught and logged w/ a second LogError).
3) Might see if adding the RB icon to the front of the IconNames array works.
4) Go ahead and keep the SortOrder the same as Amarok - they should then be grouped together, but sorted alphabetically (even w/ translations).

For playlists you're attaching them to the MusicLibrary, so it's your PrimarySource, accessible via ServiceManager.SourceManager.MusicLibrary.

How much testing of this have you done?  Can you test with 1) a empty RB db 2) one w/ content/playlists/etc  3) one w/ a corrupted db (go into the file and mess it up) ?

Thanks, this is getting really close to commitable!

Created attachment 119942 [details] [review]
updated patch

@ 1: Hope I found all now. If not it would be great to get line numbers :P
@ 2+4: done.
@ 3: Great idea! Works for me :)

On playlists: I added some code (from PlaylistFileUtil). The playlists are now imported with name but no tracks are added. Any ideas?

On tests (tested with podcasts and songs): Works great with empty, small (10 songs, 1 album) and big (>2700 songs) libraries. I tried a corrupted library but this ends in neither importing anything nor ending the importer code. I'll have a look at this tomorrow. Maybe XmlDocument can tell me if the library is corrupted.

Oh, forgot one thing.

I tried to delete the Amarok and Rhythmbox importers code in ImportSourceManager.

Therefore I changed the code in Banshee.Services.addins.xml like this:
<ExtensionPoint path="/Banshee/Library/ImportSource">
    <ExtensionNode name="ImportSource"/>

    <ImportSource class="Banshee.PlayerMigration.AmarokPlayerImportSource"/>
    <ImportSource class="Banshee.PlayerMigration.RhythmboxPlayerImportSource"/>
</ExtensionPoint>

As result booth importers are not shown up in Banshee. Ideas on this?

(In reply to comment #11)
> As result booth importers are not shown up in Banshee. Ideas on this?

Let's keep that as a separate bug/patch.

For the playlist importing, is your inner loop where you iterate over the tracks in the XML playlist being run?  Put some Console.WriteLine's in there, then make sure track_id is getting set right, etc.  It looks ok to me at a glance.

Seems like Rhythmbox deleted all playlist entries when I started it with an empty database. Works great with refilled playlists.

Created attachment 120227 [details] [review]
unnamed playlists stay unnamed

This is only a small update. If a playlist in Rhythmbox is unnamed it's now also unnamed in Banshee.

I also did further testing and I found only corrupted databases not working. Are there further things that need to be done?

On corrupted databases: maybe the best way would be to generate xsd files and validate the document before importing. Thoughts?

Paul, I only care that corrupted databases are ignored and don't crash Banshee - I certainly don't expect us/you to try to work around them.

Created attachment 120351 [details] [review]
no further endless loops

This patch adds detection of corrupted files. Banshee not longer runs into endless loops when importing a corrupt file.

Created attachment 120354 [details] [review]
validation update

A really small update. Only removed a exception identifier in IsValidXmlFile.

Created attachment 120357 [details] [review]
style improvements

mh, another patch. sorry for that frequency. :-/ 
This adds two whithespaces.

Created attachment 122787 [details] [review]
use extension point to register importers

This patch removes the hard coded registration of the importers and let them use the /Banshee/Library/ImportSource extension point.

Tested this patch on my 7000 large Rhythmbox library. 

When I wanted to import I had already imported the directory where Rhythmbox's files where located into Banshee manually. The reason why I wanted to import Rhythmbox's XML file was not because Banshee is not capable of by itself scanning a directory, but because of the user metadata that I had collected over the years using Rhythmbox's UI.

In particular, the rating of the songs.

The import only worked when I first cleared Banshee's library, imported using this plugin, and then scanned the directory using Banshee in search for files that failed or were missing from Rhythmbox's XML file. Else I just had a huge amount of errors in the Console of Banshee during the import.

Ideally the import plugin would cope with these "this record already exists in Banshee" by synchronizing user data. 

For example for the rating Rhythmbox uses the stupid three-stars rating by default. So a rating of three would mean: don't synchronize this. But a rating of anything but three would mean: if it already exists in Banshee, asks the user which of the two has priority. And if it's unrated in Banshee, then just overwrite the zero rating of Banshee with the non-three-stars rating of Rhythmbox's XML file.

Other than that, excellent work. Thanks a lot for this Paul.

Thanks for testing this Philip!

I agree that importing should update the metadata if the songs are already imported.

I thought LibraryImportManager.ImportTrack would return the library item if a song is already imported, but it doesn't seem to do. Is there a other way to check if an Uri is already imported and get the according DatabaseTrackInfo object (question specifically to Gabriel)

Furthermore Rhythmbox never auto-rated songs with three stars for me and I didn't find an option on a quick search. Does RB do this even in latest versions?
In any case it shouldn't be a problem to handle the 3-star-problem.

Philip, would you open a new bug about migrator-importers (Amarok, RB) not having that behavior?  I'd prefer to get this RB importer in as is first, then work on that.

Marking this final patch as commit after freeze (eg right after 1.4.2 is released).

Paul, I committed a slightly modified version of the patch, thanks!  I commented out the podcast importing for now, b/c afaict it would import podcast files into the Music Library (not ok) and doesn't subscribe the user to the feeds (the most important thing to do, imo).  If you want to work on that and open a new bug for it, it would be appreciated!