Created attachment 119966 [details] [review]
the patch

This is looking pretty good - nice job on being thorough and making this a comprehensive patch (query, sorting, columns, stats page, etc).

1. See HACKING for style issues - use 4 spaces for tabs, not actual tabs, always use {}s in if statements even if only one line, name member/local variables like_this not likeThis, etc.

2. I don't think we need Score in CoreAlbums/Artists

3. Why did you modify src/Libraries/Mtp/Mtp/Track.cs ?  MTP doesn't support Score, does it?

4. You are probably using an old-ish version of MonoDevelop (eg not the 2.0 series) I'm guessing.  Can you figure out if there's anything that really needs to be changed with the .csproj file, and if not, exclude that from your patch?

Thanks!

Created attachment 120448 [details] [review]
Updated Patch

Updated version of the patch for scoring tracks.  Cleaner :)

Looks like you addressed all of Gabriel's comments, and it seems to work fine for me.
Great work !

Looks good to me to, but we're string frozen until after 1.4 unfortunately.  Please ping us after 1.4 is out so we can commit it.

Submitting a further updated patch that properly uses percentages.  Not obsoleting the old one in case I have screwed something up.

Created attachment 120849 [details] [review]
New Patch with percentages

This one completely eliminates the need for IncrementPlayCount() and IncrementSkipCount()

Created attachment 120866 [details] [review]
fixes some minor errors in the patch

In BansheeDbFormatMigrator.cs in this change:

                         Rating,
+                        Score,

There is no Score column in the old, Banshee 0.13.2 Tracks table.  So replace 'Score' with 0 here.

TrackChanged(1) => TrackChanged (1)

In PlayerEngineService.cs there is no reason to have the completion variable at all - just pass the pos/length value to TrackChanged.  And let's rename/merge that method to be OnPlaybackFinished, maybe?  TrackChanged is way too generic.

In TrackInfo.cs you cast (double)total_plays twice, but it's already a double.

Hey Brian, this is so close - have any time to finish it up?

Created attachment 129246 [details] [review]
updated version

Changed according to comment #9, hope I didn't screw up anything, there were a few conflicts.

