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

This revision was automatically updated to reflect the committed changes. Closed by commit R293:ecebbf85dc91: Add standalone conversion functions for PropertyMap to Json and vice versa (authored by bruns). REPOSITORY R293 Baloo CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19087?vs=

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

ngraham accepted this revision. REPOSITORY R293 Baloo BRANCH submit REVISION DETAIL https://phabricator.kde.org/D19087 To: bruns, #baloo, #frameworks, ngraham, poboiko, astippich Cc: mgallien, kde-frameworks-devel, lots0logs, LeGast00n, fbampaloukas, GB_2, domson, ashaposhnikov, michaelh

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

astippich accepted this revision. astippich added a comment. This revision is now accepted and ready to land. Since released versions of dolphin, baloo-widgets and elisa now actually cope with this case, I am no longer opposed merging this REPOSITORY R293 Baloo BRANCH submit REVISION DE

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

astippich added a comment. Are you still interested in working on this? If not, I would like to take over with the following plan: I would like to propose a transition phase, e.g. we make sure that there is a release available that can handle both formats. This diff lands with my requeste

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

astippich added inline comments. INLINE COMMENTS > propertydata.cpp:88 > +if (it.value().isArray()) { > +const auto values = it.value().toArray(); > +for (const auto val : values) { Can you return arrays as lists here so that current behavior is unchanged, and pu

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

astippich added a subscriber: mgallien. astippich added a comment. My main concern is that this changes current behavior and as such is not backwards compatible. The rest is a bonus. That is why my plan was to make the extractors output (string-)lists as outlined in T8196

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

bruns updated this revision to Diff 55103. bruns added a comment. correct comments REPOSITORY R293 Baloo CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19087?vs=54768&id=55103 BRANCH submit REVISION DETAIL https://phabricator.kde.org/D19087 AFFECTED FILES autotests/unit/f

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

bruns added a comment. On the topic of "multiple values from one extractor" vs "multiple values from different extractors": I think, when this problem arrives, it should be solved by using one PropertyMap per extractor and explicitly merging these. 1. For per-extractor unique proper

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

bruns added a comment. In D19087#440361 , @astippich wrote: > This still alters the map during deserialization. Lists are converted into multi single properties afterwards. As I said, due to backwards-compatibility lists should stay lists.

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

astippich requested changes to this revision. astippich added a comment. This revision now requires changes to proceed. This still alters the map during deserialization. Lists are converted into multi single properties afterwards. As I said, due to backwards-compatibility lists should stay li

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

bruns marked an inline comment as done. REPOSITORY R293 Baloo REVISION DETAIL https://phabricator.kde.org/D19087 To: bruns, #baloo, #frameworks, ngraham, poboiko, astippich Cc: kde-frameworks-devel, gennad, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams

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

bruns updated this revision to Diff 54768. bruns added a comment. add list tests REPOSITORY R293 Baloo CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19087?vs=52401&id=54768 BRANCH submit REVISION DETAIL https://phabricator.kde.org/D19087 AFFECTED FILES autotests/unit/fil

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

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 , which is why I am so insisting.

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

bruns updated this revision to Diff 52401. bruns marked an inline comment as done. bruns added a comment. add support for multiple integer values add multiple integer test REPOSITORY R293 Baloo CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19087?vs=51928&id=52401 BRANCH subm

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

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 th

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

astippich added inline comments. INLINE COMMENTS > bruns wrote in propertydata.cpp:80 > Actually, I am not sure **if** we need the stringlists as a separate type.. > What is the difference between multiple strings in a stringlist, and multiple > strings as individual dict entries? > > What is

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

bruns marked an inline comment as done. bruns added inline comments. INLINE COMMENTS > astippich wrote in propertydata.cpp:80 > This means that multiple entries with the same key get stored as single > entries in the property map, right? > This breaks the current behavior in dolphin and baloo-w

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

bruns added a comment. In D19087#415703 , @astippich wrote: > The stringlist case is still not handled. Thats the reason I have not marked the comment as done ... > And a test case for multiple ints (2 ReleaseYear properties) will al

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

astippich requested changes to this revision. astippich added a comment. This revision now requires changes to proceed. The stringlist case is still not handled. And a test case for multiple ints (2 ReleaseYear properties) will also likely fail. When going with JSON, I think this has to be do

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

bruns marked an inline comment as done. REPOSITORY R293 Baloo REVISION DETAIL https://phabricator.kde.org/D19087 To: bruns, #baloo, #frameworks, ngraham, poboiko, astippich Cc: kde-frameworks-devel, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams

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

bruns updated this revision to Diff 51928. bruns marked an inline comment as done. bruns added a comment. Add `double` test case Fix double conversion Fix nitpicks ('*') REPOSITORY R293 Baloo CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19087?vs=51886&id=51928 BRANCH subm

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

astippich added a comment. I was wondering if using QDataStream would be simpler, but the data size was over twice the amount compared to the size using JSON during my testing. But this would solve a lot of the limitations which JSON imposes, like handling int and doubles, qdatetime objects,

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

bruns added a dependent revision: D19088: Use propertymap to store properties in Baloo::Result. REPOSITORY R293 Baloo REVISION DETAIL https://phabricator.kde.org/D19087 To: bruns, #baloo, #frameworks, ngraham, poboiko, astippich Cc: kde-frameworks-devel, ashaposhnikov, michaelh, astippich,

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

bruns created this revision. bruns added reviewers: Baloo, Frameworks, ngraham, poboiko, astippich. Herald added projects: Frameworks, Baloo. Herald added a subscriber: kde-frameworks-devel. bruns requested review of this revision. REVISION SUMMARY Baloo caches property data, and uses Json for s