<table><tr><td style="">mgallien requested changes to this revision.<br />mgallien added a comment.<br />This revision now requires changes to proceed.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D11365">View Revision</a></tr></table><br /><div><div><p>Please fix the issues.</p></div></div><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D11365#inline-58607">View Inline</a><span style="color: #4b4d51; font-weight: bold;">taglibextractor.cpp:363</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">if</span> <span class="p">(</span><span style="color: #aa2211">!</span><span class="n">genresList</span><span class="p">.</span><span class="n">isEmpty</span><span class="p">())</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">result</span><span style="color: #aa2211">-></span><span class="n">add</span><span class="p">(</span><span class="n">Property</span><span style="color: #aa2211">::</span><span class="n">Genre</span><span class="p">,</span> <span class="n">genresList</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span class="p">}</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Is it really needed to push directly a list ?<br />
You are ignoring the fact that the original developer intended add to be called once for each value. I would prefer to keep doing that until we can be absolutely sure that nothing breaks if we change that.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D11365#inline-58610">View Inline</a><span style="color: #4b4d51; font-weight: bold;">taglibextractor.cpp:370</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span style="color: #aa4000">const</span> <span class="bright"></span><span style="color: #aa4000"><span class="bright">auto</span></span> <span class="n">artists</span> <span style="color: #aa2211">=</span> <span class="n">contactsFromString</span><span class="p">(</span><span class="n">artistString</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="bright"></span><span style="color: #aa4000"><span class="bright">for</span></span><span class="bright"> </span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span style="color: #aa4000"><span class="bright">auto</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">&</span></span><span class="bright"> </span><span style="color: #a0a000"><span class="bright">a</span>rtist<span class="bright"></span></span><span class="bright"> </span><span class="p"><span class="bright">:</span></span> <span class="n">artists</span><span class="p">)<span class="bright"></span></span><span class="bright"> </span><span class="p"><span class="bright">{</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="n">result</span><span style="color: #aa2211">-></span><span class="n">add</span><span class="p">(</span><span class="n">Property</span><span style="color: #aa2211">::</span><span class="n">Artist</span><span class="p">,</span> <span class="n">artist</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="p">}</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">const</span> <span class="bright"></span><span class="n"><span class="bright">QStringList</span></span> <span class="n">artists</span> <span style="color: #aa2211">=</span> <span class="n">contactsFromString</span><span class="p">(</span><span class="n">artistString</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="bright"></span><span class="n"><span class="bright">result</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">-></span></span><span class="bright"></span><span class="n"><span class="bright">add</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span class="n"><span class="bright">Property</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">::</span></span><span class="bright"></span><span class="n"><span class="bright">A</span>rtist<span class="bright"></span></span><span class="bright"></span><span class="p"><span class="bright">,</span></span> <span class="n">artists</span><span class="p">)<span class="bright">;</span></span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">idem</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D11365#inline-58609">View Inline</a><span style="color: #4b4d51; font-weight: bold;">taglibextractor.cpp:374</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span style="color: #aa4000">const</span> <span class="bright"></span><span style="color: #aa4000"><span class="bright">auto</span></span> <span class="n">composers</span> <span style="color: #aa2211">=</span> <span class="n">contactsFromString</span><span class="p">(</span><span class="n">composersString</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="bright"></span><span style="color: #aa4000"><span class="bright">for</span></span><span class="bright"> </span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span style="color: #aa4000"><span class="bright">auto</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">&</span></span><span class="bright"> </span><span style="color: #a0a000"><span class="bright">comp</span></span><span class="bright"> </span><span class="p"><span class="bright">:</span></span> <span class="n">composers</span><span class="p">)<span class="bright"></span></span><span class="bright"> </span><span class="p"><span class="bright">{</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"><span class="bright"> </span><span class="n"><span class="bright">result</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">-></span></span><span class="bright"></span><span class="n"><span class="bright">add</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span class="n"><span class="bright">Property</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">::</span></span><span class="bright"></span><span class="n"><span class="bright">Composer</span></span><span class="bright"></span><span class="p"><span class="bright">,</span></span><span class="bright"> </span><span class="n"><span class="bright">comp</span></span><span class="bright"></span><span class="p"><span class="bright">);</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">const</span> <span class="bright"></span><span class="n"><span class="bright">QStringList</span></span> <span class="n">composers</span> <span style="color: #aa2211">=</span> <span class="n">contactsFromString</span><span class="p">(</span><span class="n">composersString</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="bright"></span><span class="n"><span class="bright">result</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">-></span></span><span class="bright"></span><span class="n"><span class="bright">add</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span class="n"><span class="bright">Property</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">::</span></span><span class="bright"></span><span class="n"><span class="bright">Composer</span></span><span class="bright"></span><span class="p"><span class="bright">,</span></span> <span class="n">composers</span><span class="p">)<span class="bright">;</span></span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">idem</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D11365#inline-58608">View Inline</a><span style="color: #4b4d51; font-weight: bold;">taglibextractor.cpp:385</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span style="color: #aa4000">const</span> <span class="bright"></span><span style="color: #aa4000"><span class="bright">auto</span></span> <span class="n">albumArtists<span class="bright">String</span></span> <span style="color: #aa2211">=</span> <span class="n">con<span class="bright">vertWCharsToQ</span>String</span><span class="p">(<span class="bright"></span></span><span class="bright"></span><span class="n"><span class="bright">data</span></span><span class="bright"></span><span class="p"><span class="bright">.</span></span><span class="n">albumArtists<span class="bright"></span></span><span class="bright"></span><span class="p"><span class="bright">).</span></span><span class="bright"></span><span class="n"><span class="bright">trimmed</span></span><span class="bright"></span><span class="p"><span class="bright">(</span>);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="bright"></span><span style="color: #aa4000"><span class="bright">const</span></span><span class="bright"> </span><span style="color: #aa4000"><span class="bright">auto</span></span><span class="bright"> </span><span class="n"><span class="bright">a</span>lbumArtist<span class="bright">s</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">=</span></span><span class="bright"> </span><span class="n"><span class="bright">contactsFromString</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="n">albumArtists<span class="bright">String</span></span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span style="color: #aa4000">for</span> <span class="p">(</span><span style="color: #aa4000">auto</span><span style="color: #aa2211">&</span> <span style="color: #a0a000">res</span> <span class="p">:</span> <span class="n">albumArtists</span><span class="p">)</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="n">result</span><span style="color: #aa2211">-></span><span class="n">add</span><span class="p">(</span><span class="n">Property</span><span style="color: #aa2211">::</span><span class="n">AlbumArtist</span><span class="p">,</span> <span class="n">res</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="p">}</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">const</span> <span class="bright"></span><span class="n"><span class="bright">QStringList</span></span> <span class="n">albumArtists</span> <span style="color: #aa2211">=</span> <span class="n">con<span class="bright">tactsFrom</span>String</span><span class="p">(</span><span class="n">albumArtists<span class="bright">String</span></span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="bright"></span><span class="n"><span class="bright">result</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">-></span></span><span class="bright"></span><span class="n"><span class="bright">add</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span class="n"><span class="bright">Property</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">::</span></span><span class="bright"></span><span class="n"><span class="bright">A</span>lbumArtist<span class="bright"></span></span><span class="bright"></span><span class="p"><span class="bright">,</span></span><span class="bright"> </span><span class="n">albumArtists</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span class="p">}</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">idem</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D11365#inline-58612">View Inline</a><span style="color: #4b4d51; font-weight: bold;">taglibextractor.cpp:389</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; "> <span style="color: #aa4000">if</span> <span class="p">(</span><span class="n">tags</span><span style="color: #aa2211">-></span><span class="n">track</span><span class="p">())</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="n">result</span><span style="color: #aa2211">-></span><span class="n">add</span><span class="p">(</span><span class="n">Property</span><span style="color: #aa2211">::</span><span class="n">TrackNumber</span><span class="p">,</span> <span class="n">tags</span><span style="color: #aa2211">-></span><span class="n">track</span><span class="p">());</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">result</span><span style="color: #aa2211">-></span><span class="n">add</span><span class="p">(</span><span class="n">Property</span><span style="color: #aa2211">::</span><span class="n">TrackNumber</span><span class="p">,</span> <span class="bright"></span><span style="color: #aa4000"><span class="bright">static_cast</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright"><</span></span><span class="bright"></span><span style="color: #aa4000"><span class="bright">int</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">></span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="n">tags</span><span style="color: #aa2211">-></span><span class="n">track</span><span class="p">(<span class="bright">)</span>));</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span class="p">}</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Are you sure we need this cast ? I do not think we will ever need to have negative track numbers added. The original type is unsigned int and should exactly convey the fact that we do not expect negative numbers.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D11365#inline-58611">View Inline</a><span style="color: #4b4d51; font-weight: bold;">taglibextractor.cpp:393</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; "> <span style="color: #aa4000">if</span> <span class="p">(</span><span class="n">tags</span><span style="color: #aa2211">-></span><span class="n">year</span><span class="p">())</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="n">result</span><span style="color: #aa2211">-></span><span class="n">add</span><span class="p">(</span><span class="n">Property</span><span style="color: #aa2211">::</span><span class="n">ReleaseYear</span><span class="p">,</span> <span class="n">tags</span><span style="color: #aa2211">-></span><span class="n">year</span><span class="p">());</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">result</span><span style="color: #aa2211">-></span><span class="n">add</span><span class="p">(</span><span class="n">Property</span><span style="color: #aa2211">::</span><span class="n">ReleaseYear</span><span class="p">,</span> <span class="bright"></span><span style="color: #aa4000"><span class="bright">static_cast</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright"><</span></span><span class="bright"></span><span style="color: #aa4000"><span class="bright">int</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">></span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="n">tags</span><span style="color: #aa2211">-></span><span class="n">year</span><span class="p">(<span class="bright">)</span>));</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span class="p">}</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">idem</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R286 KFileMetaData</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D11365">https://phabricator.kde.org/D11365</a></div></div><br /><div><strong>To: </strong>astippich, Frameworks, Baloo, mgallien, michaelh<br /><strong>Cc: </strong>michaelh, Frameworks, ashaposhnikov, astippich, spoorun, nicolasfella, ngraham, alexeymin<br /></div>