-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[nemo-qml-plugin-email] Model and storage for autofill emaillist JB#4… #1
base: master
Are you sure you want to change the base?
Conversation
MR has moved here from https://git.sailfishos.org/mer-core/nemo-qml-plugin-email/merge_requests/71 |
Doesn't compile
etc. Should be static. |
src/emailcontactstorage.cpp
Outdated
} | ||
|
||
|
||
QString EmailContactStorage::name(const int index) const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor detail, but const on by-value parameters is a bit unnecessary noise. Shouldn't matter to the caller if the method internally adjusts its own copy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it when the parameters are constant.
- It is symmetrical with constant references.
2.very rare, but helps inside the function
src/emailcontactstorage.cpp
Outdated
return; | ||
|
||
QMailMessageKey incomingKey(QMailMessageKey::status(QMailMessage::Incoming)); | ||
QMailMessageIdList incomingIds = mailStore->queryMessages(incomingKey); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be a limit here? E.g. 100 newest or something, supposedly needing sorting too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are supposed to search across all emails.
There may be rare combinations of initial letters. And none of these combinations will get into the first 100 or 1000.
IMHO, Loading data once is preferable to loading it every time.
but, there were no optimization tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are supposed to search across all emails.
Why? Some mail boxes can be huge, why should some rarely used contact matter? This is anyway just for suggestions, it doesn't need to go look for anyone the user might know of.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only the user knows who he is looking for (we still do not know how to get into his head)
It is a mechanism for helping search.
During the work of which it is very often necessary to view the all mailbox.
Each new character entered decreases the set of found.
if we want to see always five results (as an example).
We'll get to the end of the box
Example. User types a name - "Dmitriy". He wants to find email [email protected]
Press - D.
Result:
Djen
David
[email protected]
And many others
After Dmi
Dmitriy
[email protected]
DmitriyAK
[email protected]
dmiranom
He is unsure and adds another character. The number of matches has become less than five.
And if there were less than five of them, then we had to sort out the whole emailbox.
We are not showing the last five addresses.
And the last five addresses by the template. Quite often the set will be less than five. And this means that we have to sort through the item box completely.
*five is just an example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this done synchronously, in the main thread? In precisely which cases will this EmailContactStorage instance be initialized (which apps is it used in? and is the instantiation and initialization deferred until the user creates the first UI component which includes a recipient entry field, or is it done on email app startup, etc?) Have any benchmarks been done to determine what the effect on email app startup time is, if the user has a synced exchange inbox with 5k emails, for example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There were no performance tests. This is bad.
Used in еmail on version Aurora 3.4.0
is planned to use it in the calendar on version Aurora 4.0.1
Initialized now on the main thread. No optimization performed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's just add some limitation here, shall we? Sort by the newest ones, 100 emails might be short, but just picking a number, 200 could already provide enough relevant contacts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have limited the number of letters for the list
src/emailcontactstorage.cpp
Outdated
} | ||
|
||
|
||
QString EmailContactStorage::email(const int index) const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and above don't seem to be called from anywhere? Could remove?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/emailcontactstorage.cpp
Outdated
|
||
EmailContact EmailContactStorage::at(const int index) const | ||
{ | ||
return m_storage[index]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this takes external requests, could protect the indexing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super minor style nitpick, obviously feel free to ignore this: I prefer having only a single return statement for cases like this, e.g. return (index < 0 || index >= count()) ? EmailContact() : m_storage[index];
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it
done
src/emailcontactstorage.cpp
Outdated
} | ||
|
||
|
||
void EmailContactStorage::initialize() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing updates the contacts after start?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The add() function adds contacts.
For one session, only list adding.
Deleting a contact should not remove it from the list (there may be an erroneous deletion)
Renaming should keep the previous contact and add a new one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean you start the app, write emails to 5 people, start writing a new one -> should include the suggestions from the earlier 5, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right.
the names and emails of these five should be added.
src/emailcontactlistmodel.cpp
Outdated
|
||
QVariant EmailContactListModel::data(const QModelIndex &index, int role) const | ||
{ | ||
EmailContactStorage *storage = EmailContactStorage::instance(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The contact storage seems to be used only here and couple other places in this file. As this is kind of thin wrapper, pondering if it warrants to keep these separate or should those be just combined into one class. If the contact store is a heavy object it could be one reason, but now it seems unnecessarily heavy by going through all(?) the messages. And there's nothing refreshing it. If the stuff was instantiated by need, it would be also up-to-date by free.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all right.
This is a thin wrapper. I dont know which is best. Now you can merge. If the storage class grows, it will increase the difficulty.
src/emailcontactlistmodel.cpp
Outdated
|
||
QString name = variant.toString(); | ||
if (!name.isEmpty()) | ||
storage->removeByName(name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't either play well with a common storage of the contacts. If it's allowed to remove duplicates (or why should there be duplicates ever?) those should be removed only from a single instance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly a bad name.
Duplicates from another model that we don't know anything about in this class.
src/emailcontactstorage.cpp
Outdated
} | ||
|
||
// email can have duplicate. We must erase all email duplicates | ||
void EmailContactStorage::removeByEmail(const QString &email) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing seems to call this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And consequently nothing needs findByEmail()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes you are right. Done
src/emailcontactstorage.cpp
Outdated
|
||
void EmailContactStorage::add(const EmailContact &contact) | ||
{ | ||
auto iter = findByName(contact.name()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now every insertion loops everything already inserted? Uh.
And why is it not allowing mutiple contacts by the same name? They could be even different persons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we often correspond with the same people. We will find matches very often. We need to replace the stamp with the latest one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the name here is the name of the EmailContact (it is not name of Contact).
This is what the user is finding (maybe a name or an address, what the user is finding we don't know). But the EmaiContact name is unique.
src/plugin/plugin.cpp
Outdated
@@ -83,6 +85,8 @@ class Q_DECL_EXPORT NemoEmailPlugin : public QQmlExtensionPlugin | |||
qmlRegisterType<AttachmentListModel>(uri, 0, 1, "AttachmentListModel"); | |||
qmlRegisterUncreatableType<FolderAccessor>(uri, 0, 1, "FolderAccessor", | |||
"FolderAccessor is created via FolderListModel or similar"); | |||
qmlRegisterType<EmailContactListModel>(uri, 0, 1, "EmailContactListModel"); | |||
qmlRegisterType<EmailContactListProxyModel>(uri, 0, 1, "EmailContactListProxyModel"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented already earlier on the app side, but this proxy model thing is a bit strange as the proxied source model is set internally. For the app developer the proxy part is almost invisible. Only funkier detail could be that the proxy model is a property in the qt class so that exposes the internals outside.
Then the EmailContactListModel itself is not at the moment used at all on the app side? Does it need to be registered and exposed? Is there need to even have a separate proxy and source or could it all be embedded into a single contact model?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The separation into model and proxy model is needed.
They have different responsibilities and tasks.
Now there is no need for a model outside, fixed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are maybe two things here. One is that a model + proxy might be easier to implement in c++, the other is what gets exposed to QML. Now the QML only sees something that's called a proxy model but does not have visibility to what is the proxy target.
The name having other problems too. "List" is maybe redundant with "model", aren't all models more or less lists? And if the point is to have recent contacts, not necessarily all the contact, the name could indicate that too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
proxy are easier to implement in C++ side.
In QML side model name are EmailContactModel (remove suffix List)
done |
Description.
d.kotin - [email protected] there is an overuse of data in the list.
|
src/emailcontactlistmodel.cpp
Outdated
QHash<int, QByteArray> EmailContactListModel::roleNames() const | ||
{ | ||
static QHash<int, QByteArray> names = { | ||
{NameRole, "name"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style nitpick: prefer single space around curly braces, e.g. { NameRole, "name" },
etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/emailcontactlistmodel.cpp
Outdated
QModelIndex index = model->index(i, 0); | ||
QVariant variant = model->data(index, role); | ||
|
||
QString name = variant.toString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor nitpicks: index, variant, and name could probably all be const
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good.
like when the maximum constants.
done
src/emailcontactstorage.cpp
Outdated
} | ||
|
||
// name can't have duplicate | ||
void EmailContactStorage::remove(const QString &name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You've probably already answered this elsewhere, if so I apologise: what is the name here? Is it some combination of the display name plus email address? (I assume it must be, otherwise two contacts with the same name but different email addresses would not be able to be represented in the storage, e.g. "[email protected]" / "[email protected]")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, this is the name (first part of the contact)
remove additional function. Maybe with a bad name
It is needed to work with other models. To remove what other models showed.
The phonbooks model is like that.
It is necessary that this model does not have contacts of another model. This function is just for this
src/emailcontactstorage.h
Outdated
@@ -0,0 +1,39 @@ | |||
/* | |||
* Copyright (c) 2020 Open Mobile Platform LLC. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this should be 2020 - 2021
now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/emailcontact.h
Outdated
#include <QMailTimeStamp> | ||
|
||
|
||
class Q_DECL_EXPORT EmailContact |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not necessary, but could potentially make this a Q_GADGET if we ever think it might be useful to expose the type directly to QML (rather than field-wise via the model roles). But, of course, we can do that when needed, rather than doing so speculatively now, so, never mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the moment, in QML this class is not used directly, only as part the model. Now you don't need to change it to Q_GADGET. Maybe later
c02e603
to
746ff68
Compare
src/plugin/plugin.cpp
Outdated
@@ -83,6 +85,8 @@ class Q_DECL_EXPORT NemoEmailPlugin : public QQmlExtensionPlugin | |||
qmlRegisterType<AttachmentListModel>(uri, 0, 1, "AttachmentListModel"); | |||
qmlRegisterUncreatableType<FolderAccessor>(uri, 0, 1, "FolderAccessor", | |||
"FolderAccessor is created via FolderListModel or similar"); | |||
qmlRegisterType<EmailContactListModel>(uri, 0, 1, "EmailContactListModel"); | |||
qmlRegisterType<EmailContactListProxyModel>(uri, 0, 1, "EmailContactListProxyModel"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are maybe two things here. One is that a model + proxy might be easier to implement in c++, the other is what gets exposed to QML. Now the QML only sees something that's called a proxy model but does not have visibility to what is the proxy target.
The name having other problems too. "List" is maybe redundant with "model", aren't all models more or less lists? And if the point is to have recent contacts, not necessarily all the contact, the name could indicate that too?
src/emailcontactlistproxymodel.h
Outdated
|
||
signals: | ||
void filterChanged(const QString&); | ||
void countChanged(int); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the signals are mostly for qml, it can be better just omit the parameters as qml doesn't use them that much, instead just reads the property value again on changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix naming
src/emailcontactlistproxymodel.h
Outdated
virtual int count() const; | ||
QString filter() const; | ||
|
||
Q_INVOKABLE void removeDuplicates(QAbstractItemModel *model, const QString &role); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd get rid of this. The model should take care itself, not requiring someone else to ask for cleanup.
src/emailcontactlistproxymodel.h
Outdated
|
||
QHash<int, QByteArray> roleNames() const override; | ||
bool filterAcceptsRow(int source_row, const QModelIndex &source_parent = QModelIndex()) const override; | ||
virtual int count() const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this isn't meant for inheritance, i'd skip the virtual.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/emailcontactlistproxymodel.h
Outdated
Q_INVOKABLE void removeDuplicates(QAbstractItemModel *model, const QString &role); | ||
|
||
public slots: | ||
void setFilter(const QString &filter); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why in public slots?
src/emailcontactstorage.h
Outdated
void remove(const QString &name); | ||
|
||
private: | ||
using Container = QVector<EmailContact>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think this indirection provides too much.
src/emailcontactstorage.cpp
Outdated
} else { | ||
if (iter->timeStamp() < contact.timeStamp()) | ||
iter->setTimeStamp(contact.timeStamp()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could see all this becoming cleaner and more efficient with QHash or QSet.
src/emailcontactstorage.cpp
Outdated
if (storage != nullptr && storage->m_isInit == false) | ||
storage->initialize(); | ||
return storage; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps said already, but would just move all the content here to the EmailContactListModel which shouldn't need global data.
Actually looks like the models could get broken since they don't follow up any changes on the storage, yet this instance fetching refreshes the data.
src/emailcontactlistmodel.h
Outdated
#include <QAbstractListModel> | ||
|
||
|
||
class Q_DECL_EXPORT EmailContactListModel : public QAbstractListModel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't need Q_DECL_EXPORT since it's internal? Same for the contact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/emailcontact.h
Outdated
|
||
void setName(const QString &name); | ||
void setEmail(const QString &email); | ||
void setAddress(const QMailAddress &address); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of this group only the below timestamp seems to be used. -> could remove everything else. Could be even an option to remove the timestamp and just do new instances per need.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed it
4e5729e
to
f65a2a6
Compare
f65a2a6
to
d51d1c8
Compare
89ac114
to
f39e70b
Compare
Made a number of changes
|
src/src.pro
Outdated
@@ -28,6 +30,8 @@ PUBLIC_HEADERS += \ | |||
$$PWD/emailmessage.h \ | |||
$$PWD/emailaccountsettingsmodel.h \ | |||
$$PWD/emailaccount.h \ | |||
$$PWD/emailcontactmodel.h \ | |||
$$PWD/emailcontactproxymodel.h \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you really need these public, that is provided to external c++ projects?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/src.pro
Outdated
@@ -43,7 +47,7 @@ PRIVATE_HEADERS += \ | |||
|
|||
HEADERS += \ | |||
$$PUBLIC_HEADERS \ | |||
$$PRIVATE_HEADERS | |||
$$PRIVATE_HEADERS \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sometimes nice to have trailing "" in case more lines are added but here I' not expecting more header categories.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking a lot better now than earlier! Some small comments, need to actually get testing this next.
src/emailcontactmodel.cpp
Outdated
for (QHash<QMailAddress, QMailTimeStamp>::const_iterator i = container.begin(); i != container.end(); ++i) { | ||
m_container.push_back(qMakePair(i.value(), i.key())); | ||
} | ||
qSort(m_container.begin(), m_container.end(), comparer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
qSort is obsoleted in favor of std::sort. Could already use that instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea
std::somethinks I like more
done
src/emailcontactmodel.cpp
Outdated
} | ||
|
||
|
||
static void push(QHash<QMailAddress, QMailTimeStamp> &container, const QMailAddress &address, const QMailTimeStamp &stamp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small thing but commonly nicer to use pointers instead of non-const references. Even if more obvious with this method, you don't commonly see from foo(bar) that it can have side-effects on the passed parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/emailcontactproxymodel.h
Outdated
virtual int count() const; | ||
QString filter() const; | ||
|
||
Q_INVOKABLE void setMaxCount(const int maxCount); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If no particular need for this, I'd just skip. Can always add later if it seems necessary. Would be anyhow more of a property thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
replaced on property
src/emailcontactproxymodel.cpp
Outdated
{ | ||
if (m_filter != filter) { | ||
m_filter = filter; | ||
invalidateFilter(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be called after the real filter, the regexp, has been updated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes you are right.
done
src/emailcontactproxymodel.h
Outdated
|
||
Q_INVOKABLE void setMaxCount(const int maxCount); | ||
|
||
public slots: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to declare slot for filter if it's already a property. Better if the qml doesn't get a chance to call both filter = "foo" and setFilter("foo")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/emailcontactmodel.cpp
Outdated
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nitpicking, but commonly the separation is just one line :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/emailcontactmodel.cpp
Outdated
return; | ||
|
||
QMailMessageKey incomingKey(QMailMessageKey::status(QMailMessage::Incoming) & ~QMailMessageKey::status(QMailMessage::Trash)); | ||
QMailMessageIdList incomingIds = mailStore->queryMessages(incomingKey); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could use the limit parameter here and in the next query?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
b997332
to
db444e6
Compare
src/emailcontactmodel.cpp
Outdated
} | ||
|
||
QMailMessageKey outgoingKey(QMailMessageKey::status(QMailMessage::Outgoing)); | ||
QMailMessageIdList outgoingIds = mailStore->queryMessages(outgoingKey, QMailMessageSortKey(), maxCount); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the max count used on two queries the result might be up to 2*maxCount. That's fine if the max count is specified to be kind of per direction, but now that it's exposed as property on the proxy model, it's now behaving well. Easiest way to avoid that would be to get rid of the max count in the proxy model side. Though suppose it would be possible to fetch both incoming and outgoing in a single query.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted the maxCount parameter to be set externally.
This is the reason why he is in the proxy-model.
In requests I can do maxCount / 2
If I do this, then I am specifying the exact value, not its order.
There may be no incoming or outgoing email, If the value is accurate, this must be taken into account. That's why I did maxCount, not maxCount / 2
t doesn't really matter to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted the maxCount parameter to be set externally.
Is there some reason to? Haven't still seen anything actually using the property.
In requests I can do maxCount / 2
If I do this, then I am specifying the exact value, not its order.
At least it's then the maximum value. Might be lower but the "promise" is kept, so guess fine by such.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done maxCount / 2 now
src/emailcontactmodel.cpp
Outdated
QMailMessageKey incomingKey(QMailMessageKey::status(QMailMessage::Incoming) & ~QMailMessageKey::status(QMailMessage::Trash)); | ||
QMailMessageIdList incomingIds = mailStore->queryMessages(incomingKey, QMailMessageSortKey(), maxCount / 2); | ||
|
||
for (const auto& id: incomingIds) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: bunch of trailing space here.
src/emailcontactproxymodel.h
Outdated
|
||
QHash<int, QByteArray> roleNames() const override; | ||
bool filterAcceptsRow(int source_row, const QModelIndex &source_parent = QModelIndex()) const override; | ||
virtual int count() const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For minor detail wonder why this is virtual when the other getters aren't.
For bigger detail the countChanged() signal seems to be emitted explicitly from setFilter() and setMaxCount() which doesn't feel like the proper thing to do. Is this property even needed? Simplest to just drop.
src/emailcontactproxymodel.h
Outdated
QString filter() const; | ||
int maxCount() const; | ||
|
||
public: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why a second public:?
src/emailcontactproxymodel.h
Outdated
signals: | ||
void countChanged(int); | ||
void filterChanged(const QString&); | ||
void maxCountChanged(int); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor thing, but with the model being private, i.e. only for qml, the signal parameters don't provide much.
int rowCount(const QModelIndex &parent = QModelIndex()) const override; | ||
QVariant data(const QModelIndex &index, int role = Qt::DisplayRole) const override; | ||
|
||
Q_INVOKABLE void initialize(const int maxCount); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Such a thing still shouldn't exist on the api and not spotting any usage either.
QHash<int, QByteArray> roles; | ||
QList<QPair<QMailTimeStamp, QMailAddress>> m_container; | ||
|
||
const int portionMultiplier = 5; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems used only in the .cpp so shouldn't be included in the header.
Title is "Model and storage for autofill emaillist" but there's only the model now? |
@@ -83,6 +84,7 @@ class Q_DECL_EXPORT NemoEmailPlugin : public QQmlExtensionPlugin | |||
qmlRegisterType<AttachmentListModel>(uri, 0, 1, "AttachmentListModel"); | |||
qmlRegisterUncreatableType<FolderAccessor>(uri, 0, 1, "FolderAccessor", | |||
"FolderAccessor is created via FolderListModel or similar"); | |||
qmlRegisterType<EmailContactModel>(uri, 0, 1, "EmailContactModel"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is not actually a generic email contact model but rather a model for recent email contacts, the name could indicate that. RecentEmailContactModel or something like that?
…7000