<table><tr><td style="">bruns added inline comments.
</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/D18601">View Revision</a></tr></table><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/D18601#inline-108473">View Inline</a><span style="color: #4b4d51; font-weight: bold;">astippich</span> wrote in <span style="color: #4b4d51; font-weight: bold;">taglibwriter.cpp:70</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Not sure I understand. I would like to avoid doing <tt style="background: #ebebeb; font-size: 13px;">file->properties();</tt> and <tt style="background: #ebebeb; font-size: 13px;">file->setProperties();</tt> twice for those formats for which only the PropertyMap is sufficient, e.g. Ape and Ogg, to avoid doing any unnecessary work.  The beauty of Ape and Ogg in the TagLib implementation is that they solely work with the PropertyMap and do not require any special handling. See <a href="https://phabricator.kde.org/D18826" style="background-color: #e7e7e7;
          border-color: #e7e7e7;
          border-radius: 3px;
          padding: 0 4px;
          font-weight: bold;
          color: black;text-decoration: none;">D18826</a>, Ape and Ogg do not have any extra codepath at all. For writing tags, we have to be a little bit more careful, though. If writing Rating information is added to writeGenericProperties, for example Id3v2's "TXXX" tags will be polluted with these values.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I think for <strong>reading</strong> it as generic as possible is fine, but for writing being a little bit more explicit does not hurt.</p>

<p style="padding: 0; margin: 8px;">My proposal for Ape and Ogg is to split the writing for these to two different functions. Yes, it <strong>is</strong> possible to use the propertymap for both, as both use the same scale and the same tag name, but the implementations on the taglib side are completely different.</p>

<p style="padding: 0; margin: 8px;">Writing the "rating" for XiphComment (Ogg) and  Ape in distinct functions (see my comment in <a href="https://phabricator.kde.org/D18604" style="background-color: #e7e7e7;
          border-color: #e7e7e7;
          border-radius: 3px;
          padding: 0 4px;
          font-weight: bold;
          color: black;text-decoration: none;">D18604</a>) hardly adds any code, but gets the Ape and Ogg code paths in line with the other file formats. You don't write the ASF/MP4 rating using the property interface although it would be possible, and IMHO thats the right thing to do,  also for Ape and Ogg.</p>

<p style="padding: 0; margin: 8px;">As soon Ape and Ogg are split, you no longer rely on the PropertyMap for the rating, and you won't have to use <tt style="background: #ebebeb; font-size: 13px;">properties()</tt>/<tt style="background: #ebebeb; font-size: 13px;">setProperties()</tt> twice.</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/D18601">https://phabricator.kde.org/D18601</a></div></div><br /><div><strong>To: </strong>astippich, bruns, mgallien, broulik, cfeck<br /><strong>Cc: </strong>kde-frameworks-devel, Baloo, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams<br /></div>