Merge "Small fixes to DumpManager" into udc-dev am: 88debc70e1 am: 7bc572e338 am: 9303aaa615

Original change: https://googleplex-android-review.googlesource.com/c/platform/frameworks/base/+/22932416

Change-Id: I09c837ee17afc73949fe99113b34db2f1594e886
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>

2 files changed, 169 insertions, 55 deletions

diff --git a/packages/SystemUI/src/com/android/systemui/dump/DumpManager.kt b/packages/SystemUI/src/com/android/systemui/dump/DumpManager.kt
index 7d1ffca88d9f..ab8052c54f92 100644
--- a/packages/SystemUI/src/com/android/systemui/dump/DumpManager.kt
+++ b/packages/SystemUI/src/com/android/systemui/dump/DumpManager.kt
@@ -16,12 +16,12 @@
 
 package com.android.systemui.dump
 
-import android.util.ArrayMap
 import com.android.systemui.Dumpable
 import com.android.systemui.ProtoDumpable
 import com.android.systemui.dump.nano.SystemUIProtoDump
 import com.android.systemui.plugins.log.LogBuffer
 import java.io.PrintWriter
+import java.util.TreeMap
 import javax.inject.Inject
 import javax.inject.Singleton
 
@@ -36,8 +36,9 @@ import javax.inject.Singleton
  */
 @Singleton
 open class DumpManager @Inject constructor() {
-    private val dumpables: MutableMap<String, RegisteredDumpable<Dumpable>> = ArrayMap()
-    private val buffers: MutableMap<String, RegisteredDumpable<LogBuffer>> = ArrayMap()
+    // NOTE: Using TreeMap ensures that iteration is in a predictable & alphabetical order.
+    private val dumpables: MutableMap<String, RegisteredDumpable<Dumpable>> = TreeMap()
+    private val buffers: MutableMap<String, RegisteredDumpable<LogBuffer>> = TreeMap()
 
     /** See [registerCriticalDumpable]. */
     fun registerCriticalDumpable(module: Dumpable) {
@@ -132,7 +133,8 @@ open class DumpManager @Inject constructor() {
     }
 
     /**
-     * Dumps the first dumpable or buffer whose registered name ends with [target]
+     * Dumps the alphabetically first, shortest-named dumpable or buffer whose registered name ends
+     * with [target].
      */
     @Synchronized
     fun dumpTarget(
@@ -141,19 +143,14 @@ open class DumpManager @Inject constructor() {
         args: Array<String>,
         tailLength: Int,
     ) {
-        for (dumpable in dumpables.values) {
-            if (dumpable.name.endsWith(target)) {
-                dumpDumpable(dumpable, pw, args)
-                return
+        sequence {
+            findBestTargetMatch(dumpables, target)?.let {
+                yield(it.name to { dumpDumpable(it, pw, args) })
             }
-        }
-
-        for (buffer in buffers.values) {
-            if (buffer.name.endsWith(target)) {
-                dumpBuffer(buffer, pw, tailLength)
-                return
+            findBestTargetMatch(buffers, target)?.let {
+                yield(it.name to { dumpBuffer(it, pw, tailLength) })
             }
-        }
+        }.sortedBy { it.first }.minByOrNull { it.first.length }?.second?.invoke()
     }
 
     @Synchronized
@@ -162,11 +159,8 @@ open class DumpManager @Inject constructor() {
         protoDump: SystemUIProtoDump,
         args: Array<String>
     ) {
-        for (dumpable in dumpables.values) {
-            if (dumpable.dumpable is ProtoDumpable && dumpable.name.endsWith(target)) {
-                dumpProtoDumpable(dumpable.dumpable, protoDump, args)
-                return
-            }
+        findBestProtoTargetMatch(dumpables, target)?.let {
+            dumpProtoDumpable(it, protoDump, args)
         }
     }
 
@@ -303,6 +297,22 @@ open class DumpManager @Inject constructor() {
         val existingDumpable = dumpables[name]?.dumpable ?: buffers[name]?.dumpable
         return existingDumpable == null || newDumpable == existingDumpable
     }
+
+    private fun <V : Any> findBestTargetMatch(map: Map<String, V>, target: String): V? = map
+        .asSequence()
+        .filter { it.key.endsWith(target) }
+        .minByOrNull { it.key.length }
+        ?.value
+
+    private fun findBestProtoTargetMatch(
+        map: Map<String, RegisteredDumpable<Dumpable>>,
+        target: String
+    ): ProtoDumpable? = map
+        .asSequence()
+        .filter { it.key.endsWith(target) }
+        .filter { it.value.dumpable is ProtoDumpable }
+        .minByOrNull { it.key.length }
+        ?.value?.dumpable as? ProtoDumpable
 }
 
 private data class RegisteredDumpable<T>(
diff --git a/packages/SystemUI/tests/src/com/android/systemui/dump/DumpManagerTest.kt b/packages/SystemUI/tests/src/com/android/systemui/dump/DumpManagerTest.kt
index 0c5a74cd39fa..55826141739f 100644
--- a/packages/SystemUI/tests/src/com/android/systemui/dump/DumpManagerTest.kt
+++ b/packages/SystemUI/tests/src/com/android/systemui/dump/DumpManagerTest.kt
@@ -60,16 +60,14 @@ class DumpManagerTest : SysuiTestCase() {
 
         // WHEN a dumpable is dumped explicitly
         val args = arrayOf<String>()
-        dumpManager.dumpTarget("dumpable2", pw, arrayOf(), tailLength = 0)
+        dumpManager.dumpTarget("dumpable2", pw, args, tailLength = 0)
 
         // THEN only the requested one has their dump() method called
-        verify(dumpable1, never())
-            .dump(any(PrintWriter::class.java), any(Array<String>::class.java))
+        verify(dumpable1, never()).dump(any(), any())
         verify(dumpable2).dump(pw, args)
-        verify(dumpable3, never())
-            .dump(any(PrintWriter::class.java), any(Array<String>::class.java))
-        verify(buffer1, never()).dump(any(PrintWriter::class.java), anyInt())
-        verify(buffer2, never()).dump(any(PrintWriter::class.java), anyInt())
+        verify(dumpable3, never()).dump(any(), any())
+        verify(buffer1, never()).dump(any(), anyInt())
+        verify(buffer2, never()).dump(any(), anyInt())
     }
 
     @Test
@@ -82,17 +80,15 @@ class DumpManagerTest : SysuiTestCase() {
         dumpManager.registerBuffer("buffer2", buffer2)
 
         // WHEN a buffer is dumped explicitly
-        dumpManager.dumpTarget("buffer1", pw, arrayOf(), tailLength = 14)
+        val args = arrayOf<String>()
+        dumpManager.dumpTarget("buffer1", pw, args, tailLength = 14)
 
         // THEN only the requested one has their dump() method called
-        verify(dumpable1, never())
-            .dump(any(PrintWriter::class.java), any(Array<String>::class.java))
-        verify(dumpable2, never())
-            .dump(any(PrintWriter::class.java), any(Array<String>::class.java))
-        verify(dumpable2, never())
-            .dump(any(PrintWriter::class.java), any(Array<String>::class.java))
+        verify(dumpable1, never()).dump(any(), any())
+        verify(dumpable2, never()).dump(any(), any())
+        verify(dumpable3, never()).dump(any(), any())
         verify(buffer1).dump(pw, tailLength = 14)
-        verify(buffer2, never()).dump(any(PrintWriter::class.java), anyInt())
+        verify(buffer2, never()).dump(any(), anyInt())
     }
 
     @Test
@@ -109,6 +105,122 @@ class DumpManagerTest : SysuiTestCase() {
     }
 
     @Test
+    fun testDumpTarget_selectsShortestNamedDumpable() {
+        // GIVEN a variety of registered dumpables and buffers
+        dumpManager.registerCriticalDumpable("first-dumpable", dumpable1)
+        dumpManager.registerCriticalDumpable("scnd-dumpable", dumpable2)
+        dumpManager.registerCriticalDumpable("third-dumpable", dumpable3)
+
+        // WHEN a dumpable is dumped by a suffix that matches multiple options
+        val args = arrayOf<String>()
+        dumpManager.dumpTarget("dumpable", pw, args, tailLength = 0)
+
+        // THEN the matching dumpable with the shorter name is dumped
+        verify(dumpable1, never()).dump(any(), any())
+        verify(dumpable2).dump(pw, args)
+        verify(dumpable3, never()).dump(any(), any())
+    }
+
+    @Test
+    fun testDumpTarget_selectsShortestNamedBuffer() {
+        // GIVEN a variety of registered dumpables and buffers
+        dumpManager.registerBuffer("first-buffer", buffer1)
+        dumpManager.registerBuffer("scnd-buffer", buffer2)
+
+        // WHEN a dumpable is dumped by a suffix that matches multiple options
+        val args = arrayOf<String>()
+        dumpManager.dumpTarget("buffer", pw, args, tailLength = 14)
+
+        // THEN the matching buffer with the shorter name is dumped
+        verify(buffer1, never()).dump(any(), anyInt())
+        verify(buffer2).dump(pw, tailLength = 14)
+    }
+
+    @Test
+    fun testDumpTarget_selectsShortestNamedMatch_dumpable() {
+        // GIVEN a variety of registered dumpables and buffers
+        dumpManager.registerCriticalDumpable("dumpable1", dumpable1)
+        dumpManager.registerCriticalDumpable("dumpable2", dumpable2)
+        dumpManager.registerCriticalDumpable("dumpable3", dumpable3)
+        dumpManager.registerBuffer("big-buffer1", buffer1)
+        dumpManager.registerBuffer("big-buffer2", buffer2)
+
+        // WHEN a dumpable is dumped by a suffix that matches multiple options
+        val args = arrayOf<String>()
+        dumpManager.dumpTarget("2", pw, args, tailLength = 14)
+
+        // THEN the matching dumpable with the shorter name is dumped
+        verify(dumpable1, never()).dump(any(), any())
+        verify(dumpable2).dump(pw, args)
+        verify(dumpable3, never()).dump(any(), any())
+        verify(buffer1, never()).dump(any(), anyInt())
+        verify(buffer2, never()).dump(any(), anyInt())
+    }
+
+    @Test
+    fun testDumpTarget_selectsShortestNamedMatch_buffer() {
+        // GIVEN a variety of registered dumpables and buffers
+        dumpManager.registerCriticalDumpable("dumpable1", dumpable1)
+        dumpManager.registerCriticalDumpable("dumpable2", dumpable2)
+        dumpManager.registerCriticalDumpable("dumpable3", dumpable3)
+        dumpManager.registerBuffer("buffer1", buffer1)
+        dumpManager.registerBuffer("buffer2", buffer2)
+
+        // WHEN a dumpable is dumped by a suffix that matches multiple options
+        val args = arrayOf<String>()
+        dumpManager.dumpTarget("2", pw, args, tailLength = 14)
+
+        // THEN the matching buffer with the shorter name is dumped
+        verify(dumpable1, never()).dump(any(), any())
+        verify(dumpable2, never()).dump(any(), any())
+        verify(dumpable3, never()).dump(any(), any())
+        verify(buffer1, never()).dump(any(), anyInt())
+        verify(buffer2).dump(pw, tailLength = 14)
+    }
+
+    @Test
+    fun testDumpTarget_selectsTheAlphabeticallyFirstShortestMatch_dumpable() {
+        // GIVEN a variety of registered dumpables and buffers
+        dumpManager.registerCriticalDumpable("d1x", dumpable1)
+        dumpManager.registerCriticalDumpable("d2x", dumpable2)
+        dumpManager.registerCriticalDumpable("a3x", dumpable3)
+        dumpManager.registerBuffer("ab1x", buffer1)
+        dumpManager.registerBuffer("b2x", buffer2)
+
+        // WHEN a dumpable is dumped by a suffix that matches multiple options
+        val args = arrayOf<String>()
+        dumpManager.dumpTarget("x", pw, args, tailLength = 14)
+
+        // THEN the alphabetically first dumpable/buffer (of the 3 letter names) is dumped
+        verify(dumpable1, never()).dump(any(), any())
+        verify(dumpable2, never()).dump(any(), any())
+        verify(dumpable3).dump(pw, args)
+        verify(buffer1, never()).dump(any(), anyInt())
+        verify(buffer2, never()).dump(any(), anyInt())
+    }
+
+    @Test
+    fun testDumpTarget_selectsTheAlphabeticallyFirstShortestMatch_buffer() {
+        // GIVEN a variety of registered dumpables and buffers
+        dumpManager.registerCriticalDumpable("d1x", dumpable1)
+        dumpManager.registerCriticalDumpable("d2x", dumpable2)
+        dumpManager.registerCriticalDumpable("az1x", dumpable3)
+        dumpManager.registerBuffer("b1x", buffer1)
+        dumpManager.registerBuffer("b2x", buffer2)
+
+        // WHEN a dumpable is dumped by a suffix that matches multiple options
+        val args = arrayOf<String>()
+        dumpManager.dumpTarget("x", pw, args, tailLength = 14)
+
+        // THEN the alphabetically first dumpable/buffer (of the 3 letter names) is dumped
+        verify(dumpable1, never()).dump(any(), any())
+        verify(dumpable2, never()).dump(any(), any())
+        verify(dumpable3, never()).dump(any(), any())
+        verify(buffer1).dump(pw, tailLength = 14)
+        verify(buffer2, never()).dump(any(), anyInt())
+    }
+
+    @Test
     fun testDumpDumpables() {
         // GIVEN a variety of registered dumpables and buffers
         dumpManager.registerCriticalDumpable("dumpable1", dumpable1)
@@ -125,8 +237,8 @@ class DumpManagerTest : SysuiTestCase() {
         verify(dumpable1).dump(pw, args)
         verify(dumpable2).dump(pw, args)
         verify(dumpable3).dump(pw, args)
-        verify(buffer1, never()).dump(any(PrintWriter::class.java), anyInt())
-        verify(buffer2, never()).dump(any(PrintWriter::class.java), anyInt())
+        verify(buffer1, never()).dump(any(), anyInt())
+        verify(buffer2, never()).dump(any(), anyInt())
     }
 
     @Test
@@ -142,12 +254,9 @@ class DumpManagerTest : SysuiTestCase() {
         dumpManager.dumpBuffers(pw, tailLength = 1)
 
         // THEN all buffers are dumped (and no dumpables)
-        verify(dumpable1, never())
-            .dump(any(PrintWriter::class.java), any(Array<String>::class.java))
-        verify(dumpable2, never())
-            .dump(any(PrintWriter::class.java), any(Array<String>::class.java))
-        verify(dumpable3, never())
-            .dump(any(PrintWriter::class.java), any(Array<String>::class.java))
+        verify(dumpable1, never()).dump(any(), any())
+        verify(dumpable2, never()).dump(any(), any())
+        verify(dumpable3, never()).dump(any(), any())
         verify(buffer1).dump(pw, tailLength = 1)
         verify(buffer2).dump(pw, tailLength = 1)
     }
@@ -168,10 +277,9 @@ class DumpManagerTest : SysuiTestCase() {
         // THEN only critical modules are dumped (and no buffers)
         verify(dumpable1).dump(pw, args)
         verify(dumpable2).dump(pw, args)
-        verify(dumpable3, never())
-            .dump(any(PrintWriter::class.java), any(Array<String>::class.java))
-        verify(buffer1, never()).dump(any(PrintWriter::class.java), anyInt())
-        verify(buffer2, never()).dump(any(PrintWriter::class.java), anyInt())
+        verify(dumpable3, never()).dump(any(), any())
+        verify(buffer1, never()).dump(any(), anyInt())
+        verify(buffer2, never()).dump(any(), anyInt())
     }
 
     @Test
@@ -188,10 +296,8 @@ class DumpManagerTest : SysuiTestCase() {
         dumpManager.dumpNormal(pw, args, tailLength = 2)
 
         // THEN the normal module and all buffers are dumped
-        verify(dumpable1, never())
-            .dump(any(PrintWriter::class.java), any(Array<String>::class.java))
-        verify(dumpable2, never())
-            .dump(any(PrintWriter::class.java), any(Array<String>::class.java))
+        verify(dumpable1, never()).dump(any(), any())
+        verify(dumpable2, never()).dump(any(), any())
         verify(dumpable3).dump(pw, args)
         verify(buffer1).dump(pw, tailLength = 2)
         verify(buffer2).dump(pw, tailLength = 2)
@@ -213,9 +319,7 @@ class DumpManagerTest : SysuiTestCase() {
 
         // THEN the unregistered dumpables (both normal and critical) are not dumped
         verify(dumpable1).dump(pw, args)
-        verify(dumpable2, never())
-            .dump(any(PrintWriter::class.java), any(Array<String>::class.java))
-        verify(dumpable3, never())
-            .dump(any(PrintWriter::class.java), any(Array<String>::class.java))
+        verify(dumpable2, never()).dump(any(), any())
+        verify(dumpable3, never()).dump(any(), any())
     }
 }