[Merge] lp:~tiagosh/history-service/cached-conversations into lp:history-service
Gustavo Pichorim Boiko
gustavo.boiko at canonical.com
Wed Oct 7 21:51:38 UTC 2015
Review: Needs Fixing
Please see the inline diff comments.
Diff comments:
>
> === modified file 'Ubuntu/History/historygroupedthreadsmodel.cpp'
> --- Ubuntu/History/historygroupedthreadsmodel.cpp 2015-05-18 17:51:46 +0000
> +++ Ubuntu/History/historygroupedthreadsmodel.cpp 2015-10-07 21:06:01 +0000
> @@ -214,15 +235,21 @@
>
> void HistoryGroupedThreadsModel::processThreadGrouping(const History::Thread &thread)
> {
> - QVariantMap properties = thread.properties();
> - int pos = existingPositionForEntry(properties[mGroupingProperty]);
> + QVariantMap queryProperties;
> + queryProperties["groupingProperty"] = mGroupingProperty;
Can you declare a History::FieldGroupingProperty instead of using the string directly?
> + History::Thread groupedThread = History::Manager::instance()->getSingleThread((History::EventType)mType, thread.accountId(), thread.threadId(), queryProperties);
> + if (groupedThread.properties().isEmpty()) {
> + removeThreadFromGroup(thread);
> + return;
> + }
> + int pos = existingPositionForEntry(groupedThread);
>
> // if the group is empty, we need to insert it into the map
> if (pos < 0) {
> HistoryThreadGroup group;
> - int newPos = positionForItem(thread.properties());
> - group.threads.append(thread);
> - group.displayedThread = thread;
> + int newPos = positionForItem(groupedThread.properties());
> + group.threads = groupedThread.groupedThreads();
> + group.displayedThread = groupedThread;
> beginInsertRows(QModelIndex(), newPos, newPos);
> mGroups.insert(newPos, group);
> endInsertRows();
>
> === modified file 'historyprivate/utils.cpp'
> --- historyprivate/utils.cpp 2015-10-07 21:06:01 +0000
> +++ historyprivate/utils.cpp 2015-10-07 21:06:01 +0000
> @@ -31,12 +31,17 @@
> {
> }
>
> +bool Utils::shouldGroupAccount(const QString &accountId)
Can you add a FIXME here saying we need a better way to determine which accounts should be grouped?
> +{
> + return (matchFlagsForAccount(accountId) & MatchPhoneNumber);
> +}
>
> MatchFlags Utils::matchFlagsForAccount(const QString &accountId)
> {
> static QMap<QString, History::MatchFlags> protocolFlags;
> if (protocolFlags.isEmpty()) {
> protocolFlags["ofono"] = MatchPhoneNumber;
> + protocolFlags["multimedia"] = MatchPhoneNumber;
> }
>
> QString protocol = protocolFromAccountId(accountId);
>
> === modified file 'plugins/sqlite/sqlitehistoryplugin.cpp'
> --- plugins/sqlite/sqlitehistoryplugin.cpp 2015-10-07 21:06:01 +0000
> +++ plugins/sqlite/sqlitehistoryplugin.cpp 2015-10-07 21:06:01 +0000
> @@ -185,9 +329,35 @@
> return results;
> }
>
> -QVariantMap SQLiteHistoryPlugin::getSingleThread(History::EventType type, const QString &accountId, const QString &threadId)
> +QVariantMap SQLiteHistoryPlugin::getSingleThread(History::EventType type, const QString &accountId, const QString &threadId, const QVariantMap &properties)
> {
> QVariantMap result;
> + bool grouped = false;
> + if (accountId.isEmpty() || threadId.isEmpty()) {
> + return result;
> + }
> + if (properties.contains("groupingProperty")) {
> + grouped = properties["groupingProperty"].toString() == History::FieldParticipants;
> + }
> + if (grouped) {
> + const QString &threadKey = accountId+threadId;
I think you should use generateThreadMapKey() or have a equivalent function.
> + // we have to find which conversation this thread belongs to
> + if (mConversationsCacheKeys.contains(threadKey)) {
> + // found the thread.
> + // get the displayed thread now
> + const History::Threads &groupedThreads = mConversationsCache[mConversationsCacheKeys[threadKey]];
> + QVariantList finalGroupedThreads;
> + Q_FOREACH(const History::Thread &displayedThread, groupedThreads) {
> + finalGroupedThreads << displayedThread.properties();
> + if (generateThreadMapKey(displayedThread) == threadKey) {
> + result = displayedThread.properties();
> + }
> + }
> + result[History::FieldGroupedThreads] = QVariant::fromValue(finalGroupedThreads);
> + return result;
> + }
> + return result;
> + }
>
> QString condition = QString("accountId=\"%1\" AND threadId=\"%2\"").arg(accountId, threadId);
> QString queryText = sqlQueryForThreads(type, condition, QString::null);
> @@ -524,16 +715,39 @@
> return queryText;
> }
>
> -QList<QVariantMap> SQLiteHistoryPlugin::parseThreadResults(History::EventType type, QSqlQuery &query)
> +QList<QVariantMap> SQLiteHistoryPlugin::parseThreadResults(History::EventType type, QSqlQuery &query, const QVariantMap &properties)
> {
> QList<QVariantMap> threads;
> QSqlQuery attachmentsQuery(SQLiteDatabase::instance()->database());
> QList<QVariantMap> attachments;
> + bool grouped = false;
> + if (properties.contains("groupingProperty")) {
> + grouped = properties["groupingProperty"].toBool();
I think this needs to be:
grouped = properties["groupingProperty"].toString() == History::FieldParticipants;
> + }
> while (query.next()) {
> QVariantMap thread;
> + QString accountId = query.value(0).toString();
> + QString threadId = query.value(1).toString();
> + if (threadId.trimmed().isEmpty()) {
> + continue;
> + }
> thread[History::FieldType] = (int) type;
> - thread[History::FieldAccountId] = query.value(0);
> - thread[History::FieldThreadId] = query.value(1);
> + thread[History::FieldAccountId] = accountId;
> + thread[History::FieldThreadId] = threadId;
> + if (grouped) {
> + const QString &threadKey = accountId+threadId;
I think you should use generateThreadMapKey() or have a equivalent function.
> + if (mInitialised && type == History::EventTypeText &&
> + !mConversationsCache.contains(threadKey)) {
> + continue;
> + }
> + QVariantList groupedThreads;
> + if (mConversationsCache.contains(threadKey)) {
> + Q_FOREACH (const History::Thread &thread, mConversationsCache[threadKey]) {
> + groupedThreads << thread.properties();
> + }
> + }
> + thread[History::FieldGroupedThreads] = QVariant::fromValue(groupedThreads);
> + }
>
> thread[History::FieldEventId] = query.value(2);
> thread[History::FieldCount] = query.value(3);
>
> === modified file 'src/thread.cpp'
> --- src/thread.cpp 2014-09-09 22:56:16 +0000
> +++ src/thread.cpp 2015-10-07 21:06:01 +0000
> @@ -60,9 +65,12 @@
> const QStringList &participants,
> const Event &lastEvent,
> int count,
> - int unreadCount)
> -: d_ptr(new ThreadPrivate(accountId, threadId, type, participants, lastEvent, count, unreadCount))
> + int unreadCount,
> + const QList<Thread> &groupedThreads)
Can you change QList<Thread> by Threads?
> +: d_ptr(new ThreadPrivate(accountId, threadId, type, participants, lastEvent, count, unreadCount, groupedThreads))
> {
> + qDBusRegisterMetaType<QList<QVariantMap> >();
> + qRegisterMetaType<QList<QVariantMap> >();
> }
>
> Thread::Thread(const Thread &other)
> @@ -185,6 +214,19 @@
> int count = properties[FieldCount].toInt();
> int unreadCount = properties[FieldUnreadCount].toInt();
>
> + QList<Thread> groupedThreads;
Same here, can you change QList<Thread> to Threads?
> + if (properties.contains(FieldGroupedThreads)) {
> + QVariant variant = properties[FieldGroupedThreads];
> + if (variant.canConvert<QVariantList>()) {
> + Q_FOREACH(const QVariant& entry, variant.toList()) {
> + groupedThreads << Thread::fromProperties(entry.toMap());
> + }
> + } else if (variant.canConvert<QDBusArgument>()) {
> + QDBusArgument argument = variant.value<QDBusArgument>();
> + argument >> groupedThreads;
> + }
> + }
> +
> Event event;
> switch (type) {
> case EventTypeText:
> @@ -194,7 +236,24 @@
> event = VoiceEvent::fromProperties(properties);
> break;
> }
> - return Thread(accountId, threadId, type, participants, event, count, unreadCount);
> + return Thread(accountId, threadId, type, participants, event, count, unreadCount, groupedThreads);
> +}
> +
> +const QDBusArgument &operator>>(const QDBusArgument &argument, QList<Thread> &threads)
Same here, can you change QList<Thread> to Threads?
> +{
> + argument.beginArray();
> + while (!argument.atEnd()) {
> + QVariantMap props;
> + QVariant variant;
> + argument >> variant;
> + QDBusArgument innerArgument = variant.value<QDBusArgument>();
> + if (!innerArgument.atEnd()) {
> + innerArgument >> props;
> + }
> + threads << Thread::fromProperties(props);
> + }
> + argument.endArray();
> + return argument;
> }
>
> }
>
> === modified file 'src/thread.h'
> --- src/thread.h 2013-12-03 20:04:18 +0000
> +++ src/thread.h 2015-10-07 21:06:01 +0000
> @@ -60,6 +62,7 @@
> Event lastEvent() const;
> int count() const;
> int unreadCount() const;
> + QList<Thread> groupedThreads() const;
Same here, can you change QList<Thread> to Threads?
>
> bool isNull() const;
> bool operator==(const Thread &other) const;
> @@ -75,6 +78,8 @@
>
> typedef QList<Thread> Threads;
>
> +const QDBusArgument &operator>>(const QDBusArgument &argument, QList<Thread> &threads);
And here too.
> +
> }
>
> #endif // HISTORY_THREAD_H
--
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