(In reply to comment #9)
> And let's rename/merge that method to be OnPlaybackFinished, maybe?
> TrackChanged is way too generic.

Do you mean the code from TrackInfo.TrackChanged() and from its overrides should be moved to TrackInfo.OnPlaybackFinished() and its overrides?

Good to see someone has picked this up where I left off.. Thanks Alexander. 

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

(In reply to comment #11)
> Do you mean the code from TrackInfo.TrackChanged() and from its overrides
> should be moved to TrackInfo.OnPlaybackFinished() and its overrides?

Yeah, exactly.

Can you update this to trunk (probably some conflicts in the db migration code) too?  Also, rename 'completion' to 'percent_completed' (or percentCompleted in method signatures) please.

Created attachment 129768 [details] [review]
Updated to comment #14

Created attachment 129770 [details] [review]
Fixed a typo.

Should we default the score for the db migration to "score = playcount/(playcount + skipcount)?

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

(In reply to comment #17)
> Should we default the score for the db migration to "score =
> playcount/(playcount + skipcount)?
> 
Makes sense, marking as needs-work.

Created attachment 130994 [details] [review]
Updated to comment #17

Created attachment 131010 [details] [review]
Updated to trunk, the database schema version has been changed.

Created attachment 131063 [details] [review]
Updated to trunk, the database schema version has been changed.

Thanks Brian and Alexander, committed!

Thanks Alexander for all your hard work in finishing this up!

I'm getting this using today's trunk. Falling back to r5143 (newer ones may still work) works. Any idea how to modify the DB to get it into a state where the migration can work successfully? Would dropping the Score column do?


$ banshee-1 --debug
** Running Mono with --debug   **
[Info  19:24:40.113] Running Banshee 1.5.0: [svn-checkout (linux-gnu, i686) @ 2009-03-21 14:49:26 CET]
[Debug 19:24:41.092] Bus.Session.RequestName ('org.bansheeproject.Banshee') replied with PrimaryOwner
[Debug 19:24:41.098] Core service started (DBusServiceManager, 0.001287s)
[Debug 19:24:41.101] Registering remote object /org/bansheeproject/Banshee/DBusCommandService (Banshee.ServiceStack.DBusCommandService) on org.bansheeproject.Banshee
[Debug 19:24:41.113] Core service started (DBusCommandService, 0.013871s)
[Debug 19:24:41.266] Opened SQLite connection to /home/nx/.config/banshee-1/banshee.db
[Debug 19:24:41.266] Core service started (DbConnection, 0.152764s)
[Debug 19:24:41.269] Migrating from database version 25 to 29
[Debug 19:24:42.506] Exception executing command: ALTER TABLE CoreTracks ADD COLUMN Score INTEGER DEFAULT 0
[Warn  19:24:42.506] Rolling back database migration
[Error 19:24:42.583] Error initializing required service DbConnection
Exception has been thrown by the target of an invocation.
System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation. ---> System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation. ---> Mono.Data.Sqlite.SqliteException: Sqlite error
duplicate column name: Score
  at Mono.Data.Sqlite.Sqlite3.Prepare (System.String strSql, Mono.Data.Sqlite.SqliteStatement previous, System.String& strRemain) [0x00126] in /home/nx/code/svn/gnome/banshee/src/Libraries/Mono.Data.Sqlite/Mono.Data.Sqlite/SQLite3.cs:267 
  at Mono.Data.Sqlite.SqliteCommand.BuildNextCommand () [0x00019] in /home/nx/code/svn/gnome/banshee/src/Libraries/Mono.Data.Sqlite/Mono.Data.Sqlite/SQLiteCommand.cs:226 
  --- End of inner exception stack trace ---
  at System.Reflection.MonoMethod.Invoke (System.Object obj, BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) [0x00071] in /home/nx/Desktop/mono-2.2/mcs/class/corlib/System.Reflection/MonoMethod.cs:169 
  at System.Reflection.MethodBase.Invoke (System.Object obj, System.Object[] parameters) [0x00000] in /home/nx/Desktop/mono-2.2/mcs/class/corlib/System.Reflection/MethodBase.cs:111 
  at Banshee.Database.BansheeDbFormatMigrator.InnerMigrate () [0x000a2] in /home/nx/code/svn/gnome/banshee/src/Core/Banshee.Services/Banshee.Database/BansheeDbFormatMigrator.cs:176 
  at Banshee.Database.BansheeDbFormatMigrator.Migrate () [0x00018] in /home/nx/code/svn/gnome/banshee/src/Core/Banshee.Services/Banshee.Database/BansheeDbFormatMigrator.cs:136 
  --- End of inner exception stack trace ---
  at System.Reflection.MonoCMethod.Invoke (System.Object obj, BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) [0x000b8] in /home/nx/Desktop/mono-2.2/mcs/class/corlib/System.Reflection/MonoMethod.cs:440 
  at System.Reflection.MonoCMethod.Invoke (BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) [0x00000] in /home/nx/Desktop/mono-2.2/mcs/class/corlib/System.Reflection/MonoMethod.cs:449 
  at System.Reflection.ConstructorInfo.Invoke (System.Object[] parameters) [0x0000e] in /home/nx/Desktop/mono-2.2/mcs/class/corlib/System.Reflection/ConstructorInfo.cs:77 
  at System.Activator.CreateInstance (System.Type type, Boolean nonPublic) [0x00083] in /home/nx/Desktop/mono-2.2/mcs/class/corlib/System/Activator.cs:323 
  at System.Activator.CreateInstance (System.Type type) [0x00000] in /home/nx/Desktop/mono-2.2/mcs/class/corlib/System/Activator.cs:213 
  at Banshee.Gui.GtkBaseClient.Startup () [0x00000] in /home/nx/code/svn/gnome/banshee/src/Core/Banshee.ThickClient/Banshee.Addins.Gui/AddinDetailsDialog.cs:1 
  at Hyena.Gui.CleanRoomStartup.Startup (Hyena.Gui.StartupInvocationHandler startup) [0x00044] in /home/nx/code/svn/gnome/banshee/src/Libraries/Hyena.Gui/Hyena.Gui/CleanRoomStartup.cs:54 

(In reply to comment #25)
> I'm getting this using today's trunk. Falling back to r5143 (newer ones may
> still work) works. Any idea how to modify the DB to get it into a state where
> the migration can work successfully? Would dropping the Score column do?

I couldn't reproduce this on my dev database, but can on the main db. I will try to find a solution ASAP.

Created attachment 131103 [details] [review]
Fixing the database migration error.

Michael, please try this patch and let me know if it works for you.

Gabriel, could you ping me on IRC, I have a question regarding this issue.

Comment on attachment 131103 [details] [review]
Fixing the database migration error.

>diff --git a/src/Core/Banshee.Services/Banshee.Database/BansheeDbFormatMigrator.cs b/src/Core/Banshee.Services/Banshee.Database/BansheeDbFormatMigrator.cs
>index 23784a5..5609581 100644
>--- a/src/Core/Banshee.Services/Banshee.Database/BansheeDbFormatMigrator.cs
>+++ b/src/Core/Banshee.Services/Banshee.Database/BansheeDbFormatMigrator.cs
>@@ -666,7 +666,9 @@ namespace Banshee.Database
>         [DatabaseVersion (29)]
>         private bool Migrate_29 ()
>         {
>-            Execute ("ALTER TABLE CoreTracks ADD COLUMN Score INTEGER DEFAULT 0");
>+            if (!connection.ColumnExists ("CoreTracks", "Score")) {
>+                Execute ("ALTER TABLE CoreTracks ADD COLUMN Score INTEGER DEFAULT 0");
>+            }
>             Execute (@"
>                 UPDATE CoreTracks
>                 SET Score = CAST(ROUND(100.00 * PlayCount / (PlayCount + SkipCount)) AS INTEGER)

Created attachment 131106 [details] [review]
Fixed a typo.

Ignore the previous comment, I screwed up patch editing.

(In reply to comment #27)
> Michael, please try this patch and let me know if it works for you.

With this patch The error is still the same. I don't have a backup of the db before the first migration failure, though and I guess this is what I am supposed to use now.

I originally tested the code on my test db as you did and did not run into any problems with this one :/

(In reply to comment #30)
> With this patch The error is still the same. 

Awww..... Sorry! Sometimes, actually *applying* the patch (instead of just --try-run) makes a lot of difference!

With the patch applied the "broken" db works again. Thanks!

Wait, why is this check needed?  Is it b/c you'd ran an earlier version of the patch?

(In reply to comment #32)
> Wait, why is this check needed?  Is it b/c you'd ran an earlier version of the
> patch?
> 

Because apparently the column is added in Migrate_27(). That's actually what I wanted to discuss, I guess it will be easier on IRC...

Created attachment 131218 [details] [review]
Disabling tables check during the database migration.

This patch fixes the underlying problem.

For anyone wondering, the migration error on startup was caused when a patch from this bug was committed in r5155, so you can `svn up -r5154` if you want to go back to a working rev.

Alexander, glad to see you working to fix this, and excited to play with this new feature!

Marking last patch as needs-work, it still doesn't fix the issue. We discussed this with Gabriel on IRC, I will try to code a solution ASAP.

(In reply to comment #35)
> For anyone wondering, the migration error on startup was caused when a patch
> from this bug was committed in r5155, so you can `svn up -r5154` if you want to
> go back to a working rev.

Actually, reverting to r5154, making and running banshee, and then getting the trunk version will also work. The error occurs only when database migration added in r5155 is executed along with the migration from r5144.

Created attachment 131314 [details] [review]
Avoiding calling DatabaseTrackInfo.Provider when migrating the database

Thanks Alexander, I committed a modified version of this, working fine for me.