astippich added a comment.
In D17302#417646 <https://phabricator.kde.org/D17302#417646>, @bruns wrote: > In D17302#410231 <https://phabricator.kde.org/D17302#410231>, @astippich wrote: > > > In D17302#410189 <https://phabricator.kde.org/D17302#410189>, @bruns wrote: > > > > > Currently, both > > > `Result::add(prop, "value1"); Result::add(prop, "value2");` > > > and > > > `Result::add(prop, {"value1", "value2"});` > > > are serialized (JSON) in the same way as `{prop: ["value1", "value2"]}` by Baloo, which is IMHO fine. > > > > > > 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. > > The result after deserialization is then a QVariantList with the values, and probably no client currently handles that since they expect no list. > > IMHO Baloo should not alter the output of KFileMetaData in any way (e.g. merging, which it currently relies on for stringlist). > > > No, this is no issue. **Either** the client uses `QMap::find()`, and it will get exactly one value (QVariant), **or** it uses `QMap::values()` and it will **always** get a list (QVariantList == QList<QVariant>) - which may have a single element. I disagree. Right now, clients (Dolphin, Baloo-widgets, Elisa) only use QMap::find(), ignoring all other entries. Currently, properties of the integer are merged into list and will be given as a single property of QVariantList after deserialization. The clients then directly call QVariant::toInt()/toDouble() etc., which fails if the result is a QVariantList. (This will be fixed by D19087 <https://phabricator.kde.org/D19087> and D19088 <https://phabricator.kde.org/D19088>) >>> On the other hand, >>> `Result::add(prop, "value1"); Result::add(prop, {"value2", "value3"});` >>> ends up as `{prop: ["value1", ["value2", "value3"] ]}`, which is nonsense, should be `{prop: ["value1", "value2", "value3"]}` >> >> I have patches ready for that and was waiting on this revision to land in order to continue... >> 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. >> In that case, the entries may also be duplicated and merging is not a good idea. >> >>> After deserializing, we should have `KFM::Propertymap pm.values(prop)` -> `QList<QVariant<QString>>({"value1", "value2", "value3"})`. >>> >>> I am currently moving the serialization (in file/result.cpp) /deserialization (in file/file.cpp) into a separate function so it becomes testable. >> >> Anyways, the test tests how Baloo::Result _currently_ behaves. If another patch then modifies this behavior, the test should be changed when it lands. > > No, it codifies that both, `add(value1); add(value2)` and `add({value1, value2})` have the same result. And next you say this is wrong ... Iff it is wrong, write down the *correct* behavior and make it QEXPECT_FAIL. Well, that is the current behavior, after deserialization you cannot differentiate between these two. And it is actually a nice behavior considering a smooth transition period. It ensures that multiple entries, falsely added individually by the extractors, are read as list, which is expected by the clients. If the extractors are switched to output stringlists instead of multiple properties, the same output is generated. Once all extractors are switched, the merging of the values can be removed, and the QMap will not be changed when serialized/deserialized. I can mark the merging of the strings as QEXPECT_FAIL if you prefer, but it looked wrong to me as the clients currently rely on that behavior. REPOSITORY R293 Baloo REVISION DETAIL https://phabricator.kde.org/D17302 To: astippich, #baloo, bruns Cc: kde-frameworks-devel, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
