<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/105290/">http://git.reviewboard.kde.org/r/105290/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On April 18th, 2013, 12:11 p.m. UTC, <b>Matěj Laitl</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/105290/diff/5/?file=139513#file139513line199" style="color: black; font-weight: bold; text-decoration: underline;">src/musicbrainz/MusicBrainzTagsModel.cpp</a>
<span style="font-weight: normal;">
(Diff revision 5)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">199</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">emit</span> <span class="nf">dataChanged</span><span class="p">(</span> <span class="n">createIndex</span><span class="p">(</span> <span class="mi">0</span><span class="p">,</span> <span class="mi">0</span><span class="p">,</span> <span class="n">itemParent</span><span class="o">-></span><span class="n">child</span><span class="p">(</span> <span class="mi">0</span> <span class="p">)</span> <span class="p">),</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">200</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">createIndex</span><span class="p">(</span> <span class="n">lastRow</span><span class="p">,</span> <span class="mi">0</span><span class="p">,</span> <span class="n">itemParent</span><span class="o">-></span><span class="n">child</span><span class="p">(</span> <span class="n">lastRow</span> <span class="p">)</span> <span class="p">)</span> <span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">It would be a tiny bit safer to use index() instead of createIndex() at various places like this (i.e. have createIndex() only in actual index() implementation). Not a big deal though, certainly doesn't block this patch from merging.</pre>
</blockquote>
<p>On April 20th, 2013, 12:35 a.m. UTC, <b>Alberto Villa</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;">In some methods where no indexes are used (e.g., chooseBestMatches()) createIndex() is much easier to use. Anyway, I'll check this after the commit.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I've fixed this, now createIndex() is in index() only.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On April 18th, 2013, 12:11 p.m. UTC, <b>Matěj Laitl</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/105290/diff/5/?file=139513#file139513line251" style="color: black; font-weight: bold; text-decoration: underline;">src/musicbrainz/MusicBrainzTagsModel.cpp</a>
<span style="font-weight: normal;">
(Diff revision 5)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">251</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">beginResetModel</span><span class="p">();</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">252</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">m_rootItem</span><span class="o">-></span><span class="n">appendChild</span><span class="p">(</span> <span class="k">new</span> <span class="n">MusicBrainzTagsItem</span><span class="p">(</span> <span class="n">m_rootItem</span><span class="p">,</span> <span class="n">track</span><span class="p">,</span> <span class="n">tags</span> <span class="p">)</span> <span class="p">);</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">253</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">endResetModel</span><span class="p">();</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Should be that easy (if this is *the* thing on your TODO list):
beginInsertRows( QModelIndex(), rowCount(), rowCount() );
...
endInsertRows();</pre>
</blockquote>
<p>On April 20th, 2013, 12:35 a.m. UTC, <b>Alberto Villa</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;">Not that easy, if I understand it correctly. appendChild() isn't as simple as adding a result to the bottom; it may add results to each track, or even merge them (which means that no result is appended).</pre>
</blockquote>
<p>On April 20th, 2013, 11:51 p.m. UTC, <b>Matěj Laitl</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;">Oh, I see. It may then make sense for appendChild() to return some kind of this information.
Also note that according to my research, when adding an item that itself has children, it suffices to call begin/endInsertRows() only for the "parent" added item (provided it already has the children when endInsertRows() is called). Also I think that emitting more dataUpdated() than necessary just sacrifices a few cycles, while emitting less than necessary will probably cause view not to be updated properly.</pre>
</blockquote>
<p>On April 21st, 2013, 8:02 a.m. UTC, <b>Alberto Villa</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;">> Oh, I see. It may then make sense for appendChild() to return some kind of this information.
That's what I plan to do.
> Also note that according to my research, when adding an item that itself has children, it suffices to call begin/endInsertRows() only for the "parent" added item (provided it already has the children when endInsertRows() is called).
Sure.
> Also I think that emitting more dataUpdated() than necessary just sacrifices a few cycles, while emitting less than necessary will probably cause view not to be updated properly.
I don't understand: am I skipping some dataUpdated()?</pre>
</blockquote>
<p>On April 21st, 2013, 10:04 a.m. UTC, <b>Matěj Laitl</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 I think that emitting more dataUpdated() than necessary just sacrifices a few cycles, while emitting less than necessary will probably cause view not to be updated properly.
>
> I don't understand: am I skipping some dataUpdated()?
No, no, just mentioned that so you don't fear calling it perhaps more than strictly needed (perhaps to keep the code simple) -- but even that that doesn't seem to be an issue.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">According to my tests, it should be fixed now.</pre>
<br />
<p>- Alberto</p>
<br />
<p>On April 22nd, 2013, 5:57 p.m. UTC, Alberto Villa wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Amarok and Sergey Ivanov.</div>
<div>By Alberto Villa.</div>
<p style="color: grey;"><i>Updated April 22, 2013, 5:57 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;">The attached patch addresses several issues and brings some improvements, listed below.
Web service 2 is now being used in place of the deprecated version 1: disc number and artist credit are now better defined. This alone required a complete rewrite of MusicBrainzXmlParser.(cpp|h).
Make track search query more complicated and let it handle some mistakes (documented in code).
As in web service 2 multiple artists (e.g., Hans Zimmer & James Newton Howard) are returned splitted (i.e., they do not have one page, but each one has its own), the "Go to Artist Page" menu button was changed to a KActionMenu to support showing a sub list.
Get track title from release information, as different releases can change track title (e.g., adding (single edit) or similar).
Instead of doing *release* requests to get album artist and year, do *release group* requests. It often means fewer requests will be done (and each one takes at least 1 second, so...).
See http://tickets.musicbrainz.org/browse/SEARCH-214 and http://tickets.musicbrainz.org/browse/SEARCH-218
Reimplement Levenshtein distance algorithm using a more efficient version (pirated from Wikibooks as well as the current one).
Tracks without results are now listed and disabled, to let user know what results are missing. Prior to this, you had to count them by hand.
File name is shown in track tooltip. Helpful when existing tags are equivocal.
Selected results are shown in bold font to be easily spotted.
Track number, track count, disc number (when existing) and release year (first release date from the release group, actually) are now shown to help distinguish among results. This means that all the fetched information is now shown to the user.
Tracks are sorted by file name (fair choice). It's done after each insertion, but since insertions are quite sparse, it doesn't cause any performance problem.
Toolbar features "Select Best Matches" and "Deselect", the latter being new, and the former being moved here from the hidden header button. To keep it not too wide, Expand and Collapse buttons were modified to be KActionMenu. They now require two clicks, but are easier to distinguish (I always had to stop and read them to distinguish between "Collapse All" and "Collapse Chosen", for example), so the time needed to click them looks mostly the same as before.
The progress bar now gets to 100% even if MusicDNS search is disabled.
All releases per track are processed instead of only the best one. I'll explain better. Each track is associated to several releases. In current code, all the releases are parsed, but only the one with the highest score is returned to the dialog. This excludes lots of good results. Now, all of them can be returned. This obviously can increase the number of returned resuts, but that's acceptable given that, before, you were not able to tag a lot of tracks. Also, two following paragraphs are about two features that mitigate this "problem".
Tracks with the same visible information are merged. There's no point in showing several tracks with same title, artist, album name and artist, album year, disc number, track count and track number, as the user will not be able to distinguish among them. Just merge them into only one result, keeping a list of artist, release and track IDs (they are and will be used). Also, the score is updated to reflect the higher one. The "Go to ... Page" menu buttons currently link to the top result (i.e., highest scoring) IDs, but in the future I might add support for showing a list.
A "Select Best Matches from This Album" menu button was added. It matches the top result release ID in other *unselected* results, to make album tagging much easier. Since a result might reference many release IDs (see the paragraph above), the match is done on the whole list of them. This is highly recommended over the "Select Best Matches" toolbar button.
Process MusicDNS results just like MusicBrainz ones (i.e., do not duplicate the logic). Simply, as they will not carry existing tags, they'll end up being checked only by track length.
Rewrite MusicDNS result matching method in the list view. Actually, remove it (it was checking for equal track ID, but that is a very weak method, especially after my updates), and use the matching system described above.
Make MusicBrainzTagsView implementation agnostic (i.e., never use MusicBrainzTagsItem inside it, and make use of UserRole).
Remove stale code from MusicBrainzFinder.cpp and MusicBrainzXmlParser.(cpp|h) dealing with MBID requests (talked to maintainer about this).
Move and rename methods to enhance consistency and tidyness. Source files are a bit easier to navigate now.
Adapt to global Amarok coding style.
That's about it. The attached screenshot shows most of the visible updates.
Even if the patch rewrites many parts of the tagger, this work was possible only thanks to the solid foundations made by Sergey, who I thank for his work.</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">(6156a83)</span></li>
<li>src/dialogs/MusicBrainzTagger.h <span style="color: grey">(5fa0271)</span></li>
<li>src/dialogs/MusicBrainzTagger.cpp <span style="color: grey">(9ac99a6)</span></li>
<li>src/dialogs/MusicBrainzTagger.ui <span style="color: grey">(6762418)</span></li>
<li>src/musicbrainz/MusicBrainzFinder.h <span style="color: grey">(db8cb05)</span></li>
<li>src/musicbrainz/MusicBrainzFinder.cpp <span style="color: grey">(6635d04)</span></li>
<li>src/musicbrainz/MusicBrainzMeta.h <span style="color: grey">(5cbd190)</span></li>
<li>src/musicbrainz/MusicBrainzTags.h <span style="color: grey">(f70d494)</span></li>
<li>src/musicbrainz/MusicBrainzTags.cpp <span style="color: grey">(c90c2f8)</span></li>
<li>src/musicbrainz/MusicBrainzTagsItem.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/musicbrainz/MusicBrainzTagsItem.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/musicbrainz/MusicBrainzTagsModel.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/musicbrainz/MusicBrainzTagsModel.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/musicbrainz/MusicBrainzTagsModelDelegate.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/musicbrainz/MusicBrainzTagsModelDelegate.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/musicbrainz/MusicBrainzTagsView.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/musicbrainz/MusicBrainzTagsView.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/musicbrainz/MusicBrainzXmlParser.h <span style="color: grey">(171a340)</span></li>
<li>src/musicbrainz/MusicBrainzXmlParser.cpp <span style="color: grey">(473d27a)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/105290/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments </h1>
<ul>
<li><a href="http://git.reviewboard.kde.org/media/uploaded/files/2013/04/15/snapshot1.png">Toolbar with icons and QToolButtons</a></li>
<li><a href="http://git.reviewboard.kde.org/media/uploaded/files/2013/04/18/toolbar_wider.png">New toolbar with latest icons and descriptions.</a></li>
</ul>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Screenshots </h1>
<div>
<a href="http://git.reviewboard.kde.org/r/105290/s/605/"><img src="http://git.reviewboard.kde.org/media/uploaded/images/2012/06/18/mbtagger_400x100.png" style="border: 1px black solid;" alt="" /></a>
</div>
</td>
</tr>
</table>
</div>
</body>
</html>