----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105488/#review16264 -----------------------------------------------------------
Ship it! Provided the auto test run through and if you are really feeling brave enought you can push it. - Ralf Engels On July 10, 2012, 2:54 p.m., Matěj Laitl wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/105488/ > ----------------------------------------------------------- > > (Updated July 10, 2012, 2:54 p.m.) > > > Review request for Amarok and Ralf Engels. > > > Description > ------- > > SqlScanResultProcessor: debugging; not intended to be merged to master > > > SqlScanResultProcessor: cope with non-unique uniqueid in the database > > Unfortunately, the uniqueid column (or rather its index) of our urls > table is not defined as unique and unfortunately at least some code in > SqlCollection doesn't check for duplicates before inserting to the > table. This can be provoked for example by using the "Organize > Collection" functionality. > > While fixing SqlCollectionLocation in short-term and making the > uniqueid index unique in long-term is probably needed, we need to cope > with existing user databases. This change is needed because > SqlScanResultProcessor identified tracks fully by their uniqueid which > resulted in unpredictable and incorrect behaviour - it for example > never removed the "old" duplicate entry in deleteDeletedTracks( int ) > and sometimes found incorrect entry when importing a track in > commitTrack(). > > SqlScanResultProcessor: skip removeTrack() if there were errors > > Another in a series that try to minimize chance of users losing their > tracks, statistics etc. > > SqlScanResultProcessor: don't accidentally delete tracks, defensive rewrite > > This fixes data-loss that I can trigger every time by toggling "Local > Files & USB Mass Storage Backend" in Config -> Plugins, restarting > Amarok and triggering collection update / rescan. > > In theory, more things such as cloning changing disk could trigger this > problem, from SqlScanResultProcessor::deleteDeletedDirectories() comment: > > We need to match directories by their (absolute) path, otherwise following > scenario triggers statistics loss (bug 298275): > > 1. user relocates collection to different filesystem, but clones path > structure > or toggles MassStorageDeviceHandler enabled in Config -> plugins. > 2. collectionscanner knows nothings about directory ids, so it doesn't detect > any track changes and emits a bunch of skipped (unchanged) dirs with no > tracks. > 3. SqlRegistry::getDirectory() called there from returns different directory > id > then in past. > 4. deleteDeletedDirectories() is called, and if it operates on directory ids, > it happily removes _all_ directories, taking tracks with it. > 5. Tracks disappear from the UI until full rescan, stats, lyrics, labels are > lost forever. > > Also add a handful of asserts, ScanResultProcessor is very complicated > and small error or corner-case in logic may result in horrible data > losses. > > Reporters of linked bugs, please try to reproduce your data-loss with > this patch applied and report back in both cases. In negative case, > please reopen and attach full updated amarok --debug log. > > After this patch, only (statistics, lyrics and labels)-loss operations > should be: > a) moving track out of mounted collection folders [by design] > b) changing both metadata and url from outside of a track not tagged > by amarok_afttagger [we can do nothing about this] > > BUG: 298275 > FIXED-IN: 2.6 > REVIEW: 105488 > > SqlMeta::Track: remove unused methods deviceId() and rpath() > > There are unused and rather internal, to remove them. > id() and urlId() are unused, too, but these are at least > in theory useful. > > SqlScanResultProcessor: remove dead code, sanitize includes > > > SqlRegistry, DatabaseUpdater: delete stats and everything in removeTrack() > > SqlRegistry::removeTrack() had code to remove the entry from the tracks > table and to preserve the entry in the statistics table. This doesn't > work, because SqlRegistry construction calls > DatabaseUpdater::deleteAllRedundant( url ) on next start-up that > removes the url entry, plus we don't know how long we should keep the > entry, so we just delete everything. ScanResultProcessor should be > witty enough not to delete tracks that have been moved to another > directory and/or device, even if it is currently unavailable. > > Additionally, we clean up the statistics, urls_labels, lyrics tables > on start-up to avoid stale entries (pointing to deleted url) in a way > similar to what it is done with other tables. > > SqlScanResultProcessor: rename attributes in preparation for a fix > > ...because I was really confused by the old names. > > > This addresses bug 298275. > https://bugs.kde.org/show_bug.cgi?id=298275 > > > Diffs > ----- > > src/core-impl/collections/db/sql/DatabaseUpdater.h > 57af65de0e3cbea5d7b7693f2d586eb1fe823870 > src/core-impl/collections/db/sql/DatabaseUpdater.cpp > 59e071d2f4fc69d79196e22e9119ff02805f28ca > src/core-impl/collections/db/sql/SqlMeta.h > 8ee1014eb160547c4cbc0ea5571c12a895a79c64 > src/core-impl/collections/db/sql/SqlMeta.cpp > a9b678aa4e3e55a2394da6f6581c159580fe6fc5 > src/core-impl/collections/db/sql/SqlRegistry.h > 8d801178cc070c7772363d28d86ca63467996ebe > src/core-impl/collections/db/sql/SqlRegistry.cpp > 1b9efe5490bebc849b6c52c0b0b3fd51a5565418 > src/core-impl/collections/db/sql/SqlScanResultProcessor.h > 1bd96db03363cf8f760811fb231fd9facf521e8e > src/core-impl/collections/db/sql/SqlScanResultProcessor.cpp > 6699b982e124193d068e2bd093bf0d15d6e34a9c > > Diff: http://git.reviewboard.kde.org/r/105488/diff/ > > > Testing > ------- > > > Thanks, > > Matěj Laitl > >
_______________________________________________ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
