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.
Details
- Reviewers
bruns - Commits
- R824:d04e9281d7ad: Handle multiple entries in map
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
src/propertymaputil_p.h | ||
---|---|---|
47 | This now has O(n^2) complexity. Please use a nested iterator approach. |
src/propertymaputil_p.h | ||
---|---|---|
47 | Something like this? If not, please provide some guidance |
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; } } |
src/propertymaputil_p.h | ||
---|---|---|
43 | There is obviously an error here - you wan't to break on ==. |
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.
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; }); ... |
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." |
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)?
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; }
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.
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.
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.