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
Branch
multiple_conversion
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 11359
Build 11377: arc lint + arc unit
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
47

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
47

Something like this? If not, please provide some guidance

bruns added inline comments.Apr 28 2019, 5:59 PM
src/propertymaputil_p.h
47

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
43

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
46

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

53

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–30

"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 ↗(On Diff #57414)

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.
rjvbb added a subscriber: rjvbb.Apr 21 2020, 9:23 AM

Hi,

I'm updating some of my packaging for a set-up that should meet the minimum requirements that were current when this commit was made (Qt 5.9 and KF5 5.60.0; baloo-widgets 19.08.3 requires Qt 5.8 and KF5 5.58.0). Evidently this change wasn't checked against those requirements; the constKeyValue* iterators were introduced in Qt 5.10 (this lack of testing is also apparent in filemetadatadatedisplaytest.cpp:57!)

I've tried to roll a Qt 5.9 implementation of the new algorithm, but apparently failed:

#if QT_VERSION < QT_VERSION_CHECK(5, 10, 0)
    auto begin = propMap.constBegin();
    auto beginKey = propMap.keyBegin();

    while (begin != propMap.constEnd()) {
        auto key = begin.key();
        KFileMetaData::PropertyInfo property(key);
        auto rangeEnd = std::find_if(beginKey, propMap.keyEnd(),
            [key](const KFileMetaData::Property::Property& e) { return e != key; });

        auto distance = std::distance(beginKey, rangeEnd);
        if (distance > 1) {
            QVariantList list;
            list.reserve(static_cast<int>(distance));
            std::for_each(beginKey, rangeEnd, [&list,&propMap](const KFileMetaData::Property::Property& s) { list.append(propMap[s]); });
            map.insert(property.name(), list);
        } else {
            map.insert(property.name(), begin.value());
        }
        beginKey = rangeEnd;
        begin = propMap.find(*rangeEnd);
    }
#else
    auto begin = propMap.constKeyValueBegin();

    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; });

        auto distance = std::distance(begin, rangeEnd);
        if (distance > 1) {
            QVariantList list;
            list.reserve(static_cast<int>(distance));
            std::for_each(begin, rangeEnd, [&list](const entry& s) { list.append(s.second); });
            map.insert(property.name(), list);
        } else {
            map.insert(property.name(), (*begin).second);
        }
        begin = rangeEnd;
    }
#endif
FAIL!  : FileMetadataWidgetTest::shouldShowMultiValueProperties() Compared values are not the same
   Actual   (artistWidget->text())                 : "Artist1 and Artist1"
   Expected (QStringLiteral("Artist1 and Artist2")): "Artist1 and Artist2"
   Loc: [/opt/local/var/lnxports/build/_opt_local_site-ports_kf5_kf5-baloo-widgets/kf5-baloo-widgets/work/baloo-widgets-19.08.3/autotests/filemetadatawidgettest.cpp(205)]

Can I ask some feedback (from @astippich maybe)?

Hi,

I'm updating some of my packaging for a set-up that should meet the minimum requirements that were current when this commit was made (Qt 5.9 and KF5 5.60.0; baloo-widgets 19.08.3 requires Qt 5.8 and KF5 5.58.0). Evidently this change wasn't checked against those requirements; the constKeyValue* iterators were introduced in Qt 5.10 (this lack of testing is also apparent in filemetadatadatedisplaytest.cpp:57!)

I've tried to roll a Qt 5.9 implementation of the new algorithm, but apparently failed:

Can I ask some feedback (from @astippich maybe)?

Qt 5.10. is required since KF 5.55: https://kde.org/announcements/kde-frameworks-5.55.0.php.

That said, I think the issue is in this line

std::for_each(beginKey, rangeEnd, [&list,&propMap](const KFileMetaData::Property::Property& s) { list.append(propMap[s]); });

the []operator always returns the most recently inserted value for the same key according to docs. You must iterate over the values directly.

Qt 5.10. is required since KF 5.55: https://kde.org/announcements/kde-frameworks-5.55.0.php.

True, but not relevant here because the issue is not with KF5 APIs but with QMap. The frameworks relevant for baloo all build against Qt 5.9 either without or with minimal patching (which in essence reintroduce removed code).

> std::for_each(beginKey, rangeEnd, [&list,&propMap](const KFileMetaData::Property::Property& s) { list.append(propMap[s]); });

