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

List:       amarok-devel
Subject:    Re: [PATCH] ReplayGain tags
From:       Edward Hades <edward.hades () gmail ! com>
Date:       2009-01-11 10:08:22
Message-ID: 200901111308.41238.edward.hades () gmail ! com
[Download RAW message or body]

[Attachment #2 (multipart/signed)]


Hi,

Thanks for the patch, Alex. Looks nice and clean to me (and works fine), 
although I have several concerns.

The first is that, perhaps, from the general view of database design, it would 
be better to put replayGain stuff in another table, referenced by foreign key.

> I incremented the DB_VERSION to 2 and made DatabaseUpdater::update() modify
> the tracks table if the version of the existing database was 1.  Also, it
> does a full collection rescan (since an incremental scan only checks for
> changed files - there is no "look for new metadata in all the old files
> without clearing out the database" mode for collectionscanner).

I don't think this would be a nice behavior. I'd make it pop out a message 
saying something in lines of "Amarok needs more information from your music 
files, please rescan your collection to enable shiny new features".

Also, I am not exactly sure the update logic is correct, because i can't fully 
understand the old logic (why was there dbVersion > DB_VERSION?).

As a minor rant, I'd make qreal variables initialized with 0.0 instead of 0.

Otherwise the patch looks perfectly valid to me.

Knowing that this feature request is most wanted not only of Amarok, but all 
the KDE [1], I'm for committing the patch (with modifications), and proceeding 
in this direction.

[1] http://tinyurl.com/aynpgt

-- 
Edward "Hades" Toroshchin,
Aides on irc.freenode.org



["signature.asc" (application/pgp-signature)]

_______________________________________________
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]