Fix holding behavior across self-manged ConnectionServices.
- CallsManager#holdActiveCallForNewCall - make logs more concise.
- CallsManager#makeRoomForOutgoingCall - there is behavior which assumes
that if a new call is added by the SAME connection service that we will
let the ConnectionService worry about hold/unhold on its own. That is a
legacy behavior from Telephony and really doesn't make sense for VOIP
apps. For VOIP apps we SHOULD auto hold the ongoing call when a new
outgoing call is added. This was not noticed in the past because the focus
tracker was doing this in some cases so it wasn't obvious. However, even
then the signalling was not optimal as you could have an ACTIVE and
DIALING call, when realistically as soon as the new dialing call is added
it should hold the old active call.
- CSFocusManager - add local log of focus call to dumpsys to make tracking
focus issues easier in dumpsys.
- Updated the test app to include another connection service which was
useful for reproing this issue.
Test: Manual tests with test app; make calls across different cs in same
app.
Test: Added new unit test coverage for these cases.
Fixes: 279669173
Change-Id: If7d8e9f7280ce1d78b16a0d3b2f0b50b1063a4e6
diff --git a/src/com/android/server/telecom/CallsManager.java b/src/com/android/server/telecom/CallsManager.java
index ccc8e59..e0e17cc 100644
--- a/src/com/android/server/telecom/CallsManager.java
+++ b/src/com/android/server/telecom/CallsManager.java
@@ -3412,10 +3412,11 @@
*/
boolean holdActiveCallForNewCall(Call call) {
Call activeCall = (Call) mConnectionSvrFocusMgr.getCurrentFocusCall();
- Log.i(this, "holdActiveCallForNewCall, newCall: %s, activeCall: %s", call, activeCall);
+ Log.i(this, "holdActiveCallForNewCall, newCall: %s, activeCall: %s", call.getId(),
+ (activeCall == null ? "<none>" : activeCall.getId()));
if (activeCall != null && activeCall != call) {
if (canHold(activeCall)) {
- activeCall.hold();
+ activeCall.hold("swap to " + call.getId());
return true;
} else if (supportsHold(activeCall)
&& areFromSameSource(activeCall, call)) {
@@ -4900,12 +4901,25 @@
liveCallPhoneAccount);
}
- // First thing, if we are trying to make a call with the same phone account as the live
- // call, then allow it so that the connection service can make its own decision about
- // how to handle the new call relative to the current one.
+ // First thing, for managed calls, if we are trying to make a call with the same phone
+ // account as the live call, then allow it so that the connection service can make its own
+ // decision about how to handle the new call relative to the current one.
+ // Note: This behavior is primarily in place because Telephony historically manages the
+ // state of the calls it tracks by itself, holding and unholding as needed. Self-managed
+ // calls, even though from the same package are normally held/unheld automatically by
+ // Telecom. Calls within a single ConnectionService get held/unheld automatically during
+ // "swap" operations by CallsManager#holdActiveCallForNewCall. There is, however, a quirk
+ // in that if an app declares TWO different ConnectionServices, holdActiveCallForNewCall
+ // would not work correctly because focus switches between ConnectionServices, yet we
+ // tended to assume that if the calls are from the same package that the hold/unhold should
+ // be done by the app. That was a bad assumption as it meant that we could have two active
+ // calls.
+ // TODO(b/280826075): We need to come back and revisit all this logic in a holistic manner.
if (PhoneAccountHandle.areFromSamePackage(liveCallPhoneAccount,
- call.getTargetPhoneAccount())) {
- Log.i(this, "makeRoomForOutgoingCall: phoneAccount matches.");
+ call.getTargetPhoneAccount())
+ && !call.isSelfManaged()
+ && !liveCall.isSelfManaged()) {
+ Log.i(this, "makeRoomForOutgoingCall: managed phoneAccount matches");
call.getAnalytics().setCallIsAdditional(true);
liveCall.getAnalytics().setCallIsInterrupted(true);
return true;
@@ -5485,6 +5499,13 @@
impl.dump(pw);
pw.decreaseIndent();
}
+
+ if (mConnectionSvrFocusMgr != null) {
+ pw.println("mConnectionSvrFocusMgr:");
+ pw.increaseIndent();
+ mConnectionSvrFocusMgr.dump(pw);
+ pw.decreaseIndent();
+ }
}
/**
diff --git a/src/com/android/server/telecom/ConnectionServiceFocusManager.java b/src/com/android/server/telecom/ConnectionServiceFocusManager.java
index 6fbc494..3694727 100644
--- a/src/com/android/server/telecom/ConnectionServiceFocusManager.java
+++ b/src/com/android/server/telecom/ConnectionServiceFocusManager.java
@@ -25,8 +25,10 @@
import android.telecom.Log;
import android.telecom.Logging.Session;
import android.text.TextUtils;
+import android.util.LocalLog;
import com.android.internal.annotations.VisibleForTesting;
+import com.android.internal.util.IndentingPrintWriter;
import java.util.ArrayList;
import java.util.List;
@@ -41,6 +43,7 @@
public class ConnectionServiceFocusManager {
private static final String TAG = "ConnectionSvrFocusMgr";
private static final int GET_CURRENT_FOCUS_TIMEOUT_MILLIS = 1000;
+ private final LocalLog mLocalLog = new LocalLog(20);
/** Factory interface used to create the {@link ConnectionServiceFocusManager} instance. */
public interface ConnectionServiceFocusManagerFactory {
@@ -124,6 +127,11 @@
* @return {@code True} if this call can receive focus, {@code false} otherwise.
*/
boolean isFocusable();
+
+ /**
+ * @return the ID of the focusable for debug purposes.
+ */
+ String getId();
}
/** Interface define a call back for focus request event. */
@@ -361,10 +369,11 @@
}
private void updateCurrentFocusCall() {
+ CallFocus previousFocus = mCurrentFocusCall;
mCurrentFocusCall = null;
if (mCurrentFocus == null) {
- Log.d(this, "updateCurrentFocusCall: mCurrentFocus is null");
+ Log.i(this, "updateCurrentFocusCall: mCurrentFocus is null");
return;
}
@@ -377,11 +386,16 @@
for (CallFocus call : calls) {
if (PRIORITY_FOCUS_CALL_STATE.contains(call.getState())) {
mCurrentFocusCall = call;
+ if (previousFocus != call) {
+ mLocalLog.log(call.getId());
+ }
Log.i(this, "updateCurrentFocusCall %s", mCurrentFocusCall);
return;
}
}
-
+ if (previousFocus != null) {
+ mLocalLog.log("<none>");
+ }
Log.i(this, "updateCurrentFocusCall = null");
}
@@ -477,6 +491,11 @@
}
}
+ public void dump(IndentingPrintWriter pw) {
+ pw.println("Call Focus History:");
+ mLocalLog.dump(pw);
+ }
+
private final class FocusManagerHandler extends Handler {
FocusManagerHandler(Looper looper) {
super(looper);
diff --git a/testapps/AndroidManifest.xml b/testapps/AndroidManifest.xml
index dd8258a..645a42b 100644
--- a/testapps/AndroidManifest.xml
+++ b/testapps/AndroidManifest.xml
@@ -259,6 +259,15 @@
</intent-filter>
</service>
+ <service android:name="com.android.server.telecom.testapps.OtherSelfManagedConnectionService"
+ android:permission="android.permission.BIND_TELECOM_CONNECTION_SERVICE"
+ android:process="com.android.server.telecom.testapps.SelfMangingCallingApp"
+ android:exported="true">
+ <intent-filter>
+ <action android:name="android.telecom.ConnectionService"/>
+ </intent-filter>
+ </service>
+
<receiver android:exported="false"
android:process="com.android.server.telecom.testapps.SelfMangingCallingApp"
android:name="com.android.server.telecom.testapps.SelfManagedCallNotificationReceiver"/>
diff --git a/testapps/res/layout/self_managed_sample_main.xml b/testapps/res/layout/self_managed_sample_main.xml
index d26d629..98b879a 100644
--- a/testapps/res/layout/self_managed_sample_main.xml
+++ b/testapps/res/layout/self_managed_sample_main.xml
@@ -55,6 +55,12 @@
android:layout_height="wrap_content"
android:background="@color/test_call_b_color"
android:text="2"/>
+ <RadioButton
+ android:id="@+id/useAcct3Button"
+ android:layout_width="wrap_content"
+ android:layout_height="wrap_content"
+ android:background="@color/test_call_c_color"
+ android:text="3"/>
</RadioGroup>
<TextView
android:id="@+id/hasFocus"
diff --git a/testapps/res/values/colors.xml b/testapps/res/values/colors.xml
index 3939e78..9447ac8 100644
--- a/testapps/res/values/colors.xml
+++ b/testapps/res/values/colors.xml
@@ -17,4 +17,5 @@
<resources>
<color name="test_call_a_color">#f2eebf</color>
<color name="test_call_b_color">#afc5e6</color>
+ <color name="test_call_c_color">#c5afe6</color>
</resources>
diff --git a/testapps/src/com/android/server/telecom/testapps/OtherSelfManagedConnectionService.java b/testapps/src/com/android/server/telecom/testapps/OtherSelfManagedConnectionService.java
new file mode 100644
index 0000000..7bb9830
--- /dev/null
+++ b/testapps/src/com/android/server/telecom/testapps/OtherSelfManagedConnectionService.java
@@ -0,0 +1,20 @@
+/*
+ * Copyright (C) 2023 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.android.server.telecom.testapps;
+
+public class OtherSelfManagedConnectionService extends SelfManagedConnectionService {
+}
diff --git a/testapps/src/com/android/server/telecom/testapps/SelfManagedCallList.java b/testapps/src/com/android/server/telecom/testapps/SelfManagedCallList.java
index d4661ff..273b060 100644
--- a/testapps/src/com/android/server/telecom/testapps/SelfManagedCallList.java
+++ b/testapps/src/com/android/server/telecom/testapps/SelfManagedCallList.java
@@ -46,20 +46,27 @@
public static String SELF_MANAGED_ACCOUNT_1 = "1";
public static String SELF_MANAGED_ACCOUNT_2 = "2";
+ public static String SELF_MANAGED_ACCOUNT_1A = "1A";
public static String SELF_MANAGED_ACCOUNT_3 = "3";
public static String SELF_MANAGED_NAME_1 = "SuperCall";
public static String SELF_MANAGED_NAME_2 = "Mega Call";
- public static String SELF_MANAGED_NAME_3 = "SM Call";
+ public static String SELF_MANAGED_NAME_1A = "SM Call";
+ public static String SELF_MANAGED_NAME_3 = "Sep Process";
public static String CUSTOM_URI_SCHEME = "custom";
private static SelfManagedCallList sInstance;
private static ComponentName COMPONENT_NAME = new ComponentName(
SelfManagedCallList.class.getPackage().getName(),
SelfManagedConnectionService.class.getName());
+ private static ComponentName OTHER_COMPONENT_NAME = new ComponentName(
+ SelfManagedCallList.class.getPackage().getName(),
+ OtherSelfManagedConnectionService.class.getName());
private static Uri SELF_MANAGED_ADDRESS_1 = Uri.fromParts(PhoneAccount.SCHEME_TEL, "555-1212",
"");
private static Uri SELF_MANAGED_ADDRESS_2 = Uri.fromParts(PhoneAccount.SCHEME_SIP,
"me@test.org", "");
+ private static Uri SELF_MANAGED_ADDRESS_3 = Uri.fromParts(PhoneAccount.SCHEME_SIP,
+ "hilda@test.org", "");
private static Map<String, PhoneAccountHandle> mPhoneAccounts = new ArrayMap();
public static SelfManagedCallList getInstance() {
@@ -101,20 +108,29 @@
SELF_MANAGED_NAME_1, true /* areCallsLogged */);
registerPhoneAccount(context, SELF_MANAGED_ACCOUNT_2, SELF_MANAGED_ADDRESS_2,
SELF_MANAGED_NAME_2, false /* areCallsLogged */);
- registerPhoneAccount(context, SELF_MANAGED_ACCOUNT_3, SELF_MANAGED_ADDRESS_1,
- SELF_MANAGED_NAME_3, true /* areCallsLogged */);
+ registerPhoneAccount(context, SELF_MANAGED_ACCOUNT_1A, SELF_MANAGED_ADDRESS_1,
+ SELF_MANAGED_NAME_1A, true /* areCallsLogged */);
+ registerPhoneAccount(context, SELF_MANAGED_ACCOUNT_1A, SELF_MANAGED_ADDRESS_1,
+ SELF_MANAGED_NAME_1A, true /* areCallsLogged */);
+ registerPhoneAccount(context, OTHER_COMPONENT_NAME, SELF_MANAGED_ACCOUNT_3,
+ SELF_MANAGED_ADDRESS_3, SELF_MANAGED_NAME_3, false /* areCallsLogged */);
}
public void registerPhoneAccount(Context context, String id, Uri address, String name,
- boolean areCallsLogged) {
- PhoneAccountHandle handle = new PhoneAccountHandle(COMPONENT_NAME, id);
+ boolean areCallsLogged) {
+ registerPhoneAccount(context, COMPONENT_NAME, id, address, name, areCallsLogged);
+ }
+
+ public void registerPhoneAccount(Context context, ComponentName componentName, String id,
+ Uri address, String name, boolean areCallsLogged) {
+ PhoneAccountHandle handle = new PhoneAccountHandle(componentName, id);
mPhoneAccounts.put(id, handle);
Bundle extras = new Bundle();
extras.putBoolean(PhoneAccount.EXTRA_SUPPORTS_HANDOVER_TO, true);
if (areCallsLogged) {
extras.putBoolean(PhoneAccount.EXTRA_LOG_SELF_MANAGED_CALLS, true);
}
- if (id.equals(SELF_MANAGED_ACCOUNT_3)) {
+ if (id.equals(SELF_MANAGED_ACCOUNT_1A)) {
extras.putBoolean(PhoneAccount.EXTRA_ADD_SELF_MANAGED_CALLS_TO_INCALLSERVICE, true);
}
PhoneAccount.Builder builder = PhoneAccount.builder(handle, name)
diff --git a/testapps/src/com/android/server/telecom/testapps/SelfManagedCallListAdapter.java b/testapps/src/com/android/server/telecom/testapps/SelfManagedCallListAdapter.java
index 75ceb62..475f255 100644
--- a/testapps/src/com/android/server/telecom/testapps/SelfManagedCallListAdapter.java
+++ b/testapps/src/com/android/server/telecom/testapps/SelfManagedCallListAdapter.java
@@ -166,8 +166,10 @@
SelfManagedConnection.EXTRA_PHONE_ACCOUNT_HANDLE);
if (phoneAccountHandle.getId().equals(SelfManagedCallList.SELF_MANAGED_ACCOUNT_1)) {
result.setBackgroundColor(result.getContext().getColor(R.color.test_call_a_color));
- } else {
+ } else if (phoneAccountHandle.getId().equals(SelfManagedCallList.SELF_MANAGED_ACCOUNT_2)) {
result.setBackgroundColor(result.getContext().getColor(R.color.test_call_b_color));
+ } else {
+ result.setBackgroundColor(result.getContext().getColor(R.color.test_call_c_color));
}
CallAudioState audioState = connection.getCallAudioState();
diff --git a/testapps/src/com/android/server/telecom/testapps/SelfManagedCallingActivity.java b/testapps/src/com/android/server/telecom/testapps/SelfManagedCallingActivity.java
index 44410d2..5cdaf3d 100644
--- a/testapps/src/com/android/server/telecom/testapps/SelfManagedCallingActivity.java
+++ b/testapps/src/com/android/server/telecom/testapps/SelfManagedCallingActivity.java
@@ -43,8 +43,6 @@
import android.widget.TextView;
import android.widget.Toast;
-import com.android.server.telecom.testapps.R;
-
import java.util.Objects;
/**
@@ -66,6 +64,7 @@
private Button mDisableCarMode;
private RadioButton mUseAcct1Button;
private RadioButton mUseAcct2Button;
+ private RadioButton mUseAcct3Button;
private CheckBox mHoldableCheckbox;
private CheckBox mVideoCallCheckbox;
private EditText mNumber;
@@ -165,6 +164,7 @@
}));
mUseAcct1Button = findViewById(R.id.useAcct1Button);
mUseAcct2Button = findViewById(R.id.useAcct2Button);
+ mUseAcct3Button = findViewById(R.id.useAcct3Button);
mHasFocus = findViewById(R.id.hasFocus);
mVideoCallCheckbox = findViewById(R.id.videoCall);
mHoldableCheckbox = findViewById(R.id.holdable);
@@ -183,6 +183,8 @@
return mCallList.getPhoneAccountHandle(SelfManagedCallList.SELF_MANAGED_ACCOUNT_1);
} else if (mUseAcct2Button.isChecked()) {
return mCallList.getPhoneAccountHandle(SelfManagedCallList.SELF_MANAGED_ACCOUNT_2);
+ } else if (mUseAcct3Button.isChecked()) {
+ return mCallList.getPhoneAccountHandle(SelfManagedCallList.SELF_MANAGED_ACCOUNT_3);
}
return null;
}
@@ -214,8 +216,7 @@
private void placeSelfManagedOutgoingCall() {
TelecomManager tm = TelecomManager.from(this);
- PhoneAccountHandle phoneAccountHandle = mCallList.getPhoneAccountHandle(
- SelfManagedCallList.SELF_MANAGED_ACCOUNT_3);
+ PhoneAccountHandle phoneAccountHandle = getSelectedPhoneAccountHandle();
if (mCheckIfPermittedBeforeCalling.isChecked()) {
Toast.makeText(this, R.string.outgoingCallNotPermitted, Toast.LENGTH_SHORT).show();
@@ -264,7 +265,7 @@
private void placeSelfManagedIncomingCall() {
TelecomManager tm = TelecomManager.from(this);
PhoneAccountHandle phoneAccountHandle = mCallList.getPhoneAccountHandle(
- SelfManagedCallList.SELF_MANAGED_ACCOUNT_3);
+ SelfManagedCallList.SELF_MANAGED_ACCOUNT_1A);
if (mCheckIfPermittedBeforeCalling.isChecked()) {
if (!tm.isIncomingCallPermitted(phoneAccountHandle)) {
diff --git a/tests/src/com/android/server/telecom/tests/CallsManagerTest.java b/tests/src/com/android/server/telecom/tests/CallsManagerTest.java
index fc4e935..413989b 100644
--- a/tests/src/com/android/server/telecom/tests/CallsManagerTest.java
+++ b/tests/src/com/android/server/telecom/tests/CallsManagerTest.java
@@ -166,6 +166,8 @@
ComponentName.unflattenFromString("com.voip/.Stuff"), "Voip1");
private static final PhoneAccountHandle SELF_MANAGED_HANDLE = new PhoneAccountHandle(
ComponentName.unflattenFromString("com.baz/.Self"), "Self");
+ private static final PhoneAccountHandle SELF_MANAGED_2_HANDLE = new PhoneAccountHandle(
+ ComponentName.unflattenFromString("com.baz/.Self2"), "Self2");
private static final PhoneAccount SIM_1_ACCOUNT = new PhoneAccount.Builder(SIM_1_HANDLE, "Sim1")
.setCapabilities(PhoneAccount.CAPABILITY_SIM_SUBSCRIPTION
| PhoneAccount.CAPABILITY_CALL_PROVIDER
@@ -190,6 +192,11 @@
.setCapabilities(PhoneAccount.CAPABILITY_SELF_MANAGED)
.setIsEnabled(true)
.build();
+ private static final PhoneAccount SELF_MANAGED_2_ACCOUNT = new PhoneAccount.Builder(
+ SELF_MANAGED_2_HANDLE, "Self2")
+ .setCapabilities(PhoneAccount.CAPABILITY_SELF_MANAGED)
+ .setIsEnabled(true)
+ .build();
private static final Uri TEST_ADDRESS = Uri.parse("tel:555-1212");
private static final Uri TEST_ADDRESS2 = Uri.parse("tel:555-1213");
private static final Uri TEST_ADDRESS3 = Uri.parse("tel:555-1214");
@@ -818,7 +825,7 @@
mCallsManager.answerCall(incomingCall, VideoProfile.STATE_AUDIO_ONLY);
// THEN the ongoing call is held and the focus request for incoming call is sent
- verify(ongoingCall).hold();
+ verify(ongoingCall).hold(anyString());
verifyFocusRequestAndExecuteCallback(incomingCall);
// and the incoming call is answered.
@@ -1026,7 +1033,7 @@
mCallsManager.markCallAsActive(newCall);
// THEN the ongoing call is held
- verify(ongoingCall).hold();
+ verify(ongoingCall).hold(anyString());
verifyFocusRequestAndExecuteCallback(newCall);
// and the new call is active
@@ -1683,6 +1690,57 @@
assertTrue(mCallsManager.makeRoomForOutgoingCall(ongoingCall2));
}
+ /**
+ * Test where a VoIP app adds another new call and has one active already; ensure we hold the
+ * active call. This assumes same connection service in the same app.
+ */
+ @SmallTest
+ @Test
+ public void testMakeRoomForOutgoingCallForSameVoipApp() {
+ Call activeCall = addSpyCall(SELF_MANAGED_HANDLE, null /* connMgr */,
+ CallState.ACTIVE, Connection.CAPABILITY_HOLD | Connection.CAPABILITY_SUPPORT_HOLD,
+ 0 /* properties */);
+ Call newDialingCall = createCall(SELF_MANAGED_HANDLE, CallState.DIALING);
+ newDialingCall.setConnectionProperties(Connection.CAPABILITY_HOLD
+ | Connection.CAPABILITY_SUPPORT_HOLD);
+ assertTrue(mCallsManager.makeRoomForOutgoingCall(newDialingCall));
+ verify(activeCall).hold(anyString());
+ }
+
+ /**
+ * Test where a VoIP app adds another new call and has one active already; ensure we hold the
+ * active call. This assumes different connection services in the same app.
+ */
+ @SmallTest
+ @Test
+ public void testMakeRoomForOutgoingCallForSameVoipAppDifferentConnectionService() {
+ Call activeCall = addSpyCall(SELF_MANAGED_HANDLE, null /* connMgr */,
+ CallState.ACTIVE, Connection.CAPABILITY_HOLD | Connection.CAPABILITY_SUPPORT_HOLD,
+ 0 /* properties */);
+ Call newDialingCall = createCall(SELF_MANAGED_2_HANDLE, CallState.DIALING);
+ newDialingCall.setConnectionProperties(Connection.CAPABILITY_HOLD
+ | Connection.CAPABILITY_SUPPORT_HOLD);
+ assertTrue(mCallsManager.makeRoomForOutgoingCall(newDialingCall));
+ verify(activeCall).hold(anyString());
+ }
+
+ /**
+ * Test where a VoIP app adds another new call and has one active already; ensure we hold the
+ * active call. This assumes different connection services in the same app.
+ */
+ @SmallTest
+ @Test
+ public void testMakeRoomForOutgoingCallForSameNonVoipApp() {
+ Call activeCall = addSpyCall(SIM_1_HANDLE, null /* connMgr */,
+ CallState.ACTIVE, Connection.CAPABILITY_HOLD | Connection.CAPABILITY_SUPPORT_HOLD,
+ 0 /* properties */);
+ Call newDialingCall = createCall(SIM_1_HANDLE, CallState.DIALING);
+ newDialingCall.setConnectionProperties(Connection.CAPABILITY_HOLD
+ | Connection.CAPABILITY_SUPPORT_HOLD);
+ assertTrue(mCallsManager.makeRoomForOutgoingCall(newDialingCall));
+ verify(activeCall, never()).hold(anyString());
+ }
+
@SmallTest
@Test
public void testMakeRoomForOutgoingCallHasOutgoingCallSelectingAccount() {
@@ -2968,6 +3026,10 @@
mClockProxy,
mToastFactory);
ongoingCall.setState(initialState, "just cuz");
+ if (targetPhoneAccount == SELF_MANAGED_HANDLE
+ || targetPhoneAccount == SELF_MANAGED_2_HANDLE) {
+ ongoingCall.setIsSelfManaged(true);
+ }
return ongoingCall;
}