<table><tr><td style="">astippich added a comment.
</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/D17302">View Revision</a></tr></table><br /><div><div><blockquote style="border-left: 3px solid #8C98B8;
          color: #6B748C;
          font-style: italic;
          margin: 4px 0 12px 0;
          padding: 8px 12px;
          background-color: #F8F9FC;">
<div style="font-style: normal;
          padding-bottom: 4px;">In <a href="https://phabricator.kde.org/D17302#410189" style="background-color: #e7e7e7;
          border-color: #e7e7e7;
          border-radius: 3px;
          padding: 0 4px;
          font-weight: bold;
          color: black;text-decoration: none;">D17302#410189</a>, <a href="https://phabricator.kde.org/p/bruns/" style="
              border-color: #f1f7ff;
              color: #19558d;
              background-color: #f1f7ff;
                border: 1px solid transparent;
                border-radius: 3px;
                font-weight: bold;
                padding: 0 4px;">@bruns</a> wrote:</div>
<div style="margin: 0;
          padding: 0;
          border: 0;
          color: rgb(107, 116, 140);"><p>Currently, both<br />
 <tt style="background: #ebebeb; font-size: 13px;">Result::add(prop, "value1"); Result::add(prop, "value2");</tt><br />
and<br />
 <tt style="background: #ebebeb; font-size: 13px;">Result::add(prop, {"value1", "value2"});</tt><br />
 are serialized (JSON) in the same way as <tt style="background: #ebebeb; font-size: 13px;">{prop: ["value1", "value2"]}</tt> by Baloo, which is IMHO fine.</p></div>
</blockquote>

<p>This can lead to issues if e.g. two ReleaseYears are added from different extractors. That is probably impossible right now since KFileMetaData does not have multiple extractors for the same mimetype, but still. <br />
The result after deserialization is then a QVariantList with the values, and probably no client currently handles that since they expect no list.<br />
IMHO Baloo should not alter the output of KFileMetaData in any way (e.g. merging, which it currently relies on for stringlist).</p>

<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>On the other hand,<br />
 <tt style="background: #ebebeb; font-size: 13px;">Result::add(prop, "value1"); Result::add(prop, {"value2", "value3"});</tt><br />
 ends up as <tt style="background: #ebebeb; font-size: 13px;">{prop: ["value1", ["value2", "value3"] ]}</tt>, which is nonsense, should be <tt style="background: #ebebeb; font-size: 13px;">{prop: ["value1", "value2", "value3"]}</tt></p></blockquote>

<p>I have patches ready for that and was waiting on this revision to land in order to continue...<br />
But as stated above, I also changed my mind, I do not think this is a good idea. This can happen if there are two extractors for the same mimetype.<br />
In that case, the entries may also be duplicated and merging is not a good idea.</p>

<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>After deserializing, we should have <tt style="background: #ebebeb; font-size: 13px;">KFM::Propertymap pm.values(prop)</tt> -> <tt style="background: #ebebeb; font-size: 13px;">QList<QVariant<QString>>({"value1", "value2", "value3"})</tt>.</p>

<p>I am currently moving the serialization (in file/result.cpp) /deserialization (in file/file.cpp) into a separate function so it becomes testable.</p></blockquote>

<p>Anyways, the test tests how Baloo::Result _currently_ behaves. If another patch then modifies this behavior, the test should be changed when it lands.</p></div></div><br /><div><strong>REPOSITORY</strong><div><div>R293 Baloo</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D17302">https://phabricator.kde.org/D17302</a></div></div><br /><div><strong>To: </strong>astippich, Baloo, bruns<br /><strong>Cc: </strong>kde-frameworks-devel, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams<br /></div>