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

src/com/android/settings/bluetooth/BluetoothSummaryUpdater.java

@@ -42,9 +42,6 @@ public final class BluetoothSummaryUpdater extends SummaryUpdater implements Blu
     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);
@@ -55,18 +52,11 @@ public final class BluetoothSummaryUpdater extends SummaryUpdater implements Blu
 
     @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 @@ public final class BluetoothSummaryUpdater extends SummaryUpdater implements Blu
             return;
         }
         if (listening) {
-            mEnabled = mBluetoothAdapter.isEnabled();
-            mConnectionState = mBluetoothAdapter.getConnectionState();
             notifyChangeIfNeeded();
             mBluetoothManager.getEventManager().registerCallback(this);
         } else {
@@ -103,10 +91,10 @@ public final class BluetoothSummaryUpdater extends SummaryUpdater implements Blu
 
     @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 @@ public final class BluetoothSummaryUpdater extends SummaryUpdater implements Blu
         }
     }
 
-    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()) {
+        if (devices == null) {
-            return 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 @@ public final class BluetoothSummaryUpdater extends SummaryUpdater implements Blu
             }
         }
         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);

tests/robotests/src/com/android/settings/bluetooth/BluetoothSummaryUpdaterTest.java

@@ -18,17 +18,25 @@ package com.android.settings.bluetooth;
 
 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.mockito.MockitoAnnotations;
 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 @@ public class BluetoothSummaryUpdaterTest {
     @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 @@ public class BluetoothSummaryUpdaterTest {
 
     @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 @@ public class BluetoothSummaryUpdaterTest {
 
     @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,68 +138,83 @@ public class BluetoothSummaryUpdaterTest {
 
     @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(
                 mContext.getString(R.string.bluetooth_connected_summary, DEVICE_NAME));
     }
 
+    @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);
+        mAdapterEnabled[0] = true;
-        mSummaryUpdater.register(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(
+        verify(mListener).onSummaryChanged(mContext.getString(R.string.disconnected));
-                mContext.getString(R.string.disconnected));
     }
 
     @Test
     public void onBluetoothStateChanged_ConnectedDisabledEnabled_shouldSendDisconnectedSummary() {
-        final boolean[] connected = {false};
+        mAdapterEnabled[0] = true;
-        final List<CachedBluetoothDevice> devices = new ArrayList<>();
+        mAdapterConnectionState[0] = BluetoothAdapter.STATE_DISCONNECTED;
-        devices.add(mock(CachedBluetoothDevice.class));
+        mBondedDevices.add(mConnectedDevice);
-        doAnswer(invocation -> connected[0]).when(devices.get(0)).isConnected();
+        mDeviceConnected[0] = false;
-        when(mBluetoothManager.getCachedDeviceManager().getCachedDevicesCopy())
-                .thenReturn(devices);
-        when(mBtAdapter.getConnectionState()).thenReturn(BluetoothAdapter.STATE_DISCONNECTED);
-        prepareConnectedDevice(false);
 
         mSummaryUpdater.register(true);
         verify(mListener).onSummaryChanged(mContext.getString(R.string.disconnected));
 
-        connected[0] = true;
+        mAdapterConnectionState[0] = BluetoothAdapter.STATE_CONNECTED;
-        when(mBtAdapter.getConnectionState()).thenReturn(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<>();
+        mAdapterEnabled[0] = true;
-        devices.add(mock(CachedBluetoothDevice.class));
+        mAdapterConnectionState[0] = BluetoothAdapter.STATE_CONNECTED;
-        when(devices.get(0).isConnected()).thenReturn(true);
+        mBondedDevices.add(mConnectedDevice);
-        when(mBluetoothManager.getCachedDeviceManager().getCachedDevicesCopy())
+        mDeviceConnected[0] = true;
-                .thenReturn(devices);
-        when(mBtAdapter.getConnectionState()).thenReturn(BluetoothAdapter.STATE_DISCONNECTED);
-        prepareConnectedDevice(false);
 
-        mSummaryUpdater.register(true);
-
-        when(mBtAdapter.getConnectionState()).thenReturn(BluetoothAdapter.STATE_CONNECTED);
         mSummaryUpdater.onConnectionStateChanged(null /* device */,
                 BluetoothAdapter.STATE_CONNECTED);
 
@@ -187,7 +224,22 @@ public class BluetoothSummaryUpdaterTest {
 
     @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 @@ public class BluetoothSummaryUpdaterTest {
 
     @Test
     public void onConnectionStateChanged_connecting_shouldSendConnectingMessage() {
-        mSummaryUpdater.register(true);
+        // No need for bonded devices
-        when(mBtAdapter.getConnectionState()).thenReturn(BluetoothAdapter.STATE_CONNECTING);
+        mAdapterEnabled[0] = true;
+        mAdapterConnectionState[0] = BluetoothAdapter.STATE_CONNECTING;
+
         mSummaryUpdater.onConnectionStateChanged(null /* device */,
                 BluetoothAdapter.STATE_CONNECTING);
 
@@ -207,8 +261,10 @@ public class BluetoothSummaryUpdaterTest {
 
     @Test
     public void onConnectionStateChanged_disconnecting_shouldSendDisconnectingMessage() {
-        mSummaryUpdater.register(true);
+        // No need for bonded devices
-        when(mBtAdapter.getConnectionState()).thenReturn(BluetoothAdapter.STATE_DISCONNECTING);
+        mAdapterEnabled[0] = true;
+        mAdapterConnectionState[0] = BluetoothAdapter.STATE_DISCONNECTING;
+
         mSummaryUpdater.onConnectionStateChanged(null /* device */,
                 BluetoothAdapter.STATE_DISCONNECTING);
 
@@ -217,7 +273,8 @@ public class BluetoothSummaryUpdaterTest {
 
     @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 @@ public class BluetoothSummaryUpdaterTest {
 
     @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;