[Merge] lp:~tiagosh/telephony-service/telepathyhelper-init into lp:telephony-service

[Gustavo Pichorim Boiko]

Review: Needs Fixing

Overall the code is much more readable and less complex.
A couple things though:
- can you add a unit test to make sure the setupReady is emitted when there is no account and also that it is only emitted when all accounts are ready and not before that?
- also, you should go through all the files changed and update the copyright headers when needed


Diff comments:

> 
> === modified file 'libtelephonyservice/accountentry.cpp'
> --- libtelephonyservice/accountentry.cpp	2015-06-11 16:54:33 +0000
> +++ libtelephonyservice/accountentry.cpp	2015-08-19 18:05:08 +0000
> @@ -208,32 +176,44 @@
>  
>      connect(mAccount->connection()->selfContact().data(),
>              SIGNAL(presenceChanged(Tp::Presence)),
> +            SIGNAL(statusChanged()));
> +
> +    connect(mAccount->connection()->selfContact().data(),
> +            SIGNAL(presenceChanged(Tp::Presence)),
> +            SIGNAL(statusMessageChanged()));
> +
> +    connect(mAccount->connection()->selfContact().data(),
> +            SIGNAL(presenceChanged(Tp::Presence)),
> +            SIGNAL(activeChanged()));
> +
> +    connect(mAccount->connection()->selfContact().data(),
> +            SIGNAL(presenceChanged(Tp::Presence)),
>              SIGNAL(connectedChanged()));
> +
>  }
>  
> -void AccountEntry::onSelfHandleChanged(uint handle)
> +void AccountEntry::onSelfContactChanged()
>  {
> -    Q_UNUSED(handle)
>      watchSelfContactPresence();
>  
>      Q_EMIT connectedChanged();
>      Q_EMIT selfContactIdChanged();
>  }
>  
> -void AccountEntry::onConnectionChanged()
> +void AccountEntry::onConnectionChanged(Tp::ConnectionPtr connection)

Do we really need to pass the connection as an argument? as far as I can see this is not needed: we can fetch it from mAccount inside the method itself. This way you don't need to register the telepathy type in the metaobject system.

>  {
> -    if (!mAccount->connection()) {
> -        // ensure the account gets connected
> -        ensureConnected();
> -    } else {
> -        mConnectionInfo.busName = mAccount->connection()->busName();
> -        mConnectionInfo.objectPath = mAccount->connection()->objectPath();
> +    if (!connection.isNull()) {
> +        mConnectionInfo.busName = connection->busName();
> +        mConnectionInfo.objectPath = connection->objectPath();
>  
> -        connect(mAccount->connection().data(),
> -                SIGNAL(selfHandleChanged(uint)),
> -                SLOT(onSelfHandleChanged(uint)));
> +        connect(connection.data(),
> +                SIGNAL(selfContactChanged()),
> +                SLOT(onSelfContactChanged()));
>  
>          watchSelfContactPresence();
> +    } else {
> +        mConnectionInfo.busName = QString();
> +        mConnectionInfo.objectPath = QString();

Can you use mConnectionInfo.busName.clear() and mConnectionInfo.objectPath.clear() ?

>      }
>  
>      Q_EMIT connectedChanged();
> 
> === modified file 'libtelephonyservice/multimediaaccountentry.cpp'
> --- libtelephonyservice/multimediaaccountentry.cpp	2015-07-23 01:25:44 +0000
> +++ libtelephonyservice/multimediaaccountentry.cpp	2015-08-19 18:05:08 +0000
> @@ -51,8 +51,8 @@
>      return mAccount->protocolInfo().addressableVCardFields();
>  }
>  
> -void MultimediaAccountEntry::onConnectionChanged()
> +void MultimediaAccountEntry::onConnectionChanged(Tp::ConnectionPtr connection)

Can you completelly remove the reimplementation (as the only thing it does is to call the base class version of it)?

>  {
>      // make sure the generic code is also run
> -    AccountEntry::onConnectionChanged();
> +    AccountEntry::onConnectionChanged(connection);
>  }
> 
> === modified file 'libtelephonyservice/telepathyhelper.cpp'
> --- libtelephonyservice/telepathyhelper.cpp	2015-07-02 03:22:47 +0000
> +++ libtelephonyservice/telepathyhelper.cpp	2015-08-19 18:05:08 +0000
> @@ -465,14 +446,15 @@
>          }
>      }
>  
> -    if (mAccounts.count() == 0) {
> -        mFirstTime = false;
> +    // get the number of pending accounts to be processed first
> +    mPendingAccountReady = mAccounts.count();

Can you rename the variable to reflect its actual content (something like mPendingAccountsCount)?

> +
> +    if (mPendingAccountReady == 0) {
> +        mReady = true;
>          Q_EMIT setupReady();
>          return;
>      }
>  
> -    updateConnectedStatus();
> -
>      Q_EMIT accountIdsChanged();
>      Q_EMIT accountsChanged();
>      Q_EMIT phoneAccountsChanged();


-- 
https://code.launchpad.net/~tiagosh/telephony-service/telepathyhelper-init/+merge/262991
Your team Ubuntu Phablet Team is subscribed to branch lp:telephony-service.