<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-109802">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;">No, I cannot use the the PropertyMap for ASF/MP4, those atoms/attributes are unsupported in the PropertyMap and need to be handled separately. I would have done so if it is possible.<br />
I really do not like to write unnecessary, duplicated code when TagLib handles this nicely for us. I've only chosen the rating property to be implemented first, but there are more to come. The code will be 100% the same for APE and OGG with the PropertyMap, and also more efficient as we query the PropertyMap anyway.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Taglib does not "handle this nicely". For APE and Xiph, it just accepts *any* unknown key and uses it verbatim, while for MP4 and ASF it rejects any unknown key.</p>

<p style="padding: 0; margin: 8px;">The setProperties() is also quite inconsistent, for APE and ASF it only removes items which have an empty value, while for Xiph, the properties are completely replaced.</p>

<p style="padding: 0; margin: 8px;">As soon as you add support for a property where APE and Xiph key naming differs, or is only supported by one, you will require two functions anyway.</p>

<p style="padding: 0; margin: 8px;">Using <tt style="background: #ebebeb; font-size: 13px;">APE::Tag::setItem(...)</tt> is as efficient as manipulating the key/value in the Taglib::PropertyMap first and setting it by <tt style="background: #ebebeb; font-size: 13px;">setProperties(...)</tt>. Likewise for Xiph.</p>

<p style="padding: 0; margin: 8px;">If you want to squeeze out the last bit of efficiency, you would skip the <tt style="background: #ebebeb; font-size: 13px;">setProperties(...)</tt> completely when no property is changed by <tt style="background: #ebebeb; font-size: 13px;">writeGenericProperties(....)</tt>. This happens e.g. if you only change the rating.</p>

<p style="padding: 0; margin: 8px;">If you want to avoid duplicate code, move the <tt style="background: #ebebeb; font-size: 13px;">properties()</tt>/<tt style="background: #ebebeb; font-size: 13px;">setProperties()</tt> into <tt style="background: #ebebeb; font-size: 13px;">writeGenericProperties()</tt>, that saves 12*2 lines and adds 2.</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, gennad, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams<br /></div>