[Merge] lp:~phablet-team/telephony-service/add-participants-model into lp:telephony-service/staging
Gustavo Pichorim Boiko
gustavo.boiko at canonical.com
Tue Mar 21 17:51:43 UTC 2017
Review: Needs Fixing
Some things need to be addressed, but in general it looks good.
Diff comments:
>
> === added file 'Ubuntu/Telephony/participantsmodel.cpp'
> --- Ubuntu/Telephony/participantsmodel.cpp 1970-01-01 00:00:00 +0000
> +++ Ubuntu/Telephony/participantsmodel.cpp 2017-03-03 12:37:00 +0000
> @@ -0,0 +1,229 @@
> +/*
> + * Copyright (C) 2013-2017 Canonical, Ltd.
> + *
> + * Authors:
> + * Gustavo Pichorim Boiko <gustavo.boiko at canonical.com>
> + * Tiago Salem Herrmann <tiago.herrmann at canonical.com>
> + *
> + * This file is part of telephony-service.
> + *
> + * telephony-service is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 3.
> + *
> + * telephony-service is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "participantsmodel.h"
> +#include <participant.h>
> +#include <QDebug>
> +
> +Q_DECLARE_METATYPE(Participant)
> +
> +ParticipantsModel::ParticipantsModel(QObject *parent) :
> + QAbstractListModel(parent), mWaitingForQml(false), mCanFetchMore(true)
> +{
> + qRegisterMetaType<Participant>();
> + mRoles[AliasRole] = "alias";
> + mRoles[IdentifierRole] = "identifier";
> + mRoles[RolesRole] = "roles";
> + mRoles[StateRole] = "state";
> +
> + connect(this, SIGNAL(rowsInserted(QModelIndex,int,int)), this, SIGNAL(countChanged()));
> + connect(this, SIGNAL(rowsRemoved(QModelIndex,int,int)), this, SIGNAL(countChanged()));
> + connect(this, SIGNAL(modelReset()), this, SIGNAL(countChanged()));
> +}
> +
> +bool ParticipantsModel::canFetchMore(const QModelIndex &parent) const
> +{
> + return !mParticipantsCache.isEmpty();
> +}
> +
> +ParticipantsModel::~ParticipantsModel()
> +{
> +}
> +
> +void ParticipantsModel::fetchMore(const QModelIndex &parent)
> +{
> + if (parent.isValid() ) {
> + return;
> + }
> +
> + int max = 14;
> + while (max >= 0 && !mParticipantsCache.isEmpty()) {
> + addParticipant(mParticipantsCache.takeFirst());
> + max--;
> + }
> +
> + if (mParticipantsCache.isEmpty()) {
> + mCanFetchMore = false;
> + Q_EMIT canFetchMoreChanged();
> + }
> +}
> +
> +int ParticipantsModel::rowCount(const QModelIndex &parent) const
> +{
> + if (parent.isValid()) {
> + return 0;
> + }
> +
> + return mParticipants.count();
> +}
> +
> +QHash<int, QByteArray> ParticipantsModel::roleNames() const
> +{
> + return mRoles;
> +}
> +
> +void ParticipantsModel::addParticipantCache(Participant *participant)
> +{
> + int pos = positionForItem(participant->identifier(), true);
> + mParticipantsCache.insert(pos, participant);
> +}
> +
> +void ParticipantsModel::addParticipant(Participant *participant)
> +{
> + int pos = positionForItem(participant->identifier());
> + beginInsertRows(QModelIndex(), pos, pos);
> + mParticipants.insert(pos, participant);
> + endInsertRows();
> +}
> +
> +void ParticipantsModel::removeParticipant(Participant *participant)
> +{
> + int pos = mParticipants.indexOf(participant);
> + if (pos >= 0) {
> + beginRemoveRows(QModelIndex(), pos, pos);
> + mParticipants.removeAt(pos);
> + endRemoveRows();
> + }
> + pos = mParticipantsCache.indexOf(participant);
> + if (pos >= 0) {
> + mParticipantsCache.removeAt(pos);
> + }
> +}
> +
> +QVariant ParticipantsModel::data(const QModelIndex &index, int role) const
> +{
> + if (!index.isValid() || index.row() < 0 || index.row() >= rowCount()) {
> + return QVariant();
> + }
> + switch (role) {
> + case IdentifierRole:
> + return mParticipants[index.row()]->identifier();
> + break;
> + case AliasRole:
> + return mParticipants[index.row()]->alias();
> + break;
> + case StateRole:
> + return 0;
> + break;
> + case RolesRole:
> + return mParticipants[index.row()]->roles();
> + break;
> + }
> + return QVariant();
> +}
> +
> +bool ParticipantsModel::lessThan(const QString &left, const QString &right) const
> +{
Would you mind adding a comment here explaining the logic and the cases this function tries to solve? From what I understand it gets the names with symbols to the end of the list, but it would help having something explaining it.
> + if (left.isEmpty() || right.isEmpty()) {
> + return false;
> + }
> + if (left.at(0).isLetter() && right.at(0).isLetter()) {
> + return left.localeAwareCompare(right) < 0;
> + }
> + if (!left.at(0).isLetter() && right.at(0).isLetter()) {
> + return false;
> + }
> + if (left.at(0).isLetter() && !right.at(0).isLetter()) {
> + return true;
> + }
> +
> + return false;
> +}
> +
> +int ParticipantsModel::positionForItem(const QString &item, bool cache) const
> +{
> + // do a binary search for the item position on the list
> + int lowerBound = 0;
> + int upperBound = cache ? mParticipantsCache.count() - 1 : rowCount() - 1;
> + if (upperBound < 0) {
> + return 0;
> + }
> +
> + while (true) {
> + int pos = (upperBound + lowerBound) / 2;
> + const QString posItem = cache ? mParticipantsCache[pos]->identifier() : index(pos).data(IdentifierRole).toString();
> + if (lowerBound == pos) {
> + if (lessThan(item, posItem)) {
> + return pos;
> + }
> + }
> + if (lessThan(posItem, item)) {
> + lowerBound = pos + 1; // its in the upper
> + if (lowerBound > upperBound) {
> + return pos += 1;
> + }
> + } else if (lowerBound > upperBound) {
> + return pos;
> + } else {
> + upperBound = pos - 1; // its in the lower
> + }
> + }
> +}
> +
> +void ParticipantsModel::classBegin()
> +{
> + mWaitingForQml = true;
> +}
> +
> +void ParticipantsModel::componentComplete()
> +{
> + mWaitingForQml = false;
> +}
> +
> +QVariant ParticipantsModel::get(int row) const
> +{
> + QVariantMap data;
> + QModelIndex idx = index(row, 0);
> + if (idx.isValid()) {
> + QHash<int, QByteArray> roles = roleNames();
> + Q_FOREACH(int role, roles.keys()) {
> + data.insert(roles[role], idx.data(role));
> + }
> + }
> +
> + return data;
> +}
> +
> +ChatEntry* ParticipantsModel::chatEntry() const
> +{
> + return mChatEntry;
> +}
> +
> +void ParticipantsModel::setChatEntry(ChatEntry *entry)
> +{
> + if (mChatEntry == entry) {
> + return;
> + }
> + mChatEntry = entry;
In case mChatEntry is set to a valid value, you should disconnect the add and remove participants signal from the previous instance before setting the new one.
> + if (!entry) {
> + return;
> + }
> + connect(mChatEntry, SIGNAL(participantAdded(Participant *)), SLOT(addParticipant(Participant *)));
> + connect(mChatEntry, SIGNAL(participantRemoved(Participant *)), SLOT(removeParticipant(Participant *)));
> + Q_FOREACH(Participant *participant, mChatEntry->allParticipants()) {
> + addParticipantCache(participant);
> + }
> + fetchMore();
> + mCanFetchMore = !mParticipantsCache.isEmpty();
> + Q_EMIT canFetchMoreChanged();
> + Q_EMIT chatEntryChanged();
> +}
>
> === added file 'Ubuntu/Telephony/participantsmodel.h'
> --- Ubuntu/Telephony/participantsmodel.h 1970-01-01 00:00:00 +0000
> +++ Ubuntu/Telephony/participantsmodel.h 2017-03-03 12:37:00 +0000
> @@ -0,0 +1,92 @@
> +/*
> + * Copyright (C) 2013-2017 Canonical, Ltd.
> + *
> + * Authors:
> + * Gustavo Pichorim Boiko <gustavo.boiko at canonical.com>
> + * Tiago Salem Herrmann <tiago.herrmann at canonical.com>
> + *
> + * This file is part of telephony-service.
> + *
> + * telephony-service is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 3.
> + *
> + * telephony-service is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef PARTICIPANTSMODEL_H
> +#define PARTICIPANTSMODEL_H
> +
> +#include "chatentry.h"
> +#include <QAbstractListModel>
> +#include <QStringList>
> +#include <QQmlParserStatus>
> +#include <QQmlListProperty>
> +
> +class Participant;
> +
> +class ParticipantsModel : public QAbstractListModel, public QQmlParserStatus
> +{
> + Q_OBJECT
> + Q_INTERFACES(QQmlParserStatus)
> + Q_PROPERTY(int count READ rowCount NOTIFY countChanged)
> + Q_PROPERTY(bool canFetchMore READ canFetchMore NOTIFY canFetchMoreChanged)
> + Q_PROPERTY(ChatEntry* chatEntry READ chatEntry WRITE setChatEntry NOTIFY chatEntryChanged)
> + Q_ENUMS(Role)
> +
> +public:
> + enum Role {
> + IdentifierRole = Qt::UserRole,
> + AliasRole,
> + RolesRole,
> + StateRole
> + };
> +
> + explicit ParticipantsModel(QObject *parent = 0);
> + ~ParticipantsModel();
> +
> + Q_INVOKABLE virtual bool canFetchMore(const QModelIndex &parent = QModelIndex()) const;
> + Q_INVOKABLE virtual void fetchMore(const QModelIndex &parent = QModelIndex());
> + virtual QHash<int, QByteArray> roleNames() const;
> + virtual QVariant data(const QModelIndex &index, int role) const;
> + int rowCount(const QModelIndex &parent = QModelIndex()) const;
> +
> + Q_INVOKABLE virtual QVariant get(int row) const;
> +
> + Q_INVOKABLE void setChatEntry(ChatEntry *entry);
> + ChatEntry* chatEntry() const;
> +
> + void addParticipantCache(Participant *participant);
> +
> + void classBegin();
> + void componentComplete();
> +
> +private Q_SLOTS:
> + void addParticipant(Participant *participant);
> + void removeParticipant(Participant *participant);
> +
> +Q_SIGNALS:
> + void countChanged();
> + void canFetchMoreChanged();
> + void chatEntryChanged();
> +
> +protected:
> + bool lessThan(const QString &left, const QString &right) const;
> + int positionForItem(const QString &item, bool cache = false) const;
> +
> +private:
> + QHash<int, QByteArray> mRoles;
> + QList<Participant*> mParticipants;
> + bool mWaitingForQml;
> + bool mCanFetchMore;
> + ChatEntry *mChatEntry;
> + QList<Participant*> mParticipantsCache;
> +};
> +
> +#endif // HISTORYMODEL_H
Just change by // PARTICIPANTSMODEL_H
>
> === modified file 'libtelephonyservice/chatentry.cpp'
> --- libtelephonyservice/chatentry.cpp 2017-03-03 12:36:59 +0000
> +++ libtelephonyservice/chatentry.cpp 2017-03-03 12:37:00 +0000
> @@ -141,15 +141,18 @@
> updateParticipants(mParticipants,
> groupMembersAdded,
> groupMembersRemoved,
> - account);
> + account,
> + 0);
Can you replace the hardcoded numbers by an enum?
> updateParticipants(mLocalPendingParticipants,
> groupLocalPendingMembersAdded,
> groupMembersRemoved + groupMembersAdded, // if contacts move to the main list, remove from the pending one
> - account);
> + account,
> + 1);
Same here.
> updateParticipants(mRemotePendingParticipants,
> groupRemotePendingMembersAdded,
> groupMembersRemoved + groupMembersAdded, // if contacts move to the main list, remove from the pending one
> - account);
> + account,
> + 2);
And here.
>
> // generate the list of participant IDs again
> mParticipantIds.clear();
>
> === modified file 'libtelephonyservice/chatentry.h'
> --- libtelephonyservice/chatentry.h 2017-03-03 12:36:59 +0000
> +++ libtelephonyservice/chatentry.h 2017-03-03 12:37:00 +0000
> @@ -165,7 +168,7 @@
> QVariantMap generateProperties() const;
>
> void clearParticipants();
> - void updateParticipants(QList<Participant*> &list, const Tp::Contacts &added, const Tp::Contacts &removed, AccountEntry *account);
> + void updateParticipants(QList<Participant*> &list, const Tp::Contacts &added, const Tp::Contacts &removed, AccountEntry *account, uint pending = 0);
Please use an enum instead of hardcoded numeric values.
>
> private Q_SLOTS:
> void onTextChannelAvailable(const Tp::TextChannelPtr &channel);
--
https://code.launchpad.net/~phablet-team/telephony-service/add-participants-model/+merge/317390
Your team Ubuntu Phablet Team is subscribed to branch lp:~phablet-team/telephony-service/multiple_performance_improvements1.
More information about the Ubuntu-reviews
mailing list