<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/100135/">http://git.reviewboard.kde.org/r/100135/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 15th, 2010, 3:33 p.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;">Regarding the noCompilation of the collection scanner.
Actually this makes sense.
The collection scanner has a lot of heuristics for detecting if an album is a compilation.
However we also have a compilation tag.
If the tag claims that a track is definitely not in a compilation, then we should respect this and not try to auto-detect the compilation status.
I just noticed that I was not following this rule myself, but neighter should the noCompilation variable be removed unless you can argue why this doesn't make sense.
</pre>
</blockquote>
<p>On November 15th, 2010, 4:03 p.m., <b>Sergey Ivanov</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;">Track could only has 2 states: belong_to_compilation or belong_to_regular_album. But to indicate this 1bit information is currently used 2 mutually exclusive boolean variables. Initially they all are false, if track is a part of compilation isCompilation becomes true, if not - isNoCompilation. ATM there is no any way to get both of them true or false simultaneously, so I don't see any reason to use both.
This "a lot of heuristics" contains 2 checks in Directory: 1 directory AND 1 album but several artists - this is compilation; 1 directory AND empty album name AND less then 20 tracks - directory name is album name. ( isCompilation isn't used ) and 2 twins-functions isCompilation and isNoCompilation in Album class, which checks is isCompilation and isNoCompilation flags and compare album artist name with "Various Artists". Since isCompilation and isNoCompilation are mutually exclusive all this "heuristic" fold up to 1 line compact check. :)</pre>
</blockquote>
<p>On November 15th, 2010, 7:22 p.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;">No.
That's not it.
An album is a not a compilation if all tracks have the same artists, no artist or not "various artist" as album artist.
It is also not a compilation if one of it's tracks have the corresponding tag.
It is a compilation if one of it's tracks says so.
So, you see. The twenty lines of artist name checking aren't there for show.
The three states that a track can have are: in a compilation, not in a compilation, depends on the artists.
Actually I made a mistake in Directory.cpp. There I will throw tracks together into one compilation album independent of their compilation state.
I should not throw them together if one of the tracks says "noCompilation". That's where you need the flag again.
"isCompilation" and "noCompilation" are only mutually exclusive at the very end, when you actually know all the tracks you want to put into an album.
</pre>
</blockquote>
<p>On November 16th, 2010, 11:38 a.m., <b>Sergey Ivanov</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;">Ok. Reverted collection scanner changes for this patch. But still disagree with current way of handling collections and Album Artists. Probably It will be another patch, with more deep changes. :)</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;">You are welcome.
But I propose the following process:
1. I write some sensible auto tests that generate a test collection with all the different corner cases.
2. We decide on the way each of this corner cases should be handled
3. We fix the collection scanner until it passes all the tests.
</pre>
<br />
<p>- Ralf</p>
<br />
<p>On November 16th, 2010, 11:38 a.m., Sergey Ivanov wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.orgrb/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.</div>
<div>By Sergey Ivanov.</div>
<p style="color: grey;"><i>Updated 2010-11-16 11:38:35</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;">Add new QueryType - AlbumArtist.
Add new "level" to collection browser.
Add Album Artist field to TagDialog.
Add Album Artist to TagGuesser and Organize Collection dialog.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </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;">Tested with SqlCollection. Seems works for me. </pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>src/amarokurls/NavigationUrlGenerator.cpp <span style="color: grey">(5c48e73)</span></li>
<li>src/amarokurls/NavigationUrlRunner.cpp <span style="color: grey">(2d6384b)</span></li>
<li>src/browsers/CollectionTreeItemModelBase.h <span style="color: grey">(23a189f)</span></li>
<li>src/browsers/CollectionTreeItemModelBase.cpp <span style="color: grey">(7015cd4)</span></li>
<li>src/browsers/CollectionTreeView.cpp <span style="color: grey">(2bb894c)</span></li>
<li>src/browsers/collectionbrowser/CollectionWidget.cpp <span style="color: grey">(8623079)</span></li>
<li>src/core-impl/collections/db/sql/SqlMeta.cpp <span style="color: grey">(ff899fe)</span></li>
<li>src/core-impl/collections/db/sql/SqlQueryMaker.h <span style="color: grey">(680050f)</span></li>
<li>src/core-impl/collections/db/sql/SqlQueryMaker.cpp <span style="color: grey">(ca105e8)</span></li>
<li>src/core-impl/collections/db/sql/SqlQueryMakerInternal.cpp <span style="color: grey">(72cd07d)</span></li>
<li>src/core-impl/collections/proxycollection/ProxyCollectionQueryMaker.h <span style="color: grey">(63d6c2c)</span></li>
<li>src/core-impl/collections/proxycollection/ProxyCollectionQueryMaker.cpp <span style="color: grey">(c17c3fd)</span></li>
<li>src/core-impl/collections/support/MemoryQueryMaker.h <span style="color: grey">(72ddaf1)</span></li>
<li>src/core-impl/collections/support/MemoryQueryMaker.cpp <span style="color: grey">(81bf9b6)</span></li>
<li>src/core-impl/collections/support/MemoryQueryMakerInternal.h <span style="color: grey">(db94d8b)</span></li>
<li>src/core-impl/collections/support/MemoryQueryMakerInternal.cpp <span style="color: grey">(eeffd77)</span></li>
<li>src/core-impl/collections/support/XmlQueryReader.cpp <span style="color: grey">(b5518f0)</span></li>
<li>src/core-impl/collections/support/XmlQueryWriter.h <span style="color: grey">(5caf7bc)</span></li>
<li>src/core-impl/collections/support/XmlQueryWriter.cpp <span style="color: grey">(3c9cf41)</span></li>
<li>src/core/collections/MetaQueryMaker.h <span style="color: grey">(799825a)</span></li>
<li>src/core/collections/MetaQueryMaker.cpp <span style="color: grey">(10ae2a7)</span></li>
<li>src/core/collections/QueryMaker.h <span style="color: grey">(9c15659)</span></li>
<li>src/core/collections/QueryMaker.cpp <span style="color: grey">(78c6ba7)</span></li>
<li>src/core/meta/support/MetaConstants.h <span style="color: grey">(ba56248)</span></li>
<li>src/core/meta/support/MetaUtility.cpp <span style="color: grey">(5cf4519)</span></li>
<li>src/dialogs/FilenameLayoutDialog.h <span style="color: grey">(2e20c77)</span></li>
<li>src/dialogs/FilenameLayoutDialog.cpp <span style="color: grey">(9b572a9)</span></li>
<li>src/dialogs/FilenameLayoutDialog.ui <span style="color: grey">(b65b6f1)</span></li>
<li>src/dialogs/MusicBrainzTagger.cpp <span style="color: grey">(a9fc8a2)</span></li>
<li>src/dialogs/TagDialog.h <span style="color: grey">(fbc89ae)</span></li>
<li>src/dialogs/TagDialog.cpp <span style="color: grey">(fa0b33a)</span></li>
<li>src/dialogs/TagDialogBase.ui <span style="color: grey">(816ecb5)</span></li>
<li>src/dialogs/TagGuesser.h <span style="color: grey">(476b16d)</span></li>
<li>src/dialogs/TagGuesser.cpp <span style="color: grey">(bd3820d)</span></li>
<li>src/services/ServiceSqlQueryMaker.h <span style="color: grey">(5591ecc)</span></li>
<li>src/services/ServiceSqlQueryMaker.cpp <span style="color: grey">(197f2cf)</span></li>
<li>src/services/ampache/AmpacheServiceQueryMaker.cpp <span style="color: grey">(7dee8c8)</span></li>
<li>src/services/mp3tunes/Mp3tunesServiceQueryMaker.cpp <span style="color: grey">(4dc8fed)</span></li>
<li>src/services/scriptable/ScriptableServiceQueryMaker.cpp <span style="color: grey">(76faca2)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/100135/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Screenshots </h1>
<div>
<a href="http://git.reviewboard.kde.org/r/100135/s/17/"><img src="http://git.reviewboard.kde.org/media/uploaded/images/2010/11/06/AA_400x100.png" style="border: 1px black solid;" alt="Only one album "Once". As It should be." /></a>
<a href="http://git.reviewboard.kde.org/r/100135/s/18/"><img src="http://git.reviewboard.kde.org/media/uploaded/images/2010/11/06/AA1_400x100.png" style="border: 1px black solid;" alt="2 albums, and 3rd is somewhere below." /></a>
<a href="http://git.reviewboard.kde.org/r/100135/s/19/"><img src="http://git.reviewboard.kde.org/media/uploaded/images/2010/11/06/AA_1_400x100.png" style="border: 1px black solid;" alt="" /></a>
</div>
</td>
</tr>
</table>
</div>
</body>
</html>