Differential D29602 Diff 82898 messagelist/src/storagemodel.cpp

Changeset View

Standalone View

View Options

messagelist/src/storagemodel.cpp

Show All 39 Lines
40#include <QUrl> 40​#include <QUrl>
41 41​
42#include <QAbstractItemModel> 42​#include <QAbstractItemModel>
43#include <QAtomicInt> 43​#include <QAtomicInt>
44#include <QItemSelectionModel> 44​#include <QItemSelectionModel>
45#include <QMimeData> 45​#include <QMimeData>
46#include <QCryptographicHash> 46​#include <QCryptographicHash>
47#include <QFontDatabase> 47​#include <QFontDatabase>
48​#include <QHash>
48 49​
49namespace MessageList { 50​namespace MessageList {
50class Q_DECL_HIDDEN StorageModel::Private 51​class Q_DECL_HIDDEN StorageModel::Private
51{ 52​{
52public: 53​public:
53 void onSourceDataChanged(const QModelIndex &topLeft, const QModelIndex &bottomRight); 54​ void onSourceDataChanged(const QModelIndex &topLeft, const QModelIndex &bottomRight);
54 void onSelectionChanged(); 55​ void onSelectionChanged();
55 void loadSettings(); 56​ void loadSettings();
56 57​
57 StorageModel *const q; 58​ StorageModel *const q;
58 59​
59 QAbstractItemModel *mModel = nullptr; 60​ QAbstractItemModel *mModel = nullptr;
60 QAbstractItemModel *mChildrenFilterModel = nullptr; 61​ QAbstractItemModel *mChildrenFilterModel = nullptr;
61 QItemSelectionModel *mSelectionModel = nullptr; 62​ QItemSelectionModel *mSelectionModel = nullptr;
62 63​
64​ QHash<Akonadi::Collection::Id, QString> mFolderHash;
65​
63 Private(StorageModel *owner) 66​ Private(StorageModel *owner)
64 : q(owner) 67​ : q(owner)
65 { 68​ {
66 } 69​ }
67}; 70​};
68} // namespace MessageList 71​} // namespace MessageList
69 72​
70using namespace Akonadi; 73​using namespace Akonadi;
▲ Show 20 Lines • Show All 177 Lines • ▼ Show 20 Line(s)217{
248 mi->setParentCollectionId(parentCol.id()); 251​ mi->setParentCollectionId(parentCol.id());
249 252​
250 QString subject = mail->subject()->asUnicodeString(); 253​ QString subject = mail->subject()->asUnicodeString();
251 if (subject.isEmpty()) { 254​ if (subject.isEmpty()) {
252 subject = QLatin1Char('(') + noSubject + QLatin1Char(')'); 255​ subject = QLatin1Char('(') + noSubject + QLatin1Char(')');
253 } 256​ }
254 257​
255 mi->setSubject(subject); 258​ mi->setSubject(subject);
256 259​
dvratilUnsubmitted
Done

This requires two lookups: first in the contains() and then in the value(). Use find() instead, which returns an iterator. When the iterator is invalid, assemble the string and insert it, otherwise get the value from the iterator.

dvratil: This requires two lookups: first in the `contains()` and then in the `value()`. Use `find()`…
260​ QHash<Akonadi::Collection::Id, QString>::iterator it =
dvratilUnsubmitted
Done

Use auto instead of typing out the entire iterator type, please.

dvratil: Use `auto` instead of typing out the entire iterator type, please.
261​ d->mFolderHash.find(item.storageCollectionId());
262​ if (it == d->mFolderHash.end()) {
dvratilUnsubmitted
Done

I would trim the trailing '/' from the folder path here, so that it looks like "Local Folders/Inbox" rather than "Local Folders/Inbox/"

dvratil: I would trim the trailing '/' from the folder path here, so that it looks like "Local…
263​ QString folder;
dvratilUnsubmitted
Done

This will use a ton of memory and will make populating the model slow - for each single email you will need to resolve the real parent collection, traverse the collection parent chain and assemble the string.

I suggest you add a QHash<Collection::Id, QString> lookup table into the StorageModel and try to look up the path by the collection ID there first, then fallback to building the path and inserting it into the hash map. This way we will make use of the implicit QString sharing so all items from the same collection will share the same single string with the path reducing memory footprint, and we will avoid assembling the string every time, which is more costly than a hash map lookup.

Remember to clear the hash map when the selected folder is changed.

dvratil: This will use a ton of memory and will make populating the model slow - for each single email…
gordinAuthorUnsubmitted
Done

What do you mean by "selected folder"? I thought the collection ID is globally unique? Why can it not be re-used independent from any selection?

gordin: What do you mean by "selected folder"? I thought the collection ID is globally unique? Why can…
dvratilUnsubmitted
Done

Collection ID is globally unique, this is about saving resources. Note that you are touching some very very performance and memory-sensitive parts of code here.

If you open a virtual search folder, each item can be from a different folder, so you will populate the hashtable with many mappings. However, when you then switch to a regular folder, you only need a single mapping, because all Items there belong to a single collection. Without clearing the hashtable on folder switch, you would be

  1. keeping unused mappings allocated and using memory
  2. forcing the lookup code to look up through all the irrelevant mappings just to find that one for the current folder - doing that for each Item

Since majority of usecases is browsing regular folders, where there will be only single mapping, you should optimize for that.

dvratil: Collection ID is globally unique, this is about saving resources. Note that you are touching…
264​ Collection collection = collectionForId(item.storageCollectionId());
265​ while (collection.parentCollection().isValid()) {
266​ folder = collection.displayName() + QLatin1String("/") + folder;
267​ collection = collection.parentCollection();
268​ }
269​ it = d->mFolderHash.insert(item.storageCollectionId(),
270​ folder.left(folder.size() - 1));
dvratilUnsubmitted
Done

This does an unnecessary copy of the substring, I'd propose to do folder.chop(1) after the while loop, which just shrinks the current string, then you can insert it directly into the hash table.

dvratil: This does an unnecessary copy of the substring, I'd propose to do `folder.chop(1)` after the…
271​ }
272​ mi->setFolder(it.value());
273​
257 updateMessageItemData(mi, row); 274​ updateMessageItemData(mi, row);
258 275​
259 return true; 276​ return true;
260} 277​}
261 278​
262static QByteArray md5Encode(const QByteArray &str) 279​static QByteArray md5Encode(const QByteArray &str)
263{ 280​{
264 auto trimmed = str.trimmed(); 281​ auto trimmed = str.trimmed();
▲ Show 20 Lines • Show All 157 Lines • ▼ Show 20 Line(s)
422void StorageModel::Private::onSourceDataChanged(const QModelIndex &topLeft, const QModelIndex &bottomRight) 439​void StorageModel::Private::onSourceDataChanged(const QModelIndex &topLeft, const QModelIndex &bottomRight)
423{ 440​{
424 Q_EMIT q->dataChanged(q->index(topLeft.row(), 0), 441​ Q_EMIT q->dataChanged(q->index(topLeft.row(), 0),
425 q->index(bottomRight.row(), 0)); 442​ q->index(bottomRight.row(), 0));
426} 443​}
427 444​
428void StorageModel::Private::onSelectionChanged() 445​void StorageModel::Private::onSelectionChanged()
429{ 446​{
447​ mFolderHash.clear();
430 Q_EMIT q->headerDataChanged(Qt::Horizontal, 0, q->columnCount() - 1); 448​ Q_EMIT q->headerDataChanged(Qt::Horizontal, 0, q->columnCount() - 1);
431} 449​}
432 450​
433void StorageModel::Private::loadSettings() 451​void StorageModel::Private::loadSettings()
434{ 452​{
435 // Custom/System colors 453​ // Custom/System colors
436 MessageListSettings *settings = MessageListSettings::self(); 454​ MessageListSettings *settings = MessageListSettings::self();
437 455​
▲ Show 20 Lines • Show All 42 Lines • ▼ Show 20 Line(s)490{
480 const QModelIndex etmIndex = childrenProxy->mapToSource(childrenFilterIndex); 498​ const QModelIndex etmIndex = childrenProxy->mapToSource(childrenFilterIndex);
481 Q_ASSERT(etmIndex.isValid()); 499​ Q_ASSERT(etmIndex.isValid());
482 // We cannot possibly refer to top-level collection 500​ // We cannot possibly refer to top-level collection
483 Q_ASSERT(etmIndex.parent().isValid()); 501​ Q_ASSERT(etmIndex.parent().isValid());
484 502​
485 const Collection col = etmIndex.parent().data(EntityTreeModel::CollectionRole).value<Collection>(); 503​ const Collection col = etmIndex.parent().data(EntityTreeModel::CollectionRole).value<Collection>();
486 Q_ASSERT(col.isValid()); 504​ Q_ASSERT(col.isValid());
487 505​
488 return col; 506​ return col;
dvratilUnsubmitted
Done

You can pass the d->mChildrenFilterModel here instead of the etm, the code inside is clever enough to find the ETM source model.

dvratil: You can pass the `d->mChildrenFilterModel` here instead of the `etm`, the code inside is clever…
gordinAuthorUnsubmitted
Done

Actually, this does not work. Neither passing "d->mChildrenFilterModel" nor "static_cast<QAbstractProxyModel *>(d->mChildrenFilterModel)" will produce a result, "idx" is invalid.

gordin: Actually, this does not work. Neither passing "d->mChildrenFilterModel" nor…
dvratilUnsubmitted
Done

Interesting, but very well, then.

dvratil: Interesting, but very well, then.
489} 507​}
490 508​
509​Akonadi::Collection StorageModel::collectionForId(Akonadi::Collection::Id colId) const
510​{
511​ // Get ETM
512​ QAbstractProxyModel *childrenProxy = static_cast<QAbstractProxyModel *>(d->mChildrenFilterModel);
513​ QAbstractItemModel* etm = childrenProxy->sourceModel();
514​
515​ // get index in EntityTreeModel
516​ const QModelIndex idx = EntityTreeModel::modelIndexForCollection(etm, Collection(colId));
517​ Q_ASSERT(idx.isValid());
518​
519​ // get and return collection
520​ return idx.data(EntityTreeModel::CollectionRole).value<Collection>();
521​}
522​
491void StorageModel::resetModelStorage() 523​void StorageModel::resetModelStorage()
492{ 524​{
493 beginResetModel(); 525​ beginResetModel();
494 endResetModel(); 526​ endResetModel();
495} 527​}
496 528​
497#include "moc_storagemodel.cpp" 529​#include "moc_storagemodel.cpp"