Make DeviceComboBoxDeviceTracker More Robust

Do not depend on `adbSession.hostServices.trackDevices()` to emit an entry with a device being explicitly OFFLINE.

Instead, use the absence of a device as an indication that it has disconnected.

This has a few benefits:
1. It's more robust. Since there is no spec to the ADB track-devices command, we cannot assume that an entry with an explicit OFFLINE is guaranteed to appear.
2. It results in shorter more readable code.
3. It should allow us to use the fake adb server which doesn't emit all the events in track-devices that the real adb does.

Bug: n/a
Test: Updated
Change-Id: Ia25216e12bea4b8a12d5c5d27c2dca0c32dc80e6

2 files changed, 35 insertions, 60 deletions

diff --git a/logcat/src/com/android/tools/idea/logcat/devices/DeviceComboBoxDeviceTracker.kt b/logcat/src/com/android/tools/idea/logcat/devices/DeviceComboBoxDeviceTracker.kt
index 44b12bd6c87..1289ef49a76 100644
--- a/logcat/src/com/android/tools/idea/logcat/devices/DeviceComboBoxDeviceTracker.kt
+++ b/logcat/src/com/android/tools/idea/logcat/devices/DeviceComboBoxDeviceTracker.kt
@@ -80,7 +80,8 @@ internal class DeviceComboBoxDeviceTracker(
 
     // Initialize state by reading all current devices
     coroutineScope {
-      adbSession.hostServices.devices().filter { it.isOnline() }.map { async { it.toDevice() } }.awaitAll().forEach {
+      val devices = adbSession.hostServices.devices()
+      devices.filter { it.isOnline() }.map { async { it.toDevice() } }.awaitAll().forEach {
         onlineDevicesBySerial[it.serialNumber] = it
         allDevicesById[it.deviceId] = it
         emit(Added(it))
@@ -89,60 +90,40 @@ internal class DeviceComboBoxDeviceTracker(
 
     // Add the preexisting device.
     if (preexistingDevice != null && !allDevicesById.containsKey(preexistingDevice.deviceId)) {
-      onlineDevicesBySerial[preexistingDevice.serialNumber] = preexistingDevice
       allDevicesById[preexistingDevice.deviceId] = preexistingDevice
       emit(Added(preexistingDevice))
     }
 
     // Track devices changes:
-    // There are 3 distinct cases:
-    // 1. A device that has not been seen before comes online -> callback.deviceAdded()
-    // 2. A device that was seen before and is now offline comes online -> callback.deviceStateChanged()
-    // 3. A device that is currently online goes offline -> callback.deviceStateChanged()
+    // We only care about devices that are online.
+    // If a previously unknown device comes online, we emit Added
+    // If a previously known device comes online, we emit StateChanged
+    // If previously online device is missing from the kist, we emit a StateChanged.
     adbSession.hostServices.trackDevices().collect { deviceList ->
-      for (deviceInfo in deviceList) {
-        val serialNumber = deviceInfo.serialNumber
-        val isOnline = deviceInfo.isOnline()
-        if (isOnline) {
-          if (onlineDevicesBySerial.containsKey(serialNumber)) {
-            continue
-          }
-
-          val deviceId = deviceInfo.getDeviceId()
-          val existingDevice = allDevicesById[deviceId]
-          if (existingDevice != null) {
-            val copy = if (existingDevice.isEmulator) {
-              existingDevice.copy(isOnline = true, serialNumber = serialNumber)
-            }
-            else {
-              val properties = deviceInfo.getProperties(RO_BUILD_VERSION_RELEASE, RO_BUILD_VERSION_SDK)
-              existingDevice.copy(
-                isOnline = true,
-                release = properties.getValue(RO_BUILD_VERSION_RELEASE).toIntOrNull() ?: 0,
-                sdk = properties.getValue(RO_BUILD_VERSION_SDK).toIntOrNull() ?: 0)
-
-            }
-            onlineDevicesBySerial[serialNumber] = copy
-            allDevicesById[serialNumber] = copy
-            emit(StateChanged(copy))
+      val devices = deviceList.entries.filter { it.isOnline() }.associateBy { it.serialNumber }
+      devices.values.forEach {
+        val serialNumber = it.serialNumber
+        if (!onlineDevicesBySerial.containsKey(serialNumber)) {
+          val device = it.toDevice()
+          if (allDevicesById.containsKey(device.deviceId)) {
+            emit(StateChanged(device))
           }
           else {
-            val newDevice = deviceInfo.toDevice()
-            allDevicesById[deviceId] = newDevice
-            onlineDevicesBySerial[serialNumber] = newDevice
-            emit(Added(newDevice))
-          }
-        }
-        else {
-          val device = onlineDevicesBySerial[serialNumber]
-          if (device != null) {
-            val copy = device.copy(isOnline = false)
-            onlineDevicesBySerial.remove(serialNumber)
-            allDevicesById[device.serialNumber] = copy
-            emit(StateChanged(copy))
+            emit(Added(device))
           }
+          onlineDevicesBySerial[serialNumber] = device
+          allDevicesById[device.deviceId] = device
         }
       }
+
+      // Find devices that were online and are not anymore, then remove them.
+      onlineDevicesBySerial.keys.filter { !devices.containsKey(it) }.forEach {
+        val device = onlineDevicesBySerial[it] ?: return@forEach
+        val deviceOffline = device.copy(isOnline = false)
+        emit(StateChanged(deviceOffline))
+        onlineDevicesBySerial.remove(it)
+        allDevicesById[device.deviceId] = deviceOffline
+      }
     }
   }
 
@@ -174,13 +155,7 @@ internal class DeviceComboBoxDeviceTracker(
       serialNumber
     }
 
-  private suspend fun DeviceInfo.getDeviceId(): String {
-    return when {
-      serialNumber.startsWith("emulator-") -> getAvdName(getProperties(RO_BOOT_QEMU_AVD_NAME, RO_KERNEL_QEMU_AVD_NAME))
-      else -> serialNumber
-    }
-  }
-
+  @Suppress("SameParameterValue") // The inspection is wrong. It only considers the first arg in the vararg
   private suspend fun DeviceInfo.getProperties(vararg properties: String): Map<String, String> {
     val selector = DeviceSelector.fromSerialNumber(serialNumber)
     val command = properties.joinToString(" ; ") { "getprop $it" }
diff --git a/logcat/testSrc/com/android/tools/idea/logcat/devices/DeviceComboBoxDeviceTrackerTest.kt b/logcat/testSrc/com/android/tools/idea/logcat/devices/DeviceComboBoxDeviceTrackerTest.kt
index ab4bd778b60..4ba145ca90b 100644
--- a/logcat/testSrc/com/android/tools/idea/logcat/devices/DeviceComboBoxDeviceTrackerTest.kt
+++ b/logcat/testSrc/com/android/tools/idea/logcat/devices/DeviceComboBoxDeviceTrackerTest.kt
@@ -87,10 +87,9 @@ class DeviceComboBoxDeviceTrackerTest {
     val events = async { deviceTracker.trackDevices().toList() }
 
     hostServices.use {
-      it.sendDevices(
-        emulator,
-        emulator.withState(OFFLINE),
-      )
+      it.sendDevices(emulator)
+      it.sendDevices(emulator.withState(OFFLINE))
+      it.sendDevices()
     }
 
     assertThat(events.await()).containsExactly(
@@ -110,10 +109,9 @@ class DeviceComboBoxDeviceTrackerTest {
     val events = async { deviceTracker.trackDevices().toList() }
 
     hostServices.use {
-      it.sendDevices(
-        emulator,
-        emulator.withState(OFFLINE),
-      )
+      it.sendDevices(emulator)
+      it.sendDevices(emulator.withState(OFFLINE))
+      it.sendDevices()
     }
 
     assertThat(events.await()).containsExactly(
@@ -173,7 +171,7 @@ class DeviceComboBoxDeviceTrackerTest {
 
     hostServices.use {
       it.sendDevices(device1, device2)
-      it.sendDevices(emulator1)
+      it.sendDevices(device1, device2, emulator1)
     }
 
     assertThat(events.await()).containsExactly(
@@ -259,6 +257,7 @@ class DeviceComboBoxDeviceTrackerTest {
     hostServices.use {
       it.sendDevices(device1.withState(ONLINE))
       it.sendDevices(device1.withState(OFFLINE))
+      it.sendDevices()
       it.sendDevices(device1.withState(ONLINE))
     }
 
@@ -278,6 +277,7 @@ class DeviceComboBoxDeviceTrackerTest {
     hostServices.use {
       it.sendDevices(emulator1.withState(ONLINE).withSerialNumber("emulator-1"))
       it.sendDevices(emulator1.withState(OFFLINE))
+      it.sendDevices()
       it.sendDevices(emulator1.withState(ONLINE).withSerialNumber("emulator-2"))
     }