Merge lp:~boiko/telephony-service/preserve_call_status into lp:telephony-service

[Gustavo Pichorim Boiko]

Avoid flickering the UI on client apps by addressing the following points:
- Get the "hasCalls" property value from the handler even before the telepathy observer is properly registered
- Delay the cleaning up of manager data when unregistering the observer

Avoid flickering the UI on client apps by addressing the following points:
- Get the "hasCalls" property value from the handler even before the telepathy observer is properly registered
- Delay the cleaning up of manager data when unregistering the observer

[Tiago Salem Herrmann (tiagosh)]

I think there might be a case where this logic fails.
Correct me if I am wrong, but consider the case where we are on a call and switch to another app, then while dialer-app is in background the remote party hangs up. When you bring the dialer-app to foreground again and register the observer, onCallChannelAvailable() wont be called as there are no active calls at the moment, so we will end up in an inconsistent state.

139 void CallManager::onCallChannelAvailable(Tp::CallChannelPtr channel)
140 {
141 + // if this is the first call after re-registering the observer, clear the data

review: Needs Information

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:777
http://jenkins.qa.ubuntu.com/job/telephony-service-ci/115/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/telephony-service-trusty-amd64-ci/39
    SUCCESS: http://jenkins.qa.ubuntu.com/job/telephony-service-trusty-armhf-ci/43
        deb: http://jenkins.qa.ubuntu.com/job/telephony-service-trusty-armhf-ci/43/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/telephony-service-trusty-i386-ci/39

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/telephony-service-ci/115/rebuild

review: Approve (continuous-integration)

[Gustavo Pichorim Boiko (boiko)]

> I think there might be a case where this logic fails.
> Correct me if I am wrong, but consider the case where we are on a call and
> switch to another app, then while dialer-app is in background the remote party
> hangs up. When you bring the dialer-app to foreground again and register the
> observer, onCallChannelAvailable() wont be called as there are no active calls
> at the moment, so we will end up in an inconsistent state.
>
>
> 139 void CallManager::onCallChannelAvailable(Tp::CallChannelPtr channel)
> 140 {
> 141 + // if this is the first call after re-registering the observer,
> clear the data

At first I also thought this case was not covered, but it is: the Tp::CallChannel has already introspected the DBus object for the call, so even after the observer is unregistered, the correct signals from the channel are still emitted.
So even if dialer-app is in background (sigstopped), when it comes back to life it is going to receive the notifications for the existing calls even before registering the observer again.

[Gustavo Pichorim Boiko (boiko)]

Are there any related MPs required for this MP to build/function as expected? Please list.
No

Is your branch in sync with latest trunk (e.g. bzr pull lp:trunk -> no changes)
Yes

Did you perform an exploratory manual test run of your code change and any related functionality on device or emulator?
Yes

Did you successfully run all tests found in your component's Test Plan (https://wiki.ubuntu.com/Process/Merges/TestPlan/telephony-service) on device or emulator?
Yes

If you changed the UI, was the change specified/approved by design?
No UI changes

If you changed the packaging (debian), did you subscribe a core-dev to this MP?
No packaging changes

[Tiago Salem Herrmann (tiagosh)]

Did you perform an exploratory manual test run of the code change and any related functionality on device or emulator?
Yes

Did you successfully run all tests found in your component's Test Plan (https://wiki.ubuntu.com/Process/Merges/TestPlan/telephony-service) on device or emulator?
Yes

Did CI run pass? If not, please explain why.
Yes

Have you checked that submitter has accurately filled out the submitter checklist and has taken no shortcut?
Yes

The code looks good and it works as expected.

review: Approve

=== modified file 'handler/Handler.xml'
--- handler/Handler.xml	2014-02-17 12:43:27 +0000
+++ handler/Handler.xml	2014-02-20 21:36:37 +0000
@@ -67,6 +67,12 @@
             <arg name="objectPath" type="s" direction="in"/>
             <arg name="key" type="s" direction="in"/>
         </method>
+        <method name="HasCalls">
+            <dox:d><![CDATA[
+                Returns whether the handler is managing any call
+            ]]></dox:d>
+            <arg name="result" type="b" direction="out"/>
+        </method>
         <method name="GetCallProperties">
             <dox:d><![CDATA[
                 Get the properties of a given call channel

=== modified file 'handler/callhandler.cpp'
--- handler/callhandler.cpp	2014-01-29 16:22:30 +0000
+++ handler/callhandler.cpp	2014-02-20 21:36:37 +0000
@@ -64,6 +64,24 @@
     return properties;
 }
 
+bool CallHandler::hasCalls() const
+{
+    bool hasActiveCalls = false;
+
+    Q_FOREACH(const Tp::CallChannelPtr channel, mCallChannels) {
+        bool incoming = channel->initiatorContact() != TelepathyHelper::instance()->account()->connection()->selfContact();
+        bool dialing = !incoming && (channel->callState() == Tp::CallStateInitialised);
+        bool active = channel->callState() == Tp::CallStateActive;
+
+        if (dialing || active) {
+            hasActiveCalls = true;
+            break;
+        }
+    }
+
+    return hasActiveCalls;
+}
+
 CallHandler::CallHandler(QObject *parent)
 : QObject(parent)
 {

=== modified file 'handler/callhandler.h'
--- handler/callhandler.h	2014-01-29 16:22:30 +0000
+++ handler/callhandler.h	2014-02-20 21:36:37 +0000
@@ -36,6 +36,7 @@
 public:
     static CallHandler *instance();
     QVariantMap getCallProperties(const QString &objectPath);
+    bool hasCalls() const;
 
 public Q_SLOTS:
     void onCallChannelAvailable(Tp::CallChannelPtr channel);

=== modified file 'handler/handlerdbus.cpp'
--- handler/handlerdbus.cpp	2014-02-17 12:43:27 +0000
+++ handler/handlerdbus.cpp	2014-02-20 21:36:37 +0000
@@ -48,6 +48,11 @@
     return CallHandler::instance()->getCallProperties(objectPath);
 }
 
+bool HandlerDBus::HasCalls()
+{
+    return CallHandler::instance()->hasCalls();
+}
+
 bool HandlerDBus::connectToBus()
 {
     bool ok = QDBusConnection::sessionBus().registerService(DBUS_SERVICE);

=== modified file 'handler/handlerdbus.h'
--- handler/handlerdbus.h	2014-02-17 12:43:27 +0000
+++ handler/handlerdbus.h	2014-02-20 21:36:37 +0000
@@ -40,6 +40,7 @@
     ~HandlerDBus();
 
     QVariantMap GetCallProperties(const QString &objectPath);
+    bool HasCalls();
 
 public Q_SLOTS:
     bool connectToBus();

=== modified file 'libtelephonyservice/callmanager.cpp'
--- libtelephonyservice/callmanager.cpp	2014-01-28 13:16:58 +0000
+++ libtelephonyservice/callmanager.cpp	2014-02-20 21:36:37 +0000
@@ -38,7 +38,7 @@
 }
 
 CallManager::CallManager(QObject *parent)
-: QObject(parent)
+: QObject(parent), mNeedsUpdate(false)
 {
     connect(TelepathyHelper::instance(), SIGNAL(connectedChanged()), SLOT(onConnectedChanged()));
     connect(TelepathyHelper::instance(), SIGNAL(channelObserverUnregistered()), SLOT(onChannelObserverUnregistered()));
@@ -46,11 +46,9 @@
 
 void CallManager::onChannelObserverUnregistered()
 {
-    mCallEntries.clear();
-    Q_EMIT hasCallsChanged();
-    Q_EMIT hasBackgroundCallChanged();
-    Q_EMIT foregroundCallChanged();
-    Q_EMIT backgroundCallChanged();
+    // do not clear the manager right now, wait until the observer is re-registered
+    // to avoid flickering in the UI
+    mNeedsUpdate = true;
 }
 
 void CallManager::startCall(const QString &phoneNumber)
@@ -126,7 +124,21 @@
 
 bool CallManager::hasCalls() const
 {
-    return activeCallsCount() > 0;
+    // check if the callmanager already has active calls
+    if (activeCallsCount() > 0) {
+        return true;
+    }
+
+    // if that's not the case, query the teleophony-service-handler for the availability of calls
+    // this is done only to get the live call view on clients as soon as possible, even before the
+    // telepathy observer is configured
+    QDBusInterface *phoneAppHandler = TelepathyHelper::instance()->handlerInterface();
+    QDBusReply<bool> reply = phoneAppHandler->call("HasCalls");
+    if (reply.isValid()) {
+        return reply.value();
+    }
+
+    return false;
 }
 
 bool CallManager::hasBackgroundCall() const
@@ -136,6 +148,15 @@
 
 void CallManager::onCallChannelAvailable(Tp::CallChannelPtr channel)
 {
+    // if this is the first call after re-registering the observer, clear the data
+    if (mNeedsUpdate) {
+        Q_FOREACH(CallEntry *entry, mCallEntries) {
+            entry->deleteLater();
+        }
+        mCallEntries.clear();
+        mNeedsUpdate = false;
+    }
+
     CallEntry *entry = new CallEntry(channel, this);
     if (entry->phoneNumber() == getVoicemailNumber()) {
         entry->setVoicemail(true);

=== modified file 'libtelephonyservice/callmanager.h'
--- libtelephonyservice/callmanager.h	2014-01-20 12:57:43 +0000
+++ libtelephonyservice/callmanager.h	2014-02-20 21:36:37 +0000
@@ -84,6 +84,7 @@
 
     QList<CallEntry*> mCallEntries;
     QString mVoicemailNumber;
+    bool mNeedsUpdate;
 };
 
 #endif // CALLMANAGER_H