D19087: Add standalone conversion functions for PropertyMap to Json and vice versa

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> astippich wrote in propertydata.cpp:80
> Please incorporate support for stringlists.
> All clients currently rely on having string lists (or variantlists) as output for multiple entries. It is also what is advertised in KFileMetaData.
> None of the clients query for multiple entries in the PropertyMap, Dolphin and Baloo-widgets will break without this for multi-value properties. All of them add and format the output on a per property basis, so querying for multiple keys requires significant changes.
> IMHO all extractors should only add one property for each key to the map. If there are duplicate entries, this means that multiple extractors added results (with possible duplication).
> Having it as string lists also makes the handling of them so much easier. It can be handled with one call to QVariant::toStringList().join().

> All clients currently rely on having string lists (**or variantlists**)

Actually, string lists make the code more complicated, as the code already has to handle variant lists. There are no `QDoublelists`, `QIntlists` ... baloo-widgets treats a `QVariantList` with string entries exactly like a `QStringlist`, and dolphin uses baloo-widgets.

> It is also what is advertised in KFileMetaData.

KFileMetaData does not specify in any way how values for repeated properties are retuned. It explicitly allows calling ExtractionResult::add() with the same property multiple times, but that's it. PropertyMap is just a QMap<> typedef, and does not specify any behavior. For me, it seems much more natural to expect multiple individual entries for the same key, which should be retrieved with QMap::values().

Currently, baloo-widgets (https://cgit.kde.org/baloo-widgets.git/tree/src/filemetadatawidget.cpp#n152)  only uses `data[key]`, i.e. it only retrieves the first value, but fixing this is a single line change (i.e. replace `data[key]` by `QVariant(data.values(key))`).

> Having it as string lists also makes the handling of them so much easier. It can be handled with one call to QVariant::toStringList().join().

`QVariant::toStringList()` also works on `QVariantLists` - https://doc.qt.io/qt-5/qvariant.html#toStringList

> astippich wrote in propertydata.cpp:83
> True, and I said the same quite often in other reviews. But you did not let this count for me either :)
> Anyway, not a blocker for me.

Only if it can be fixed it a straight-forward and trivial way, or when it is a regression. This is IMHO not the case here. 
Also, PropertyInfo::valueType specifies `QString` for several properties where multiple values are completely reasonable.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D19087

To: bruns, #baloo, #frameworks, ngraham, poboiko, astippich
Cc: kde-frameworks-devel, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20190223/193e00ed/attachment-0001.html>