[prev in list] [next in list] [prev in thread] [next in thread] 

List:       amarok-devel
Subject:    Re: extragear/multimedia/amarok
From:       Ian Monroe <ian.monroe () gmail ! com>
Date:       2009-06-26 20:49:43
Message-ID: f680fec50906261349p7522fa3bj72fc03af4d7851eb () mail ! gmail ! com
[Download RAW message or body]

On Fri, Jun 26, 2009 at 2:17 PM, Soren Harward<stharward@gmail.com> wrote:
> On Fri, Jun 26, 2009 at 12:36 PM, Jeff Mitchell<mitchell@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
_______________________________________________
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel


[prev in list] [next in list] [prev in thread] [next in thread]