<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/107624/">http://git.reviewboard.kde.org/r/107624/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Okay, I've found some time to review the code, it contains many overdue clean-ups, good work, Ralf! Unambiguously positive changes. Some smaller remarks and merging consideration:
* [minor] Please strip off newly added DEBUG_BLOCKs, our debugging spam is excessive
* [minor] I've seen static_cast<EnumType>( int something ); <- constructor-like cast is preferred in this case as it behaves better wrt refactoring: EnumType enum = EnumType( int );
* the patch contains i18n changes. We're in string freeze, so it cannot be merged as-is. It either needs for wait for 2.8 (fine with me) or the i18n changes would need to be (temporarily) stripped off. (could be a headache) Ralf, what would you prefer?
Ralf, I still prefer reviewing code here on reviewboard, could you please post the combined patch of your branch? `post-review --branch=$(git symbolic-ref HEAD | cut -d/ -f 3) --username=rengels --tracking-branch master --parent=$(git merge-base HEAD master) --open -r 107624` ran from the branch should work.</pre>
<br />
<p>- Matěj</p>
<br />
<p>On December 7th, 2012, 11:44 a.m., Ralf Engels 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.</div>
<div>By Ralf Engels.</div>
<p style="color: grey;"><i>Updated Dec. 7, 2012, 11:44 a.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;">Several refactoring changes centering around token pool, token, token drop target and so on.
1. splitting up the FilenameLayoutDialog (which was not a dialog at all) into two widgets to be used by the actual dialog.
2. changing the token pool from an icon list to a normal list to prevent the use from having to scroll so much.
3. simplifying the drop target so that it does not need to install event filters for it's parents.
4. fixing small issues in the token pool, token and token drop target so that they have sensible minimumSizeHint and sizeHints
5. aligning texts between the different token users. No longer different texts.
6. for the edit filter dialog changes to the layout to get rid of the space waste in the result area.
7. for the guess tag dialog I got rid of some empty areas and some useless settings (settings that the dialog could determine itself)
8. for the playlist layout dialog not much has changed except that we don't need the event-filter parent mechanism.
Have a look at the attached screenshots to see the differences.
The actual code changes are in the rengels-filenameLayoutDialog branch.
Further work: Settings and presets need a reworking, as we have several sets currently which is confusing.
</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>ChangeLog <span style="color: grey">(15a698c)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/107624/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/107624/s/878/"><img src="http://git.reviewboard.kde.org/media/uploaded/images/2012/12/07/old-layout-editor_400x100.png" style="border: 1px black solid;" alt="Old Layout editor dialog" /></a>
<a href="http://git.reviewboard.kde.org/r/107624/s/879/"><img src="http://git.reviewboard.kde.org/media/uploaded/images/2012/12/07/new-layout-editor_400x100.png" style="border: 1px black solid;" alt="New Layout editor dialog" /></a>
<a href="http://git.reviewboard.kde.org/r/107624/s/880/"><img src="http://git.reviewboard.kde.org/media/uploaded/images/2012/12/07/old-guess-tags_400x100.png" style="border: 1px black solid;" alt="Old Guess Tag Dialog" /></a>
<a href="http://git.reviewboard.kde.org/r/107624/s/881/"><img src="http://git.reviewboard.kde.org/media/uploaded/images/2012/12/07/new-guess-tags_400x100.png" style="border: 1px black solid;" alt="New Guess Tag Dialog" /></a>
<a href="http://git.reviewboard.kde.org/r/107624/s/882/"><img src="http://git.reviewboard.kde.org/media/uploaded/images/2012/12/07/old-edit-filter_400x100.png" style="border: 1px black solid;" alt="Old Edit Filter Dialog" /></a>
<a href="http://git.reviewboard.kde.org/r/107624/s/883/"><img src="http://git.reviewboard.kde.org/media/uploaded/images/2012/12/07/new-edit-filter_400x100.png" style="border: 1px black solid;" alt="New Edit Filter Dialog" /></a>
</div>
</td>
</tr>
</table>
</div>
</body>
</html>