extragear/multimedia/amarok

[Ian Monroe]

On Fri, Jun 26, 2009 at 2:17 PM, Soren Harward<stharward at gmail.com> wrote:
> On Fri, Jun 26, 2009 at 12:36 PM, Jeff Mitchell<mitchell at kde.org> wrote:
>> Ian, Alejandro and myself (at least) have multiple reservations about the changes made in this patch.  It needs review and testing before being pushed out to trunk, especially as it updates the database (in a way
>> that is probably not correct).  This really needs to be put in a git branch first and tested out, or submitted as a patch to the mailing list for review.
>
> Here's the patch against r987764.
>
> But I'd like to point out that this isn't the first time I've
> submitted a patch to the ML for review.

I believe we asked for a git repo to help review this stuff for
months. I'm actually less concerned about this stuff as the UI for
dynamic playlist replacement.

> I'm fine with it being
> reviewed before it gets committed.  But I'm not fine with people
> saying it must be reviewed before it gets committed, and then nobody
> ever reviewing it (especially those who insist on a review), and thus
> never letting me commit it.
>
> Comments please.

In general I'm uncomfortable with features that are optional via
#ifdef's. We had this all over the place in Amarok 1.4 (tunepimp!) and
I wasn't really sad to see it go. But sometimes its the only thing
that makes sense.

Just from glancing over the patch (handy fisheyeization of it:
http://kollide.net:8060/changelog/Amarok/?cs=24870) one problem I see
is that you don't add your columns at the end of the list of table
columns in table creation. And your update code would add the columns
at the end. I'm pretty sure column order matters for SQL.

Why is it using shared pointers used for Fingerprinter? Couldn't its
memory be managed by Meta::Track?

The rest is standard review nitpicking:

Your block {}'s should each go on its own line. See the hacking dir.

SqlQueryMaker.cpp
 	 	955	+	        KSharedPtr<SqlTrack> sqlT =
KSharedPtr<SqlTrack>::staticCast( t );
 	 	956	+	        if (( d->similarityTracks.size() > 0 ) && sqlT ) {

This tests if 'sqlT' is 0 or not. If you mean to test if t is a null
pointer or not, test t instead of sqlT so it clear that there isn't
confusion about what staticCast does.

Don't use the [] operator in read-only operations, the compiler
generates better code if you use a const method like .at().

Ian