<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/105488/">http://git.reviewboard.kde.org/r/105488/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 11th, 2012, 9:20 a.m., <b>Ralf Engels</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Looks good for now.
What I would really like is some more test cases to check for the current problems.
Also I am working on a solution for the database schema as you can see from the database table .svg file in the docs directory.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">> Looks good for now.
> What I would really like is some more test cases to check for the current problems.
Agreed, but due to their complexity, I won't be able to write them in Amarok 2.6 timeframe. Do you think we can merge this for 2.6 even without tests?
> Also I am working on a solution for the database schema as you can see from the database table .svg file in the docs directory.
Yeah, I've seen it and generally it's a right step forward IMO. But please let me look at it in more detail after 2.6 is released. Quick question: don't you think that multiple artists (etc) for a track will make code much more complicated?</pre>
<br />
<p>- Matěj</p>
<br />
<p>On July 10th, 2012, 2:54 p.m., Matěj Laitl wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Amarok and Ralf Engels.</div>
<div>By Matěj Laitl.</div>
<p style="color: grey;"><i>Updated July 10, 2012, 2:54 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://bugs.kde.org/show_bug.cgi?id=298275">298275</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>src/core-impl/collections/db/sql/DatabaseUpdater.h <span style="color: grey">(57af65de0e3cbea5d7b7693f2d586eb1fe823870)</span></li>
<li>src/core-impl/collections/db/sql/DatabaseUpdater.cpp <span style="color: grey">(59e071d2f4fc69d79196e22e9119ff02805f28ca)</span></li>
<li>src/core-impl/collections/db/sql/SqlMeta.h <span style="color: grey">(8ee1014eb160547c4cbc0ea5571c12a895a79c64)</span></li>
<li>src/core-impl/collections/db/sql/SqlMeta.cpp <span style="color: grey">(a9b678aa4e3e55a2394da6f6581c159580fe6fc5)</span></li>
<li>src/core-impl/collections/db/sql/SqlRegistry.h <span style="color: grey">(8d801178cc070c7772363d28d86ca63467996ebe)</span></li>
<li>src/core-impl/collections/db/sql/SqlRegistry.cpp <span style="color: grey">(1b9efe5490bebc849b6c52c0b0b3fd51a5565418)</span></li>
<li>src/core-impl/collections/db/sql/SqlScanResultProcessor.h <span style="color: grey">(1bd96db03363cf8f760811fb231fd9facf521e8e)</span></li>
<li>src/core-impl/collections/db/sql/SqlScanResultProcessor.cpp <span style="color: grey">(6699b982e124193d068e2bd093bf0d15d6e34a9c)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/105488/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>