Add field "Folder" to theme content items and to message tool tip
ClosedPublic

Authored by gordin on Sun, May 10, 4:50 PM.

Details

Summary

Add possibility to add the messages account/folder as a theme content
item. Additionally, show the path in the messages tool tip.

FEATURE: 420644
FIXED-IN:

Test Plan

New tool tip:

New "Folder" item in content items

Message list:

Diff Detail

Repository
R94 PIM: Message Library
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
gordin created this revision.Sun, May 10, 4:50 PM
Restricted Application added a project: KDE PIM. · View Herald TranscriptSun, May 10, 4:50 PM
Restricted Application added a subscriber: kde-pim. · View Herald Transcript
gordin requested review of this revision.Sun, May 10, 4:50 PM
gordin abandoned this revision.Sun, May 10, 9:28 PM

I just noticed that my original goal, showing the "real" folder of an item inside a smart list or search, does not work like this. As the the "parentCollection" actually is the smart list/search, it will not show the original folder :-(

You could still do it but it's a bit more complex: you can use Item::storageCollectionId() to get ID of the real parent collection. You would then need to use the ID against EntityTreeModel to resolve the /actual/ Collection with displayName populated.

gordin updated this revision to Diff 82703.Tue, May 12, 11:43 PM
gordin edited the test plan for this revision. (Show Details)

Now using the actual (and not the virtual) collection to obtain the message folder

Maybe "StorageModel::collectionFromId()" rather than "StorageModel::collectionForId()" would be better?

dvratil requested changes to this revision.Wed, May 13, 7:19 AM

collectionForId is a good name, I think.

messagelist/src/storagemodel.cpp
263

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.

506

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

This revision now requires changes to proceed.Wed, May 13, 7:19 AM
gordin added inline comments.Wed, May 13, 12:21 PM
messagelist/src/storagemodel.cpp
263

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?

506

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

dvratil added inline comments.Wed, May 13, 12:49 PM
messagelist/src/storagemodel.cpp
262

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

263

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.

506

Interesting, but very well, then.

gordin updated this revision to Diff 82803.Wed, May 13, 11:19 PM

Added folder name hashing, removed trailing slash

gordin marked 7 inline comments as done.Wed, May 13, 11:20 PM
dvratil requested changes to this revision.Thu, May 14, 6:11 AM

Thanks for the changes. One more change/optimization, please.

messagelist/src/storagemodel.cpp
259

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.

This revision now requires changes to proceed.Thu, May 14, 6:11 AM
gordin updated this revision to Diff 82898.Fri, May 15, 1:24 AM
gordin edited the summary of this revision. (Show Details)

Optimised hash look-up

gordin marked an inline comment as done.Fri, May 15, 1:24 AM
gordin edited the test plan for this revision. (Show Details)Fri, May 15, 1:33 AM
dvratil requested changes to this revision.Fri, May 15, 12:23 PM

Tried it locally and it's pretty cool. Just some nitpicks in the recent changes.

messagelist/src/storagemodel.cpp
260

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

270

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.

This revision now requires changes to proceed.Fri, May 15, 12:23 PM
gordin added a comment.EditedFri, May 15, 2:54 PM

Sorry, something went wrong. I did a "arc diff" and despite "arc which" states the matching revision is D29602 it created a new revision D29779. Any advice how I should proceed?

Abandon the other one and try updating this one again. You can use arc diff --update D29602 to make sure it updates this one.

gordin updated this revision to Diff 82973.Fri, May 15, 7:11 PM
  • addressed comment from dvratil and also from mlaurent (made in accidentially added revision D29779)
gordin marked 2 inline comments as done.Fri, May 15, 7:12 PM
dvratil accepted this revision.Mon, May 18, 11:31 AM

Looks good to me know. Thanks for this patch, it's a pretty cool feature, especially for the "Unified mailboxes" agent 👍

This revision is now accepted and ready to land.Mon, May 18, 11:31 AM
This revision was automatically updated to reflect the committed changes.

@ngraham: this is a feature for the weekly report. What is the proposed way to ping you for things to blog about?