[Merge] lp:~tiagosh/telephony-service/chat_state into lp:telephony-service
Gustavo Pichorim Boiko
gustavo.boiko at canonical.com
Fri Nov 6 19:05:50 UTC 2015
Review: Needs Fixing
Some comments inline.
Diff comments:
>
> === modified file 'handler/Handler.xml'
> --- handler/Handler.xml 2015-11-03 13:44:20 +0000
> +++ handler/Handler.xml 2015-11-03 13:44:20 +0000
> @@ -44,6 +44,21 @@
> <arg name="messageIds" type="as" direction="in"/>
> <arg name="accountId" type="s" direction="in"/>
> </method>
> + <method name="StartChat">
> + <dox:d><![CDATA[
> + Start a chat with the given participants
> + ]]></dox:d>
> + <arg name="accountId" type="s" direction="in"/>
> + <arg name="participants" type="as" direction="in"/>
> + </method>
> + <method name="StartChatRoom">
> + <dox:d><![CDATA[
> + Start a chat room with the given properties
> + ]]></dox:d>
> + <arg name="accountId" type="s" direction="in"/>
Maybe we could have a initialParticipants argument that could have the list of participants to include in the chat room creation? Might be empty.
> + <arg name="properties" type="a{sv}" direction="in"/>
> + <annotation name="org.qtproject.QtDBus.QtTypeName.In1" value="QVariantMap"/>
> + </method>
> <method name="AcknowledgeAllMessages">
> <dox:d><![CDATA[
> Request all messages messages from the given numbers to be acknowledged
>
> === modified file 'handler/texthandler.cpp'
> --- handler/texthandler.cpp 2015-11-03 13:44:20 +0000
> +++ handler/texthandler.cpp 2015-11-03 13:44:20 +0000
> @@ -117,6 +117,12 @@
> SLOT(onContactsAvailable(Tp::PendingOperation*)));
> }
>
> +void TextHandler::startChatRoom(const QString &accountId, const QVariantMap &properties)
> +{
> + Q_UNUSED(accountId)
> + Q_UNUSED(properties)
Can you just add a // FIXME: implement here?
> +}
> +
> void TextHandler::startChat(const Tp::AccountPtr &account, const Tp::Contacts &contacts)
> {
> if (contacts.size() == 1) {
>
> === added file 'libtelephonyservice/chatentry.cpp'
> --- libtelephonyservice/chatentry.cpp 1970-01-01 00:00:00 +0000
> +++ libtelephonyservice/chatentry.cpp 2015-11-03 13:44:20 +0000
> @@ -0,0 +1,118 @@
> +/*
> + * Copyright (C) 2015 Canonical, Ltd.
> + *
> + * Authors:
> + * 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 "telepathyhelper.h"
> +#include "accountentry.h"
> +#include "chatentry.h"
> +
> +#include <TelepathyQt/Contact>
> +#include <TelepathyQt/PendingReady>
> +#include <TelepathyQt/Connection>
> +
> +Q_DECLARE_METATYPE(ContactChatStates)
> +
> +ChatEntry::ChatEntry(const Tp::TextChannelPtr &channel, QObject *parent) :
> + QObject(parent),
> + mChannel(channel)
> +{
> + qRegisterMetaType<ContactChatStates>();
> + mAccount = TelepathyHelper::instance()->accountForConnection(mChannel->connection());
> + Q_FOREACH (Tp::ContactPtr contact, mChannel->groupContacts(false)) {
> + ContactChatState *state = new ContactChatState(contact->id(), mChannel->chatState(contact));
> + mChatStates[contact->id()] = state;
> + }
> +
> + connect(channel.data(), SIGNAL(chatStateChanged(const Tp::ContactPtr &, Tp::ChannelChatState)),
> + this, SLOT(onChatStateChanged(const Tp::ContactPtr &,Tp::ChannelChatState)));
> + connect(channel.data(), SIGNAL(groupMembersChanged(const Tp::Contacts &, const Tp::Contacts &, const Tp::Contacts &,
> + const Tp::Contacts &, const Tp::Channel::GroupMemberChangeDetails &)), this, SIGNAL(participantsChanged()));
> +}
> +
> +ChatEntry::~ChatEntry()
> +{
> + QMap<QString, ContactChatState*> tmp = mChatStates;
> + mChatStates.clear();
> + Q_EMIT chatStatesChanged();
> + QMapIterator<QString, ContactChatState*> it(tmp);
> + while (it.hasNext()) {
> + it.next();
> + delete it.value();
> + }
As we don't deal with threading here, you can probably change the code to be just:
qDeleteAll(mChatStates.values());
mChatStates.clear();
Q_EMIT chatStatesChanged();
> +}
> +
> +void ChatEntry::onChatStateChanged(const Tp::ContactPtr &contact, Tp::ChannelChatState state)
> +{
> + if (mChatStates.contains(contact->id())) {
> + mChatStates[contact->id()]->setState(state);
> + return;
> + }
> +
> + ContactChatState *newState = new ContactChatState(contact->id(), state);
> + mChatStates[contact->id()] = newState;
> + Q_EMIT chatStatesChanged();
> +}
> +
> +ChatEntry::ChatType ChatEntry::chatType()
> +{
> + return (ChatType)mChannel->targetHandleType();
> +}
> +
> +Tp::TextChannelPtr ChatEntry::channel()
> +{
> + return mChannel;
> +}
> +
> +QStringList ChatEntry::participants()
> +{
> + QStringList participantList;
> + Q_FOREACH (Tp::ContactPtr contact, mChannel->groupContacts(false)) {
> + participantList << contact->id();
> + }
> + return participantList;
> +}
> +
> +AccountEntry *ChatEntry::account()
> +{
> + return mAccount;
> +}
> +
> +QQmlListProperty<ContactChatState> ChatEntry::chatStates()
> +{
> + return QQmlListProperty<ContactChatState>(this, 0, chatStatesCount, chatStatesAt);
> +}
> +
> +int ChatEntry::chatStatesCount(QQmlListProperty<ContactChatState> *p)
> +{
> + ChatEntry *entry = qobject_cast<ChatEntry*>(p->object);
> + if (!entry) {
> + return 0;
> + }
> + return entry->mChatStates.count();
> +}
> +
> +ContactChatState *ChatEntry::chatStatesAt(QQmlListProperty<ContactChatState> *p, int index)
> +{
> + ChatEntry *entry = qobject_cast<ChatEntry*>(p->object);
> + if (!entry) {
> + return 0;
> + }
> + return entry->mChatStates.values()[index];
> +}
>
> === modified file 'libtelephonyservice/chatmanager.cpp'
> --- libtelephonyservice/chatmanager.cpp 2015-11-03 13:44:20 +0000
> +++ libtelephonyservice/chatmanager.cpp 2015-11-03 13:44:20 +0000
> @@ -46,18 +46,42 @@
> argument.endStructure();
> return argument;
> }
> +
> ChatManager::ChatManager(QObject *parent)
> -: QObject(parent)
> +: QObject(parent),
> + mReady(TelepathyHelper::instance()->ready())
> {
> qDBusRegisterMetaType<AttachmentList>();
> qDBusRegisterMetaType<AttachmentStruct>();
> // wait one second for other acknowledge calls before acknowledging messages to avoid many round trips
> mMessagesAckTimer.setInterval(1000);
> mMessagesAckTimer.setSingleShot(true);
> + connect(TelepathyHelper::instance(), SIGNAL(channelObserverUnregistered()), SLOT(onChannelObserverUnregistered()));
> + connect(TelepathyHelper::instance(), SIGNAL(setupReady()), SLOT(onTelepathyReady()));
> connect(&mMessagesAckTimer, SIGNAL(timeout()), SLOT(onAckTimerTriggered()));
> connect(TelepathyHelper::instance(), SIGNAL(setupReady()), SLOT(onConnectedChanged()));
> }
>
> +void ChatManager::onTelepathyReady()
> +{
> + mReady = true;
> + Q_FOREACH(const Tp::TextChannelPtr &channel, mPendingChannels) {
> + onTextChannelAvailable(channel);
> + }
> + mPendingChannels.clear();
> +}
> +
> +void ChatManager::onChannelObserverUnregistered()
> +{
> + QList<ChatEntry*> tmp = mChatEntries;
> + mChatEntries.clear();
> + Q_EMIT chatsChanged();
> + Q_FOREACH(ChatEntry *entry, tmp) {
> + // for some reason deleteLater is not working
> + delete entry;
> + }
Just like the chat states, you can probably do:
qDeleteAll(mChatEntries);
mChatEntries.clear();
Q_EMIT chatsChanged();
> +}
> +
> void ChatManager::onConnectedChanged()
> {
> if (TelepathyHelper::instance()->ready()) {
> @@ -228,3 +285,63 @@
>
> mMessagesToAck.clear();
> }
> +
> +QList<ChatEntry*> ChatManager::chatEntries() const
> +{
> + return mChatEntries;
> +}
> +
> +ChatEntry *ChatManager::chatEntryForParticipants(const QString &accountId, const QStringList &participants, bool create)
> +{
> + if (participants.count() == 0 || accountId.isEmpty()) {
> + return NULL;
> + }
> +
> + Q_FOREACH (ChatEntry *chatEntry, mChatEntries) {
> + int participantCount = 0;
> + Tp::Contacts contacts = chatEntry->channel()->groupContacts(false);
> + if (participants.count() != contacts.count()) {
> + continue;
> + }
> + // iterate over participants
> + Q_FOREACH (const Tp::ContactPtr &contact, contacts) {
> + if (participants.contains(contact->id())) {
You need to check if the account uses phone number comparison here I think, and then use comparePhoneNumbers if necessary.
> + participantCount++;
> + } else {
> + break;
> + }
> + }
> + if (participantCount == participants.count()) {
> + return chatEntry;
> + }
> + }
> +
> + if (create) {
> + QDBusInterface *phoneAppHandler = TelepathyHelper::instance()->handlerInterface();
> + phoneAppHandler->call("StartChat", accountId, participants);
> + }
> + return NULL;
> +}
> +
> +ChatEntry *ChatManager::chatEntryForChatRoom(const QString &accountId, const QVariantMap &properties, bool create)
> +{
> + Q_UNUSED(accountId)
> + Q_UNUSED(properties)
> + Q_UNUSED(create)
Can you add a FIXME: implement here?
> +}
> +
> +QQmlListProperty<ChatEntry> ChatManager::chats()
> +{
> + return QQmlListProperty<ChatEntry>(this, 0, chatsCount, chatAt);
> +}
> +
> +int ChatManager::chatsCount(QQmlListProperty<ChatEntry> *p)
> +{
> + return ChatManager::instance()->chatEntries().count();
> +}
> +
> +ChatEntry *ChatManager::chatAt(QQmlListProperty<ChatEntry> *p, int index)
> +{
> + return ChatManager::instance()->chatEntries()[index];
> +}
> +
>
> === modified file 'libtelephonyservice/chatmanager.h'
> --- libtelephonyservice/chatmanager.h 2015-09-03 21:11:05 +0000
> +++ libtelephonyservice/chatmanager.h 2015-11-03 13:44:20 +0000
> @@ -24,25 +24,39 @@
>
> #include <QtCore/QObject>
> #include <QtCore/QMap>
> +#include <QQmlListProperty>
> #include <TelepathyQt/TextChannel>
> #include <TelepathyQt/ReceivedMessage>
> #include "dbustypes.h"
> +#include "chatentry.h"
>
> class ChatManager : public QObject
> {
> Q_OBJECT
> + Q_PROPERTY(QQmlListProperty<ChatEntry> chats
> + READ chats
> + NOTIFY chatsChanged)
I think this line is short enough for you to put everything on a single line.
> public:
> static ChatManager *instance();
>
> Q_INVOKABLE void sendMessage(const QStringList &recipients, const QString &message, const QString &accountId = QString::null);
> Q_INVOKABLE void sendMMS(const QStringList &recipients, const QString &message, const QVariant &attachments, const QString &accountId = QString:: null);
> + Q_INVOKABLE ChatEntry *chatEntryForParticipants(const QString &accountId, const QStringList &participants, bool create = false);
> + Q_INVOKABLE ChatEntry *chatEntryForChatRoom(const QString &accountId, const QVariantMap &properties, bool create);
> +
> + QQmlListProperty<ChatEntry> chats();
> + static int chatsCount(QQmlListProperty<ChatEntry> *p);
Can you rename this to chatCount()? just nitpicking :)
> + static ChatEntry* chatAt(QQmlListProperty<ChatEntry> *p, int index);
>
> Q_SIGNALS:
> void messageReceived(const QString &sender, const QString &message, const QDateTime ×tamp, const QString &messageId, bool unread);
> void messageSent(const QStringList &recipients, const QString &message);
> + void chatsChanged();
> + void chatEntryCreated(QString accountId, QStringList participants, ChatEntry *chatEntry);
>
> public Q_SLOTS:
> void onTextChannelAvailable(Tp::TextChannelPtr channel);
> + void onChannelInvalidated();
> void onConnectedChanged();
> void onMessageReceived(const Tp::ReceivedMessage &message);
> void onMessageSent(const Tp::Message &sentMessage, const Tp::MessageSendingFlags flags, const QString &message);
--
https://code.launchpad.net/~tiagosh/telephony-service/chat_state/+merge/264275
Your team Ubuntu Phablet Team is subscribed to branch lp:telephony-service.
More information about the Ubuntu-reviews
mailing list