astippich added a comment.
I get the feeling that we need to discuss how the PropertyMap should look like and how we achieve a seamless transition in a backwards compatible way. I have slowly been working to resolve T8196 <https://phabricator.kde.org/T8196>, which is why I am so insisting. This is the last missing piece. Currently, the clients expect a list in the map when there are multiple values, probably since the inception of baloo. All other entries are simply ignored. If the map is changed to having multiple entries with duplicate keys, then we have to adjust all clients and make sure that all possible combinations of applications and frameworks continue to work. Additionally, baloo's serialization/deserialization does currently alter the structure of the map. Multiple values with the same key will be merged into a list afterwards. This causes issues in baloo-widgets for example, which breaks for multiple properties when baloo indexing is off. Baloo-widget then uses KFM directly, which results in a different PropertyMap. Considering this, my plan was that to insert multiple values as single entry containing a list. Once all extractors are converted, the merging of multiple values into lists, currently done before serialization in baloo, will be removed. This yields a backwards compatible way of fixing the issue with multiple entries (That is also what D17302 <https://phabricator.kde.org/D17302> is for). This demands that the same list is retrieved after deserialization, with no merged values, e.g. additional entries in the list, during the serialization. So there are a few things that I would like to consider: - The same PropertyMap is generated either by baloo or KFM directly. Keeping the PropertyMap the same is only possible to a certain extend with JSON, e.g. with ints becoming doubles etc... - Extractors will insert multiple entries as a list. - Multiple extractors for the same mime type may insert duplicate entries. This would allow clients to easily differentiate between data from multiple extractors in the future. INLINE COMMENTS > bruns wrote in propertydata.cpp:80 > > 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 > 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. Dolphin has its own handling for the detailed view and does not use baloo-widgets there. I think here is actually a misunderstanding, by supporting string lists I meant that if one inserts a list, one also gets the same list after deserialization. I actually do not care if it is a string list before and a variant list afterwards, that is how the current code already works. > 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(). You are right, ExtractionResult explicitly states that multiple calls are allowed. IMHO this should be more specified to "can be called multiple times from different extractors". > 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))). > QVariant::toStringList() also works on QVariantLists - > https://doc.qt.io/qt-5/qvariant.html#toStringList I know, otherwise the current code would not work at all. > bruns wrote in propertydata.cpp:83 > 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. Which property? I am only aware of Composer, which I plan to change. 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
