[Merge] lp:~tiagosh/history-service/cached-conversations into lp:history-service
Gustavo Pichorim Boiko
gustavo.boiko at canonical.com
Mon Sep 21 17:24:26 UTC 2015
Review: Needs Fixing
I have added some comments in the diff itself, but the idea is good.
I just haven't seen any code (maybe I missed it) to add new threads to the cache (when a new thread is created), is that something we want to handle in the cache system?
Diff comments:
>
> === modified file 'Ubuntu/History/historythreadmodel.h'
> --- Ubuntu/History/historythreadmodel.h 2015-05-18 17:46:09 +0000
> +++ Ubuntu/History/historythreadmodel.h 2015-09-21 16:41:24 +0000
> @@ -34,12 +34,14 @@
> {
> Q_OBJECT
> Q_ENUMS(ThreadRole)
> + Q_PROPERTY(bool groupedThreads READ groupedThreads WRITE setGroupedThreads)
Could you just rename it to "groupThreads" instead? Just not to be confused with the "groupedThreads" role in the model.
>
> public:
>
> enum ThreadRole {
> CountRole = HistoryModel::LastRole,
> UnreadCountRole,
> + GroupedThreadsRole,
> LastEventIdRole,
> LastEventSenderIdRole,
> LastEventTimestampRole,
>
> === modified file 'daemon/HistoryService.xml'
> --- daemon/HistoryService.xml 2013-10-23 19:46:45 +0000
> +++ daemon/HistoryService.xml 2015-09-21 16:41:24 +0000
> @@ -60,6 +60,18 @@
> <annotation name="org.qtproject.QtDBus.QtTypeName.In1" value="QVariantMap"/>
> <annotation name="org.qtproject.QtDBus.QtTypeName.In2" value="QVariantMap"/>
> </method>
> + <method name="QueryGroupedThreads">
The DBus interface is not public nor meant to be used directly, so it is fine to just change the QueryThreads to have a string groupingProperty.
> + <dox:d><![CDATA[
> + Creates a threads view with the given filter and sort order.
> + Returns the object path to the created view.
> + ]]></dox:d>
> + <arg name="type" type="i" direction="in"/>
> + <arg name="sort" type="a{sv}" direction="in"/>
> + <arg name="filter" type="a{sv}" direction="in"/>
> + <arg type="s" direction="out"/>
> + <annotation name="org.qtproject.QtDBus.QtTypeName.In1" value="QVariantMap"/>
> + <annotation name="org.qtproject.QtDBus.QtTypeName.In2" value="QVariantMap"/>
> + </method>
> <method name="QueryEvents">
> <dox:d><![CDATA[
> Creates an events view with the given filter and sort order.
>
> === modified file 'daemon/historydaemon.cpp'
> --- daemon/historydaemon.cpp 2015-08-28 10:16:02 +0000
> +++ daemon/historydaemon.cpp 2015-09-21 16:41:24 +0000
> @@ -113,7 +113,7 @@
> return thread;
> }
>
> -QString HistoryDaemon::queryThreads(int type, const QVariantMap &sort, const QVariantMap &filter)
> +QString HistoryDaemon::queryThreads(int type, const QVariantMap &sort, const QVariantMap &filter, bool groupedThreads)
Maybe instead of the bool groupedThreads, we could have a QString groupingProperty? If the grouping property is null, no grouping is done, if it is the "participants" property, we use the cache, if it is a different property, we group manually (we might even postpone this third case implementation as the grouped threads model will handle the grouping)
> {
> if (!mBackend) {
> return QString::null;
>
> === modified file 'historyprivate/utils.cpp'
> --- historyprivate/utils.cpp 2015-04-09 18:53:44 +0000
> +++ historyprivate/utils.cpp 2015-09-21 16:41:24 +0000
> @@ -30,6 +30,11 @@
> {
> }
>
> +bool Utils::shouldGroupAccount(const QString &accountId)
maybe use the account match flags for that? if the account has MatchPhoneNumber, group it.
> +{
> + return (protocolFromAccountId(accountId) != "ofono" &&
> + protocolFromAccountId(accountId) != "multimedia");
> +}
>
> MatchFlags Utils::matchFlagsForAccount(const QString &accountId)
> {
> @@ -70,4 +75,63 @@
> return id1 == id2;
> }
>
> +bool Utils::compareParticipants(const QStringList &participants1, const QStringList &participants2, bool phoneComparison)
Can you use the match flags for account stuff for this? I know it will then depend on telepathy being ready, but as we are only doing this at startup, I think it makes sense.
> +{
> + // if list size is different, just return
> + if (participants1.count() != participants2.count()) {
> + return false;
> + }
> +
> + if (phoneComparison) {
> + QStringList normalizedParticipants1;
> + QStringList normalizedParticipants2;
> + Q_FOREACH(const QString &participant, participants1) {
> + normalizedParticipants1 << PhoneUtils::normalizePhoneNumber(participant);
> + }
> + Q_FOREACH(const QString &participant, participants2) {
> + normalizedParticipants2 << PhoneUtils::normalizePhoneNumber(participant);
> + }
> + return compareNormalizedParticipants(normalizedParticipants1, normalizedParticipants2, phoneComparison);
> +
> + }
> +
> + return compareNormalizedParticipants(participants1, participants2, phoneComparison);
> +}
> +
> +bool Utils::compareNormalizedParticipants(const QStringList &participants1, const QStringList &participants2, bool phoneComparison)
Same here, if possible replace the bool phoneComparison by an accountId, or by the matchflags directly.
> +{
> + QStringList mutableParticipants2 = participants2;
> + // if list size is different, just return
> + if (participants1.count() != participants2.count()) {
> + return false;
> + }
> +
> + // and now compare the lists
> + bool found = true;
> + Q_FOREACH(const QString &participant, participants1) {
> + if (phoneComparison) {
> + // we need to iterate the list and call the phone number comparing function for
> + // each participant from the given thread
> + bool inList = false;
> + QStringList::iterator it = mutableParticipants2.begin();
> + while (it != mutableParticipants2.end()) {
> + if (PhoneUtils::compareNormalizedPhoneNumbers(*it, participant)) {
> + inList = true;
> + mutableParticipants2.erase(it);
> + break;
> + }
> + ++it;
> + }
> + if (!inList) {
> + found = false;
> + break;
> + }
> + } else if (!mutableParticipants2.contains(participant)) {
> + found = false;
> + break;
> + }
> + }
> + return found;
> +}
> +
> }
>
> === modified file 'historyprivate/utils_p.h'
> --- historyprivate/utils_p.h 2015-04-01 20:05:39 +0000
> +++ historyprivate/utils_p.h 2015-09-21 16:41:24 +0000
> @@ -32,6 +32,9 @@
> static MatchFlags matchFlagsForAccount(const QString &accountId);
> static QString protocolFromAccountId(const QString &accountId);
> static bool compareIds(const QString &accountId, const QString &id1, const QString & id2);
> + static bool compareParticipants(const QStringList &participants1, const QStringList &participants2, bool phoneComparison = false);
> + static bool compareNormalizedParticipants(const QStringList &participants1, const QStringList &participants2, bool phoneComparison = false);
Can you remove the HistoryModel::compareParticipants() and change all code using it to use this new function?
> + static bool shouldGroupAccount(const QString &accountId);
>
> private:
> Utils();
>
> === modified file 'plugins/sqlite/sqlitehistoryplugin.cpp'
> --- plugins/sqlite/sqlitehistoryplugin.cpp 2015-09-21 16:41:24 +0000
> +++ plugins/sqlite/sqlitehistoryplugin.cpp 2015-09-21 16:41:24 +0000
> @@ -21,30 +21,131 @@
>
> #include "sqlitehistoryplugin.h"
> #include "phoneutils_p.h"
> +#include "utils_p.h"
> #include "sqlitedatabase.h"
> #include "sqlitehistoryeventview.h"
> #include "sqlitehistorythreadview.h"
> #include "intersectionfilter.h"
> #include "unionfilter.h"
> +#include "thread.h"
> #include <QDateTime>
> #include <QDebug>
> #include <QStringList>
> #include <QSqlError>
> #include <QDBusMetaType>
>
> +Q_DECLARE_METATYPE(History::Thread)
> +Q_DECLARE_METATYPE(History::Threads)
> +
> SQLiteHistoryPlugin::SQLiteHistoryPlugin(QObject *parent) :
> - QObject(parent)
> + QObject(parent), mInitialised(false)
> {
> + qRegisterMetaType<History::Thread>();
> + qRegisterMetaType<History::Threads>();
> // just trigger the database creation or update
> SQLiteDatabase::instance();
> +
> + // TODO query only for text threads
Can you move the block of code below to a separate function? updateGroupedThreadsCache() or something like that?
> + History::PluginThreadView *view = queryThreads(History::EventTypeText, History::Sort("timestamp", Qt::DescendingOrder), History::Filter());
> + QList<QVariantMap> threads;
> + while (view->IsValid()) {
> + QList<QVariantMap> page = view->NextPage();
> + if (page.size() > 0) {
> + threads += page;
> + } else {
> + break;
> + }
> + }
> + addThreadsToCache(threads);
> + mInitialised = true;
> +}
> +
> +void SQLiteHistoryPlugin::addThreadsToCache(const QList<QVariantMap> &threads)
> +{
> + Q_FOREACH (const QVariantMap &properties, threads) {
> + History::Thread thread = History::Thread::fromProperties(properties);
> + const QString &threadKey = thread.accountId() + thread.threadId();
> +
> + if (thread.type() != History::EventTypeText) {
> + continue;
> + } else if (!History::Utils::shouldGroupAccount(thread.accountId())) {
> + // never group non phone accounts
> + mConversationsCache[threadKey] = History::Threads() << thread;
> + continue;
> + }
> + bool found = false;
> + QMap<QString, History::Threads>::iterator it = mConversationsCache.begin();
> + while (it != mConversationsCache.end()) {
> + const QString &thisThreadKey = it.key();
> + History::Threads threads = it.value();
> + Q_FOREACH(const History::Thread &groupedThread, threads) {
> + found = History::Utils::compareNormalizedParticipants(thread.participants(), groupedThread.participants(), true);
> + if (found) {
> + qDebug() << "appending " << thread.accountId() << thread.threadId() << " to " << thisThreadKey;
> + mConversationsCache[thisThreadKey] += thread;
> + break;
> + }
> + }
> + it++;
> + if (found) {
> + break;
> + }
> + }
> + if (!found) {
> + qDebug() << "added key" << threadKey;
> + mConversationsCache[threadKey] = History::Threads() << thread;
> + }
> + }
> +}
> +
> +void SQLiteHistoryPlugin::removeThreadFromCache(const QVariantMap &properties)
> +{
> + History::Thread thread = History::Thread::fromProperties(properties);
> + QString threadKey = thread.accountId() + thread.threadId();
> +
> + if (thread.type() != History::EventTypeText || !History::Utils::shouldGroupAccount(thread.accountId())) {
> + mConversationsCache.remove(threadKey);
> + return;
> + }
> +
> + // check if this is a main key first
> + if (mConversationsCache.contains(threadKey)) {
> + // Remove itself from the list and promote the next grouped thread if any
> + History::Threads threads = mConversationsCache[threadKey];
> + threads.removeAll(thread);
> + mConversationsCache.remove(threadKey);
> + if (!threads.isEmpty()) {
> + threadKey = threads.first().accountId()+threads.first().threadId();
> + mConversationsCache[threadKey] = threads;
> + }
> + } else {
> + // check if it belongs to an existing grouped thread;
> + QMap<QString, History::Threads>::iterator it = mConversationsCache.begin();
> + while (it != mConversationsCache.end()) {
> + const QString &threadKey = it.key();
> + History::Threads threads = it.value();
> + History::Threads::iterator it2 = threads.begin();
> + while (it2 != threads.end()) {
> + if (History::Utils::compareNormalizedParticipants(thread.participants(), it2->participants(), true)) {
> + qDebug() << "removing " << it2->accountId() << it2->threadId() << " from " << threadKey;
> + threads.erase(it2);
> + mConversationsCache[threadKey] = threads;
> + return;
> + }
> + it2++;
> + }
> + it++;
> + }
> + }
> }
>
> // Reader
> History::PluginThreadView *SQLiteHistoryPlugin::queryThreads(History::EventType type,
> const History::Sort &sort,
> - const History::Filter &filter)
> + const History::Filter &filter,
> + bool grouped)
> {
> - return new SQLiteHistoryThreadView(this, type, sort, filter);
> + return new SQLiteHistoryThreadView(this, type, sort, filter, grouped);
> }
>
> History::PluginEventView *SQLiteHistoryPlugin::queryEvents(History::EventType type,
>
> === modified file 'src/manager.h'
> --- src/manager.h 2013-09-17 23:35:00 +0000
> +++ src/manager.h 2015-09-21 16:41:24 +0000
> @@ -47,7 +47,8 @@
>
> ThreadViewPtr queryThreads(EventType type,
> const Sort &sort = Sort(),
> - const Filter &filter = Filter());
> + const Filter &filter = Filter(),
> + bool grouped = false);
I think this breaks the ABI, so make sure you add the affected components in the silo.
As mentioned above, maybe it would be better to pass a QString groupingProperty to the method?
>
> EventViewPtr queryEvents(EventType type,
> const Sort &sort = Sort(),
--
https://code.launchpad.net/~tiagosh/history-service/cached-conversations/+merge/271536
Your team Ubuntu Phablet Team is subscribed to branch lp:history-service.
More information about the Ubuntu-reviews
mailing list