<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/112266/">http://git.reviewboard.kde.org/r/112266/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 25th, 2013, 12:06 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/112266/diff/1/?file=184486#file184486line33" style="color: black; font-weight: bold; text-decoration: underline;">shared/tag_helpers/APETagHelper.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">33</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">m_fieldMap</span><span class="p">.</span><span class="n">insert</span><span class="p">(</span> <span class="n">Meta</span><span class="o">::</span><span class="n">valAlbumArtist</span><span class="p">,</span> <span class="n">TagLib</span><span class="o">::</span><span class="n">String</span><span class="p">(</span> <span class="s">"A<span class="hl">lbum Artist</span>"</span> <span class="p">)</span> <span class="p">);</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">33</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">m_fieldMap</span><span class="p">.</span><span class="n">insert</span><span class="p">(</span> <span class="n">Meta</span><span class="o">::</span><span class="n">valAlbumArtist</span><span class="p">,</span> <span class="n">TagLib</span><span class="o">::</span><span class="n">String</span><span class="p">(</span> <span class="s">"A<span class="hl">LBUM ARTIST</span>"</span> <span class="p">)</span> <span class="p">);</span></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">34</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">m_fieldMap</span><span class="p">.</span><span class="n">insert</span><span class="p">(</span> <span class="n">Meta</span><span class="o">::</span><span class="n">valBpm</span><span class="p">,</span> <span class="n">TagLib</span><span class="o">::</span><span class="n">String</span><span class="p">(</span> <span class="s">"BPM"</span> <span class="p">)</span> <span class="p">);</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">34</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">m_fieldMap</span><span class="p">.</span><span class="n">insert</span><span class="p">(</span> <span class="n">Meta</span><span class="o">::</span><span class="n">valBpm</span><span class="p">,</span> <span class="n">TagLib</span><span class="o">::</span><span class="n">String</span><span class="p">(</span> <span class="s">"BPM"</span> <span class="p">)</span> <span class="p">);</span></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">35</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">m_fieldMap</span><span class="p">.</span><span class="n">insert</span><span class="p">(</span> <span class="n">Meta</span><span class="o">::</span><span class="n">valCompilation</span><span class="p">,</span> <span class="n">TagLib</span><span class="o">::</span><span class="n">String</span><span class="p">(</span> <span class="s">"C<span class="hl">ompilation</span>"</span> <span class="p">)</span> <span class="p">);</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">35</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">m_fieldMap</span><span class="p">.</span><span class="n">insert</span><span class="p">(</span> <span class="n">Meta</span><span class="o">::</span><span class="n">valCompilation</span><span class="p">,</span> <span class="n">TagLib</span><span class="o">::</span><span class="n">String</span><span class="p">(</span> <span class="s">"C<span class="hl">OMPILATION</span>"</span> <span class="p">)</span> <span class="p">);</span></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">36</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">m_fieldMap</span><span class="p">.</span><span class="n">insert</span><span class="p">(</span> <span class="n">Meta</span><span class="o">::</span><span class="n">valComposer</span><span class="p">,</span> <span class="n">TagLib</span><span class="o">::</span><span class="n">String</span><span class="p">(</span> <span class="s">"Composer"</span> <span class="p">)</span> <span class="p">);</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">36</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">m_fieldMap</span><span class="p">.</span><span class="n">insert</span><span class="p">(</span> <span class="n">Meta</span><span class="o">::</span><span class="n">valComposer</span><span class="p">,</span> <span class="n">TagLib</span><span class="o">::</span><span class="n">String</span><span class="p">(</span> <span class="s">"Composer"</span> <span class="p">)</span> <span class="p">);</span></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">37</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">m_fieldMap</span><span class="p">.</span><span class="n">insert</span><span class="p">(</span> <span class="n">Meta</span><span class="o">::</span><span class="n">valDiscNr</span><span class="p">,</span> <span class="n">TagLib</span><span class="o">::</span><span class="n">String</span><span class="p">(</span> <span class="s">"D<span class="hl">isc</span>"</span> <span class="p">)</span> <span class="p">);</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">37</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">m_fieldMap</span><span class="p">.</span><span class="n">insert</span><span class="p">(</span> <span class="n">Meta</span><span class="o">::</span><span class="n">valDiscNr</span><span class="p">,</span> <span class="n">TagLib</span><span class="o">::</span><span class="n">String</span><span class="p">(</span> <span class="s">"D<span class="hl">ISC</span>"</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;">Hmm, is there a specification somewhere that says what the proper identifiers are? I fear of backwards compatibility, perhaps there are files out there that use the title-cased identifiers?</pre>
</blockquote>
<p>On August 25th, 2013, 2:34 p.m. UTC, <b>Bruno Léon</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;">I did not find specifications for this.
Actually when using "Album Artist" instead of "ALBUM ARTIST", the tag is not read at all.
Taglib output it as "ALBUM ARTIST" which made me make this correction (and checked it actually works).
Same this for compilation. This makes Amarok compliant with Taglib output format.</pre>
</blockquote>
<p>On August 25th, 2013, 2:38 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;">I don't think that TagLib itself is origin of the string, I think it just reads the identifier that is in the actual tag (which can vary file by file). Or perhaps you can prove me wrong by pointing out relevant part of TagLib source code?</pre>
</blockquote>
<p>On August 25th, 2013, 2:55 p.m. UTC, <b>Mark Kretschmann</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;">Here's the spec: http://wiki.hydrogenaudio.org/index.php?title=APE_Tag_Item
And here is the list of valid keys: http://wiki.hydrogenaudio.org/index.php?title=APE_key
Keys are case sensitive, first character uppercase, rest lowercase (e.g. "Artist"). So it looks like the all-uppercase stuff must come from TagLib, and that our original code used the keys from the actual spec (minus "Album Artist" and "BPM", which don't exist in the spec at all).</pre>
</blockquote>
<p>On August 25th, 2013, 2:58 p.m. UTC, <b>Mark Kretschmann</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;">Ergo, probably Matej is right, TagLib just reads the identifiers from the file, and the files you have are actually incompatible with the spec. We could simply toLower() the strings before comparing.</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;">Taglib doc (http://taglib.github.io/api/classTagLib_1_1APE_1_1Tag.html) states that key are case-insensitive.
I'm not sure that Hydrogen Audio page is really "the spec", these are just commnly used keys.
I attached a sample output from Taglib.
As Matej says it probably just reads the identifier that is in the tag, but it outputs in UPPER CASE.
What I can assure is that if I tag my file either with MusicBrainz or directly in python using mutagen, without my proposed modifications discussed tags are not seen in Amarok.</pre>
<br />
<p>- Bruno</p>
<br />
<p>On August 25th, 2013, 11:17 a.m. UTC, Bruno Léon 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.</div>
<div>By Bruno Léon.</div>
<p style="color: grey;"><i>Updated Aug. 25, 2013, 11:17 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;">Fix reading of Album Artist and Compilation tag in APE tags.
Add support for reading Disc Number in APE tags.</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 Musepack files (that do use APE tags)</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>shared/tag_helpers/APETagHelper.cpp <span style="color: grey">(c628694)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/112266/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>