From ac040e3b1fc3488155e5bb337908c491627900bc Mon Sep 17 00:00:00 2001 From: Jack He Date: Tue, 12 Sep 2017 16:26:06 -0700 Subject: [PATCH] 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 --- .../bluetooth/BluetoothSummaryUpdater.java | 68 ++----- .../BluetoothSummaryUpdaterTest.java | 179 +++++++++++------- 2 files changed, 124 insertions(+), 123 deletions(-) diff --git a/src/com/android/settings/bluetooth/BluetoothSummaryUpdater.java b/src/com/android/settings/bluetooth/BluetoothSummaryUpdater.java index 7d2cc181897..662cd701de2 100644 --- a/src/com/android/settings/bluetooth/BluetoothSummaryUpdater.java +++ b/src/com/android/settings/bluetooth/BluetoothSummaryUpdater.java @@ -42,31 +42,21 @@ 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); 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 @@ 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 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 getDevices() { - return mBluetoothManager != null - ? mBluetoothManager.getCachedDeviceManager().getCachedDevicesCopy() - : null; - } - @VisibleForTesting String getConnectedDeviceSummary() { String deviceName = null; int count = 0; final Set 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 @@ 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); diff --git a/tests/robotests/src/com/android/settings/bluetooth/BluetoothSummaryUpdaterTest.java b/tests/robotests/src/com/android/settings/bluetooth/BluetoothSummaryUpdaterTest.java index e3f00d807b6..0c27412c551 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 @@ 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 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); - 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 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 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 @@ 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); - 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 @@ public class BluetoothSummaryUpdaterTest { @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 @@ 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 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;