<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/100070/">http://git.reviewboard.kde.org/r/100070/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On October 21st, 2010, 7:03 p.m., <b>Soren Harward</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;">Using the MetaQueryWidget does not prohibit using the TagMatch::Comparer nested class. Removing the Comparer nested class from TagMatch is not a good idea, and replacing it with global functions is a genuinely bad one. I split off the Comparer class because I found myself tweaking the comparison algorithms on a regular basis, and I wanted to make changes in only one place. Please put it back. Thanks.</pre>
</blockquote>
<p>On October 21st, 2010, 7:04 p.m., <b>Soren Harward</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;">Also, the CompareLabels functionality in the comparer must be working before this patch can ship. Right now it's a TODO.</pre>
</blockquote>
<p>On October 21st, 2010, 9:56 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;">Hi Soren,
thank you for your review. I was waiting for someone from the Playlist Generator to post a review.
Can you please explain to me a little more why the TagMatch::Compare was needed to be seperate?
There is still only one place to change for the algorithms.
Here are the simple reasons why I removed the comparer.
It was a complete static class. All of the functions and were in principle static and it didn't have any member variables.
Now, there are still reasons for having such a class (e.g. if you want to extend it) but this one was so tightly coupled to the rest of the constraint that I couldn't see the reason for having it separate.
Expecially since it seems to have some outdated functionality (the list model)
Also the whole class boiled down to five functions.
Do your really want to have a seperate .cpp file (plus .h, because it is confusing to have a .cpp without header) just for those five functions?
And how does this make tweaking easier?
Next point:
The CompareLabels functionality was not implemented as far as I can see. I would be very happy to implement that for you (even though I think that the labels are a big TODO over the whole Amarok).
However I think it would dillute this patch whose main concern was to have a consistent UI.
A seperate patch would be more appropriate for this.
Maybe we can meet on IRC somewhen (Sunday maybe because the next two days are quite full) and talk about some other ideas I had.
</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;">I was thinking about the labels stuff.
I remembered that this didn't work before. I was just checking and I am not so sure any more.
Was this working?
I am not even sure if you can use a QueryMaker to search for labels.
I will have a look at it again anyhow.</pre>
<br />
<p>- Ralf</p>
<br />
<p>On October 18th, 2010, 12:07 p.m., Ralf Engels 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 Ralf Engels.</div>
<p style="color: grey;"><i>Updated 2010-10-18 12:07:31</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;">Instead of implementing the whole behavior of selecting field and values I am just using the MetaQueryWidget.
Also I am moving all field related texts to src/core/meta/support/MetaConstants.cpp
This would also be a good place for the playlist to get it's texts from.</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;">Generated several advanced playlists testing a couple of the fields and all the conditions.</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/CMakeLists.txt <span style="color: grey">(abdcee8)</span></li>
<li>src/core-impl/collections/support/XmlQueryReader.h <span style="color: grey">(0088608)</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">(11c818e)</span></li>
<li>src/core-impl/collections/support/XmlQueryWriter.cpp <span style="color: grey">(657e0c8)</span></li>
<li>src/core/CMakeLists.txt <span style="color: grey">(5863ca1)</span></li>
<li>src/core/meta/support/MetaConstants.h <span style="color: grey">(40cad34)</span></li>
<li>src/core/meta/support/MetaConstants.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core/meta/support/MetaUtility.h <span style="color: grey">(b161bdc)</span></li>
<li>src/core/meta/support/MetaUtility.cpp <span style="color: grey">(0bb29db)</span></li>
<li>src/dynamic/Bias.h <span style="color: grey">(6ec60b6)</span></li>
<li>src/dynamic/Bias.cpp <span style="color: grey">(dec2db2)</span></li>
<li>src/playlistgenerator/PresetModel.cpp <span style="color: grey">(9b74636)</span></li>
<li>src/playlistgenerator/constraints/PlaylistDuration.h <span style="color: grey">(992215c)</span></li>
<li>src/playlistgenerator/constraints/PlaylistDuration.cpp <span style="color: grey">(5d83dfe)</span></li>
<li>src/playlistgenerator/constraints/TagMatch.h <span style="color: grey">(094a68b)</span></li>
<li>src/playlistgenerator/constraints/TagMatch.cpp <span style="color: grey">(a719825)</span></li>
<li>src/playlistgenerator/constraints/TagMatchEditWidget.ui <span style="color: grey">(b9d97c6)</span></li>
<li>src/playlistgenerator/constraints/TagMatchSupport.cpp <span style="color: grey">(d368931)</span></li>
<li>src/widgets/MetaQueryWidget.h <span style="color: grey">(c720b6d)</span></li>
<li>src/widgets/MetaQueryWidget.cpp <span style="color: grey">(ae07e6b)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/100070/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>