Bluetooth: remove unnecessary state tracking in BluetoothSummaryUpdater
* LocalBluetoothAdapter is already a cache of BluetoothAdapter and its
methods should be used directly to obtain states instead of caching them
in BluetoothSummaryUpdater
- Use LocalBluetoothAdapter.isEnabled() to check whether Bluetooth
is enabled
- Use LocalBluetoothAdapter.getBondedDevices() to get list of bonded
devices
* BluetoothDevice is a stable Bluetooth API and its methods should not
incur large latency. We should use API methods as much as possible to
avoid intermediate wrappers
- Use BluetoothDevice.isConnected() to check if a device is connected
* Add more logging messages in error conditions
* Show status as "Not Connected" when there is a state mismatch (i.e.
adapter says it is connected, but no bonded device is connected)
* Updated unit tests to reflect the latest behavior
Bug: 65591907
Test: make, unit test, pair with Bluetooth devices, check Settings UI
Change-Id: I0fa54959c8bed8ac67a935f150785ba8197d0411
diff --git a/src/com/android/settings/bluetooth/BluetoothSummaryUpdater.java b/src/com/android/settings/bluetooth/BluetoothSummaryUpdater.java
index 7d2cc18..662cd70 100644
--- a/src/com/android/settings/bluetooth/BluetoothSummaryUpdater.java
+++ b/src/com/android/settings/bluetooth/BluetoothSummaryUpdater.java
@@ -42,31 +42,21 @@
private final LocalBluetoothManager mBluetoothManager;
private final LocalBluetoothAdapter mBluetoothAdapter;
- private boolean mEnabled;
- private int mConnectionState;
-
public BluetoothSummaryUpdater(Context context, OnSummaryChangeListener listener,
LocalBluetoothManager bluetoothManager) {
super(context, listener);
mBluetoothManager = bluetoothManager;
mBluetoothAdapter = mBluetoothManager != null
- ? mBluetoothManager.getBluetoothAdapter() : null;
+ ? mBluetoothManager.getBluetoothAdapter() : null;
}
@Override
public void onBluetoothStateChanged(int bluetoothState) {
- mEnabled = bluetoothState == BluetoothAdapter.STATE_ON
- || bluetoothState == BluetoothAdapter.STATE_TURNING_ON;
- if (!mEnabled) {
- mConnectionState = BluetoothAdapter.STATE_DISCONNECTED;
- }
notifyChangeIfNeeded();
}
@Override
public void onConnectionStateChanged(CachedBluetoothDevice cachedDevice, int state) {
- mConnectionState = state;
- updateConnected();
notifyChangeIfNeeded();
}
@@ -92,8 +82,6 @@
return;
}
if (listening) {
- mEnabled = mBluetoothAdapter.isEnabled();
- mConnectionState = mBluetoothAdapter.getConnectionState();
notifyChangeIfNeeded();
mBluetoothManager.getEventManager().registerCallback(this);
} else {
@@ -103,10 +91,10 @@
@Override
public String getSummary() {
- if (!mEnabled) {
+ if (mBluetoothAdapter == null || !mBluetoothAdapter.isEnabled()) {
return mContext.getString(R.string.bluetooth_disabled);
}
- switch (mConnectionState) {
+ switch (mBluetoothAdapter.getConnectionState()) {
case BluetoothAdapter.STATE_CONNECTED:
return getConnectedDeviceSummary();
case BluetoothAdapter.STATE_CONNECTING:
@@ -118,50 +106,17 @@
}
}
- private void updateConnected() {
- if (mBluetoothAdapter == null) {
- return;
- }
- // Make sure our connection state is up to date.
- int state = mBluetoothAdapter.getConnectionState();
- if (state != mConnectionState) {
- mConnectionState = state;
- return;
- }
- final Collection<CachedBluetoothDevice> devices = getDevices();
- if (devices == null) {
- mConnectionState = BluetoothAdapter.STATE_DISCONNECTED;
- return;
- }
- if (mConnectionState == BluetoothAdapter.STATE_CONNECTED) {
- CachedBluetoothDevice connectedDevice = null;
- for (CachedBluetoothDevice device : devices) {
- if (device.isConnected()) {
- connectedDevice = device;
- break;
- }
- }
- if (connectedDevice == null) {
- // If somehow we think we are connected, but have no connected devices, we
- // aren't connected.
- mConnectionState = BluetoothAdapter.STATE_DISCONNECTED;
- }
- }
- }
-
- private Collection<CachedBluetoothDevice> getDevices() {
- return mBluetoothManager != null
- ? mBluetoothManager.getCachedDeviceManager().getCachedDevicesCopy()
- : null;
- }
-
@VisibleForTesting
String getConnectedDeviceSummary() {
String deviceName = null;
int count = 0;
final Set<BluetoothDevice> devices = mBluetoothAdapter.getBondedDevices();
- if (devices == null || devices.isEmpty()) {
- return null;
+ if (devices == null) {
+ Log.e(TAG, "getConnectedDeviceSummary, bonded devices are null");
+ return mContext.getString(R.string.bluetooth_disabled);
+ } else if (devices.isEmpty()) {
+ Log.e(TAG, "getConnectedDeviceSummary, no bonded devices");
+ return mContext.getString(R.string.disconnected);
}
for (BluetoothDevice device : devices) {
if (device.isConnected()) {
@@ -173,12 +128,13 @@
}
}
if (deviceName == null) {
- Log.w(TAG, "getConnectedDeviceSummary, deviceName is null, numBondedDevices="
+ Log.e(TAG, "getConnectedDeviceSummary, deviceName is null, numBondedDevices="
+ devices.size());
for (BluetoothDevice device : devices) {
- Log.w(TAG, "getConnectedDeviceSummary, device=" + device.getName() + "["
+ Log.e(TAG, "getConnectedDeviceSummary, device=" + device.getName() + "["
+ device.getAddress() + "]" + ", isConnected=" + device.isConnected());
}
+ return mContext.getString(R.string.disconnected);
}
return count > 1 ? mContext.getString(R.string.bluetooth_connected_multiple_devices_summary)
: mContext.getString(R.string.bluetooth_connected_summary, deviceName);
diff --git a/tests/robotests/src/com/android/settings/bluetooth/BluetoothSummaryUpdaterTest.java b/tests/robotests/src/com/android/settings/bluetooth/BluetoothSummaryUpdaterTest.java
index e3f00d8..0c27412 100644
--- a/tests/robotests/src/com/android/settings/bluetooth/BluetoothSummaryUpdaterTest.java
+++ b/tests/robotests/src/com/android/settings/bluetooth/BluetoothSummaryUpdaterTest.java
@@ -18,17 +18,25 @@
import static com.google.common.truth.Truth.assertThat;
+import static org.mockito.Matchers.anyString;
+import static org.mockito.Mockito.doAnswer;
+import static org.mockito.Mockito.doCallRealMethod;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
import android.bluetooth.BluetoothAdapter;
import android.bluetooth.BluetoothDevice;
import android.content.Context;
+import android.util.Log;
import com.android.settings.R;
-import com.android.settings.testutils.SettingsRobolectricTestRunner;
import com.android.settings.TestConfig;
+import com.android.settings.testutils.SettingsRobolectricTestRunner;
import com.android.settings.widget.SummaryUpdater.OnSummaryChangeListener;
-import com.android.settingslib.bluetooth.CachedBluetoothDevice;
-import com.android.settingslib.bluetooth.LocalBluetoothManager;
import com.android.settingslib.bluetooth.LocalBluetoothAdapter;
+import com.android.settingslib.bluetooth.LocalBluetoothManager;
import org.junit.Before;
import org.junit.Test;
@@ -39,19 +47,9 @@
import org.robolectric.RuntimeEnvironment;
import org.robolectric.annotation.Config;
-import java.util.ArrayList;
import java.util.HashSet;
-import java.util.List;
import java.util.Set;
-import static org.mockito.Matchers.anyString;
-import static org.mockito.Mockito.doAnswer;
-import static org.mockito.Mockito.doReturn;
-import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.times;
-import static org.mockito.Mockito.verify;
-import static org.mockito.Mockito.when;
-
@RunWith(SettingsRobolectricTestRunner.class)
@Config(manifest = TestConfig.MANIFEST_PATH, sdk = TestConfig.SDK_VERSION)
public class BluetoothSummaryUpdaterTest {
@@ -70,16 +68,33 @@
@Mock
private SummaryListener mListener;
+ // Disabled by default
+ private final boolean[] mAdapterEnabled = {false};
+ // Not connected by default
+ private final int[] mAdapterConnectionState = {BluetoothAdapter.STATE_DISCONNECTED};
+ // Not connected by default
+ private final boolean[] mDeviceConnected = {false, false};
+ private final Set<BluetoothDevice> mBondedDevices = new HashSet<>();
private BluetoothSummaryUpdater mSummaryUpdater;
@Before
public void setUp() {
MockitoAnnotations.initMocks(this);
- when(mBluetoothManager.getBluetoothAdapter()).thenReturn(mBtAdapter);
- when(mBtAdapter.isEnabled()).thenReturn(true);
- when(mBtAdapter.getConnectionState()).thenReturn(BluetoothAdapter.STATE_CONNECTED);
mContext = RuntimeEnvironment.application.getApplicationContext();
+ doCallRealMethod().when(mListener).onSummaryChanged(anyString());
+ // Setup mock adapter
+ when(mBluetoothManager.getBluetoothAdapter()).thenReturn(mBtAdapter);
+ doAnswer(invocation -> mAdapterEnabled[0]).when(mBtAdapter).isEnabled();
+ doAnswer(invocation -> mAdapterConnectionState[0]).when(mBtAdapter).getConnectionState();
mSummaryUpdater = new BluetoothSummaryUpdater(mContext, mListener, mBluetoothManager);
+ // Setup first device
+ doReturn(DEVICE_NAME).when(mConnectedDevice).getName();
+ doAnswer(invocation -> mDeviceConnected[0]).when(mConnectedDevice).isConnected();
+ // Setup second device
+ doReturn(DEVICE_KEYBOARD_NAME).when(mConnectedKeyBoardDevice).getName();
+ doAnswer(invocation -> mDeviceConnected[1]).when(mConnectedKeyBoardDevice)
+ .isConnected();
+ doReturn(mBondedDevices).when(mBtAdapter).getBondedDevices();
}
@Test
@@ -98,7 +113,10 @@
@Test
public void register_true_shouldSendSummaryChange() {
- prepareConnectedDevice(false);
+ mAdapterEnabled[0] = true;
+ mAdapterConnectionState[0] = BluetoothAdapter.STATE_CONNECTED;
+ mBondedDevices.add(mConnectedDevice);
+ mDeviceConnected[0] = true;
mSummaryUpdater.register(true);
@@ -108,7 +126,11 @@
@Test
public void onBluetoothStateChanged_btDisabled_shouldSendDisabledSummary() {
- mSummaryUpdater.register(true);
+ // These states should be ignored
+ mAdapterConnectionState[0] = BluetoothAdapter.STATE_CONNECTED;
+ mBondedDevices.add(mConnectedDevice);
+ mDeviceConnected[0] = true;
+
mSummaryUpdater.onBluetoothStateChanged(BluetoothAdapter.STATE_OFF);
verify(mListener).onSummaryChanged(mContext.getString(R.string.bluetooth_disabled));
@@ -116,9 +138,11 @@
@Test
public void onBluetoothStateChanged_btEnabled_connected_shouldSendConnectedSummary() {
- prepareConnectedDevice(false);
+ mAdapterEnabled[0] = true;
+ mAdapterConnectionState[0] = BluetoothAdapter.STATE_CONNECTED;
+ mBondedDevices.add(mConnectedDevice);
+ mDeviceConnected[0] = true;
- mSummaryUpdater.register(true);
mSummaryUpdater.onBluetoothStateChanged(BluetoothAdapter.STATE_ON);
verify(mListener).onSummaryChanged(
@@ -126,58 +150,71 @@
}
@Test
+ public void onBluetoothStateChanged_btEnabled_connectedMisMatch_shouldSendNotConnected() {
+ mAdapterEnabled[0] = true;
+ mAdapterConnectionState[0] = BluetoothAdapter.STATE_CONNECTED;
+ mBondedDevices.add(mConnectedDevice);
+ // State mismatch
+ mDeviceConnected[0] = false;
+
+ mSummaryUpdater.onBluetoothStateChanged(BluetoothAdapter.STATE_ON);
+
+ verify(mListener).onSummaryChanged(mContext.getString(R.string.disconnected));
+ }
+
+ @Test
public void onBluetoothStateChanged_btEnabled_notConnected_shouldSendDisconnectedMessage() {
- when(mBtAdapter.getConnectionState()).thenReturn(BluetoothAdapter.STATE_DISCONNECTED);
- mSummaryUpdater.register(true);
+ mAdapterEnabled[0] = true;
+ mAdapterConnectionState[0] = BluetoothAdapter.STATE_DISCONNECTED;
+ mBondedDevices.add(mConnectedDevice);
+ // This should be ignored
+ mDeviceConnected[0] = true;
+
mSummaryUpdater.onBluetoothStateChanged(BluetoothAdapter.STATE_TURNING_ON);
- verify(mListener).onSummaryChanged(
- mContext.getString(R.string.disconnected));
+ verify(mListener).onSummaryChanged(mContext.getString(R.string.disconnected));
}
@Test
public void onBluetoothStateChanged_ConnectedDisabledEnabled_shouldSendDisconnectedSummary() {
- final boolean[] connected = {false};
- final List<CachedBluetoothDevice> devices = new ArrayList<>();
- devices.add(mock(CachedBluetoothDevice.class));
- doAnswer(invocation -> connected[0]).when(devices.get(0)).isConnected();
- when(mBluetoothManager.getCachedDeviceManager().getCachedDevicesCopy())
- .thenReturn(devices);
- when(mBtAdapter.getConnectionState()).thenReturn(BluetoothAdapter.STATE_DISCONNECTED);
- prepareConnectedDevice(false);
+ mAdapterEnabled[0] = true;
+ mAdapterConnectionState[0] = BluetoothAdapter.STATE_DISCONNECTED;
+ mBondedDevices.add(mConnectedDevice);
+ mDeviceConnected[0] = false;
mSummaryUpdater.register(true);
verify(mListener).onSummaryChanged(mContext.getString(R.string.disconnected));
- connected[0] = true;
- when(mBtAdapter.getConnectionState()).thenReturn(BluetoothAdapter.STATE_CONNECTED);
+ mAdapterConnectionState[0] = BluetoothAdapter.STATE_CONNECTED;
+ mDeviceConnected[0] = true;
mSummaryUpdater.onConnectionStateChanged(null /* device */,
BluetoothAdapter.STATE_CONNECTED);
verify(mListener).onSummaryChanged(
mContext.getString(R.string.bluetooth_connected_summary, DEVICE_NAME));
+ mAdapterEnabled[0] = false;
mSummaryUpdater.onBluetoothStateChanged(BluetoothAdapter.STATE_OFF);
verify(mListener).onSummaryChanged(mContext.getString(R.string.bluetooth_disabled));
- connected[0] = false;
+ // Turning ON means not enabled
mSummaryUpdater.onBluetoothStateChanged(BluetoothAdapter.STATE_TURNING_ON);
+ // There should still be only one invocation of disabled message
+ verify(mListener).onSummaryChanged(mContext.getString(R.string.bluetooth_disabled));
+
+ mAdapterEnabled[0] = true;
+ mDeviceConnected[0] = false;
+ mSummaryUpdater.onBluetoothStateChanged(BluetoothAdapter.STATE_ON);
verify(mListener, times(2)).onSummaryChanged(mContext.getString(R.string.disconnected));
verify(mListener, times(4)).onSummaryChanged(anyString());
}
@Test
public void onConnectionStateChanged_connected_shouldSendConnectedMessage() {
- final List<CachedBluetoothDevice> devices = new ArrayList<>();
- devices.add(mock(CachedBluetoothDevice.class));
- when(devices.get(0).isConnected()).thenReturn(true);
- when(mBluetoothManager.getCachedDeviceManager().getCachedDevicesCopy())
- .thenReturn(devices);
- when(mBtAdapter.getConnectionState()).thenReturn(BluetoothAdapter.STATE_DISCONNECTED);
- prepareConnectedDevice(false);
+ mAdapterEnabled[0] = true;
+ mAdapterConnectionState[0] = BluetoothAdapter.STATE_CONNECTED;
+ mBondedDevices.add(mConnectedDevice);
+ mDeviceConnected[0] = true;
- mSummaryUpdater.register(true);
-
- when(mBtAdapter.getConnectionState()).thenReturn(BluetoothAdapter.STATE_CONNECTED);
mSummaryUpdater.onConnectionStateChanged(null /* device */,
BluetoothAdapter.STATE_CONNECTED);
@@ -187,7 +224,22 @@
@Test
public void onConnectionStateChanged_inconsistentState_shouldSendDisconnectedMessage() {
- mSummaryUpdater.register(true);
+ mAdapterEnabled[0] = true;
+ mAdapterConnectionState[0] = BluetoothAdapter.STATE_DISCONNECTED;
+ mBondedDevices.add(mConnectedDevice);
+ mDeviceConnected[0] = false;
+
+ mSummaryUpdater.onConnectionStateChanged(null /* device */,
+ BluetoothAdapter.STATE_CONNECTED);
+
+ verify(mListener).onSummaryChanged(mContext.getString(R.string.disconnected));
+ }
+
+ @Test
+ public void onConnectionStateChanged_noBondedDevice_shouldSendDisconnectedMessage() {
+ mAdapterEnabled[0] = true;
+ mAdapterConnectionState[0] = BluetoothAdapter.STATE_CONNECTED;
+
mSummaryUpdater.onConnectionStateChanged(null /* device */,
BluetoothAdapter.STATE_CONNECTED);
@@ -197,8 +249,10 @@
@Test
public void onConnectionStateChanged_connecting_shouldSendConnectingMessage() {
- mSummaryUpdater.register(true);
- when(mBtAdapter.getConnectionState()).thenReturn(BluetoothAdapter.STATE_CONNECTING);
+ // No need for bonded devices
+ mAdapterEnabled[0] = true;
+ mAdapterConnectionState[0] = BluetoothAdapter.STATE_CONNECTING;
+
mSummaryUpdater.onConnectionStateChanged(null /* device */,
BluetoothAdapter.STATE_CONNECTING);
@@ -207,8 +261,10 @@
@Test
public void onConnectionStateChanged_disconnecting_shouldSendDisconnectingMessage() {
- mSummaryUpdater.register(true);
- when(mBtAdapter.getConnectionState()).thenReturn(BluetoothAdapter.STATE_DISCONNECTING);
+ // No need for bonded devices
+ mAdapterEnabled[0] = true;
+ mAdapterConnectionState[0] = BluetoothAdapter.STATE_DISCONNECTING;
+
mSummaryUpdater.onConnectionStateChanged(null /* device */,
BluetoothAdapter.STATE_DISCONNECTING);
@@ -217,7 +273,8 @@
@Test
public void getConnectedDeviceSummary_hasConnectedDevice_returnOneDeviceSummary() {
- prepareConnectedDevice(false);
+ mBondedDevices.add(mConnectedDevice);
+ mDeviceConnected[0] = true;
final String expectedSummary = mContext.getString(R.string.bluetooth_connected_summary,
DEVICE_NAME);
@@ -226,28 +283,16 @@
@Test
public void getConnectedDeviceSummary_multipleDevices_returnMultipleDevicesSummary() {
- prepareConnectedDevice(true);
+ mBondedDevices.add(mConnectedDevice);
+ mBondedDevices.add(mConnectedKeyBoardDevice);
+ mDeviceConnected[0] = true;
+ mDeviceConnected[1] = true;
final String expectedSummary = mContext.getString(
R.string.bluetooth_connected_multiple_devices_summary);
assertThat(mSummaryUpdater.getConnectedDeviceSummary()).isEqualTo(expectedSummary);
}
- private void prepareConnectedDevice(boolean multipleDevices) {
- final Set<BluetoothDevice> devices = new HashSet<>();
- doReturn(DEVICE_NAME).when(mConnectedDevice).getName();
- doReturn(true).when(mConnectedDevice).isConnected();
- devices.add(mConnectedDevice);
- if (multipleDevices) {
- // Add one more device if we need to test multiple devices
- doReturn(DEVICE_KEYBOARD_NAME).when(mConnectedKeyBoardDevice).getName();
- doReturn(true).when(mConnectedKeyBoardDevice).isConnected();
- devices.add(mConnectedKeyBoardDevice);
- }
-
- doReturn(devices).when(mBtAdapter).getBondedDevices();
- }
-
private class SummaryListener implements OnSummaryChangeListener {
String summary;