[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 &timestamp, 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