----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104966/#review14013 -----------------------------------------------------------
I'm not all that familiar with lyrics & the SQL schema so will leave others to call it shipable. Make sure to update HACKING/mysql_database_schema.txt when you commit. ChangeLog <http://git.reviewboard.kde.org/r/104966/#comment11106> "data are migrated" tickles my English grammar bone. "data is migrated" sounds better, even though it might not technically be correct with data plural. ChangeLog <http://git.reviewboard.kde.org/r/104966/#comment11105> Does this patch fix this? Or just makes it so rescanning is not required anymore? src/core-impl/collections/db/sql/DatabaseUpdater.cpp <http://git.reviewboard.kde.org/r/104966/#comment11107> You don't actually check if the previous query succeeded. src/core-impl/collections/db/sql/DatabaseUpdater.cpp <http://git.reviewboard.kde.org/r/104966/#comment11108> I was wondering if the url column could actually be intentional to point to a remote resource. But if it's not used currently there is no point in having it. - Bart Cerneels On May 16, 2012, 1:51 p.m., Matěj Laitl wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/104966/ > ----------------------------------------------------------- > > (Updated May 16, 2012, 1:51 p.m.) > > > Review request for Amarok and Ralf Engels. > > > Description > ------- > > DB: change lyrics table: text url -> integer url pointing to the urls table > > I believe that the old lyrics table structure was more or less a mistake: > TABLE lyrics ( > id INTEGER PRIMARY KEY, -- actually never used in code > url VARBINARY(324), -- actually a rpath from urls table > lyrics TEXT > ) > > Apart from data duplication and non-conformity to the "update anomaly" > requirement of the database design, there was additional problem that rpath > itself is not unique. The (deviceId, rpath) is. > > This change makes the lyrics table look more sane: > TABLE lyrics ( > url INTEGER PRIMARY KEY, -- points to url.id column > lyrics TEXT > ) > > with an effort to transition existing entries. The transition of 5000 > lyrics entries takes 16s on my 2.5 GHz Core i5 (one core used), so it > should be acceptable. > > This is the first time I'm changing db structure, I'd be glad to > receive thorough review, namely of the update13to14() method and > especially the duplicate-removing query. This is critical because > database-corrupting fault would leave many Amarok users in a state > where they would need to drop their database to make Amarok working > again. > > Note to reporters of bug 242350: there's an unrelated bug 299150 which > now applies to lyrics, too. > > ChangeLog of the unrelated iPod fix is updated because DB_VERSION bump > triggers full collection rescan as far as is documented. > > BUG: 242350 > FIXED-IN: 2.6 > REVIEW: 104966 > > > This addresses bug 242350. > https://bugs.kde.org/show_bug.cgi?id=242350 > > > Diffs > ----- > > ChangeLog 67bc020 > src/core-impl/collections/db/sql/DatabaseUpdater.h 37ccb54 > src/core-impl/collections/db/sql/DatabaseUpdater.cpp 163b089 > src/core-impl/collections/db/sql/SqlMeta.cpp f8f9bdb > > Diff: http://git.reviewboard.kde.org/r/104966/diff/ > > > Testing > ------- > > > Thanks, > > Matěj Laitl > >
_______________________________________________ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