the []operator always returns the most recently inserted value for the same key according to docs. You must iterate over the values directly.

Thanks. I presume I still have to iterate over the keys in the range, so isn't what you suggest the same as doing list.append(propMap.values(s))? When I do that I get

FAIL!  : FileMetadataWidgetTest::shouldShowMultiValueProperties() Compared values are not the same
   Actual   (artistWidget->text())                 : "Artist1, Artist2, Artist1, and Artist2"
   Expected (QStringLiteral("Artist1 and Artist2")): "Artist1 and Artist2"

I don't think it would be trivial to create a local derivative of QMap that has the required methods backported from 5.10, and then cast to that derived class. But it seems I might be able to to create non-class versions of the constKeyValue* functions; I'll try that first.

Ultimately this wasn't difficult, so apologies for the noise!

For reference:

  • copy the QKeyValueIterator definition from qiterator.h (before the Baloo namespace)
  • modify the code as so:
#if QT_VERSION < QT_VERSION_CHECK(5, 10, 0)
    // extracted from the Qt 5.10 QMap class definition
    using const_key_value_iterator = QKeyValueIterator<const KFileMetaData::Property::Property&, const QVariant&, KFileMetaData::PropertyMap::const_iterator>;
    // based on QMap::constKeyValue*() from Qt 5.10
    #define constKeyValueBegin(map) const_key_value_iterator(map.begin())
    #define constKeyValueEnd(map)   const_key_value_iterator(map.end())
#else
    #define constKeyValueBegin(map) map.constKeyValueBegin()
    #define constKeyValueEnd(map)   map.constKeyValueEnd()
#endif
    auto begin = constKeyValueBegin(propMap);

    while (begin != constKeyValueEnd(propMap)) {
        auto key = (*begin).first;
        KFileMetaData::PropertyInfo property(key);
        auto rangeEnd = std::find_if(begin, constKeyValueEnd(propMap),
            [key](const entry& e) { return e.first != key; });

        auto distance = std::distance(begin, rangeEnd);
        if (distance > 1) {
            QVariantList list;
            list.reserve(static_cast<int>(distance));
            std::for_each(begin, rangeEnd, [&list](const entry& s) { list.append(s.second); });
            map.insert(property.name(), list);
        } else {
            map.insert(property.name(), (*begin).second);
        }
        begin = rangeEnd;
    }
Qt 5.10. is required since KF 5.55: https://kde.org/announcements/kde-frameworks-5.55.0.php.

True, but not relevant here because the issue is not with KF5 APIs but with QMap. The frameworks relevant for baloo all build against Qt 5.9 either without or with minimal patching (which in essence reintroduce removed code).

It is relevant. Qt 5.9 is not supported. If you insist on shoehorning it into 5.9, fine, but any bug reports for the Windows platform will be rejected immediately then.

rjvbb added a comment.Apr 21 2020, 1:46 PM

any bug reports for the Windows platform will be rejected immediately then.

Because they aren't already given that baloo only works on Linux and *BSD?

This is the sort of attitude that always makes my eyes roll whenever I see a reference to the idea of support in most KDE software.

bruns added a comment.Apr 21 2020, 2:30 PM

any bug reports for the Windows platform will be rejected immediately then.

Because they aren't already given that baloo only works on Linux and *BSD?

This is the sort of attitude that always makes my eyes roll whenever I see a reference to the idea of support in most KDE software.

baloo-widgets (which should be name kfilemetadata-widgets) also works on Windows.

Instead of adding Windows support where the platform *is* different, you introduce additional differences.

For most developers, Windows support causes extra work. Code has to be written, code has to be tested. Testing requires a Windows installation, which costs both money and time.

The last time I have changed kfilemetadata, I also added platform support for Windows (which required reading a lot of Windows API documentation) and FreeBSD (which has an API similar to Linux), and asked both teams for a review. FreeBSD reviewed the changes, but the Windows team stayed completely silent.

rjvbb added a comment.Apr 21 2020, 4:44 PM
Instead of adding Windows support where the platform *is* different, you introduce additional differences.

Either way, this has nothing to do with MS Windows, I don't do development there. I'm also not looking to have this committed, I'd have to be braindead to think that could ever happen to such an old previous version ...

I did have a similar experience when I tried to contribute a change to some kdewin project, years ago. No reaction.