Handle multiple entries in map
ClosedPublic

Authored by astippich on Apr 22 2019, 3:04 PM.

Details

Summary

Handle multiple entries in the property map
by converting to a map with single key entries with
lists. Obsoletes D11820 and fixes display of multi-value
properties using the extractor.

Diff Detail

Repository
R824 Baloo Widgets
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
astippich created this revision.Apr 22 2019, 3:04 PM
Restricted Application added a project: Baloo. · View Herald TranscriptApr 22 2019, 3:04 PM
Restricted Application added a subscriber: Baloo. · View Herald Transcript
astippich requested review of this revision.Apr 22 2019, 3:04 PM
astippich edited the summary of this revision. (Show Details)Apr 22 2019, 3:11 PM
astippich updated this revision to Diff 56861.Apr 24 2019, 5:47 AM
  • add missing const, bump Kf version
bruns requested changes to this revision.Apr 28 2019, 12:25 PM
bruns added inline comments.
src/propertymaputil_p.h
38 ↗(On Diff #56861)

This now has O(n^2) complexity. Please use a nested iterator approach.

This revision now requires changes to proceed.Apr 28 2019, 12:25 PM
astippich updated this revision to Diff 57136.Apr 28 2019, 3:05 PM
  • Use nested iterators
astippich updated this revision to Diff 57137.Apr 28 2019, 3:07 PM
  • Use nested iterators
  • correctly rebase
astippich added inline comments.Apr 28 2019, 3:08 PM
src/propertymaputil_p.h
38 ↗(On Diff #56861)

Something like this? If not, please provide some guidance

bruns added inline comments.Apr 28 2019, 5:59 PM
src/propertymaputil_p.h
38 ↗(On Diff #56861)

In general, yes.

But I think it becomes much nicer when using the algorithms from the standard library:

#include <QList>
#include <QMap>
#include <QString>
#include <QDebug>

#include <algorithm>

int main(int argc, char* argv[])
{
    QMultiMap<int, QString> map{{0, "zero_0"}, {0, "zero_1"},
                                {1, "one_0"}, {1, "one_1"}, {1, "one_2"},
                                {2, "two_0"}, {2, "two_1"}, {2, "two_2"}, {2, "two_3"},
                                {3, "three"}};

    // begin of real code
    auto begin = map.constKeyValueBegin();
    auto rangeEnd = map.constKeyValueBegin();

    while (begin != map.constKeyValueEnd()) {
        std::tie(begin, rangeEnd) = std::equal_range(begin, map.constKeyValueEnd(), *begin,
                [](const auto& a, const auto& b) {
                    return a.first < b.first;
                });
        auto distance = std::distance(begin, rangeEnd);
        if (distance > 1) {
            QList<QString> list;
            list.reserve(distance);
            std::for_each(begin, rangeEnd, [&list](const auto& s) { list.append(s.second); });
            qDebug() << "multi" << list;
        } else {
            qDebug() << "single" << (*begin).second;
        }
        begin = rangeEnd;
    }
}
bruns added inline comments.Apr 28 2019, 6:15 PM
src/propertymaputil_p.h
37 ↗(On Diff #57137)

There is obviously an error here - you wan't to break on ==.

astippich updated this revision to Diff 57213.Apr 29 2019, 8:10 PM
  • return early for empty map
  • use standard algorithm

Next try.
Thanks for the hints. I followed them as close as possible, but QMap iterators apparently do not have .first and .second members, so I adjusted it accordingly.

Next try.
Thanks for the hints. I followed them as close as possible, but QMap iterators apparently do not have .first and .second members, so I adjusted it accordingly.

QMap has STL style iterators, available through e.g. QMap::constKeyValueBegin(). Contrary to QMap::equal_range, std::equal_range allows to limit the range to search, e.g. with each processed key the search range shrinks.

astippich updated this revision to Diff 57336.May 1 2019, 5:40 PM
  • use more std::algorithms

Finally got it. Note that the compiler did not let me use auto in lambda functions

bruns added a comment.May 2 2019, 12:05 AM

Finally got it. Note that the compiler did not let me use auto in lambda functions

Unfortunately, that is only available since C++14, IIRC.

src/propertymaputil_p.h
39 ↗(On Diff #57336)

better: using entry = std::pair<...>;

46 ↗(On Diff #57336)

Just realized this can be even simpler:

while (begin != propMap.constKeyValueEnd()) {
    auto key = (*begin).first;
    KFileMetaData::PropertyInfo property(key);
    auto rangeEnd = std::find_if(begin, propMap.constKeyValueEnd(),
        [key](const entry& e) { return e.first != key; });
    ...
bruns added inline comments.May 2 2019, 12:12 AM
src/propertymaputil_p.h
28 ↗(On Diff #57336)

"In case a key has multiple values, all its values are collected in a QVariantList which is stored as a single entry."

bruns requested changes to this revision.May 3 2019, 12:24 AM
bruns added inline comments.
CMakeLists.txt
11

Why do we need a version bump here?

src/filemetadatautil.cpp
68

no longer needed here, rangeEnd is set in the loop.

This revision now requires changes to proceed.May 3 2019, 12:24 AM
astippich updated this revision to Diff 57437.May 3 2019, 5:32 AM
  • move rangeEnd into the loop
astippich marked 2 inline comments as done.May 3 2019, 5:33 AM
astippich added inline comments.
CMakeLists.txt
11

D19445 is required for proper display of lists

bruns accepted this revision.May 3 2019, 12:17 PM
This revision is now accepted and ready to land.May 3 2019, 12:17 PM
This revision was automatically updated to reflect the committed changes.
astippich marked an inline comment as done.