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
