[SB Refactor] Update TableLogBuffer based on ABT changes.

1) Add a header and footer so ABT can find the sections.
2) Require certain format from the column names.
3) Remove `dumpTables`, since that's now in ABT.
4) Update WifiNetwork's logging to not use -1 (-1 seemed
   unclear when reading a real bug report).

Bug: 259454430
Test: Dumped BR, uploaded to ABT, and verified that the wifi table log
appeared correctly (with corresponding ABT changes)
Test: atest TableLogBufferTest

Change-Id: I0564b3d0f423adcc3c3164d10fef062c677a0769

4 files changed, 189 insertions, 112 deletions

diff --git a/packages/SystemUI/src/com/android/systemui/log/table/TableLogBuffer.kt b/packages/SystemUI/src/com/android/systemui/log/table/TableLogBuffer.kt
index 429637a0ee4d..02aac01521bc 100644
--- a/packages/SystemUI/src/com/android/systemui/log/table/TableLogBuffer.kt
+++ b/packages/SystemUI/src/com/android/systemui/log/table/TableLogBuffer.kt
@@ -16,10 +16,7 @@
 
 package com.android.systemui.log.table
 
-import androidx.annotation.VisibleForTesting
 import com.android.systemui.Dumpable
-import com.android.systemui.dump.DumpManager
-import com.android.systemui.dump.DumpsysTableLogger
 import com.android.systemui.plugins.util.RingBuffer
 import com.android.systemui.util.time.SystemClock
 import java.io.PrintWriter
@@ -83,7 +80,7 @@ class TableLogBuffer(
     maxSize: Int,
     private val name: String,
     private val systemClock: SystemClock,
-) {
+) : Dumpable {
     init {
         if (maxSize <= 0) {
             throw IllegalArgumentException("maxSize must be > 0")
@@ -104,6 +101,9 @@ class TableLogBuffer(
      * @param columnPrefix a prefix that will be applied to every column name that gets logged. This
      * ensures that all the columns related to the same state object will be grouped together in the
      * table.
+     *
+     * @throws IllegalArgumentException if [columnPrefix] or column name contain "|". "|" is used as
+     * the separator token for parsing, so it can't be present in any part of the column name.
      */
     @Synchronized
     fun <T : Diffable<T>> logDiffs(columnPrefix: String, prevVal: T, newVal: T) {
@@ -135,32 +135,31 @@ class TableLogBuffer(
 
     @Synchronized
     private fun obtain(timestamp: Long, prefix: String, columnName: String): TableChange {
+        verifyValidName(prefix, columnName)
         val tableChange = buffer.advance()
         tableChange.reset(timestamp, prefix, columnName)
         return tableChange
     }
 
-    /**
-     * Registers this buffer as dumpables in [dumpManager]. Must be called for the table to be
-     * dumped.
-     *
-     * This will be automatically called in [TableLogBufferFactory.create].
-     */
-    fun registerDumpables(dumpManager: DumpManager) {
-        dumpManager.registerNormalDumpable("$name-changes", changeDumpable)
-        dumpManager.registerNormalDumpable("$name-table", tableDumpable)
+    private fun verifyValidName(prefix: String, columnName: String) {
+        if (prefix.contains(SEPARATOR)) {
+            throw IllegalArgumentException("columnPrefix cannot contain $SEPARATOR but was $prefix")
+        }
+        if (columnName.contains(SEPARATOR)) {
+            throw IllegalArgumentException(
+                "columnName cannot contain $SEPARATOR but was $columnName"
+            )
+        }
     }
 
-    private val changeDumpable = Dumpable { pw, _ -> dumpChanges(pw) }
-    private val tableDumpable = Dumpable { pw, _ -> dumpTable(pw) }
-
-    /** Dumps the list of [TableChange] objects. */
     @Synchronized
-    @VisibleForTesting
-    fun dumpChanges(pw: PrintWriter) {
+    override fun dump(pw: PrintWriter, args: Array<out String>) {
+        pw.println(HEADER_PREFIX + name)
+        pw.println("version $VERSION")
         for (i in 0 until buffer.size) {
             buffer[i].dump(pw)
         }
+        pw.println(FOOTER_PREFIX + name)
     }
 
     /** Dumps an individual [TableChange]. */
@@ -170,70 +169,14 @@ class TableLogBuffer(
         }
         val formattedTimestamp = TABLE_LOG_DATE_FORMAT.format(timestamp)
         pw.print(formattedTimestamp)
-        pw.print(" ")
+        pw.print(SEPARATOR)
         pw.print(this.getName())
-        pw.print("=")
+        pw.print(SEPARATOR)
         pw.print(this.getVal())
         pw.println()
     }
 
     /**
-     * Coalesces all the [TableChange] objects into a table of values of time and dumps the table.
-     */
-    // TODO(b/259454430): Since this is an expensive process, it could cause the bug report dump to
-    //   fail and/or not dump anything else. We should move this processing to ABT (Android Bug
-    //   Tool), where we have unlimited time to process.
-    @Synchronized
-    @VisibleForTesting
-    fun dumpTable(pw: PrintWriter) {
-        val messages = buffer.iterator().asSequence().toList()
-
-        if (messages.isEmpty()) {
-            return
-        }
-
-        // Step 1: Create list of column headers
-        val headerSet = mutableSetOf<String>()
-        messages.forEach { headerSet.add(it.getName()) }
-        val headers: MutableList<String> = headerSet.toList().sorted().toMutableList()
-        headers.add(0, "timestamp")
-
-        // Step 2: Create a list with the current values for each column. Will be updated with each
-        // change.
-        val currentRow: MutableList<String> = MutableList(headers.size) { DEFAULT_COLUMN_VALUE }
-
-        // Step 3: For each message, make the correct update to [currentRow] and save it to [rows].
-        val columnIndices: Map<String, Int> =
-            headers.mapIndexed { index, headerName -> headerName to index }.toMap()
-        val allRows = mutableListOf<List<String>>()
-
-        messages.forEach {
-            if (!it.hasData()) {
-                return@forEach
-            }
-
-            val formattedTimestamp = TABLE_LOG_DATE_FORMAT.format(it.timestamp)
-            if (formattedTimestamp != currentRow[0]) {
-                // The timestamp has updated, so save the previous row and continue to the next row
-                allRows.add(currentRow.toList())
-                currentRow[0] = formattedTimestamp
-            }
-            val columnIndex = columnIndices[it.getName()]!!
-            currentRow[columnIndex] = it.getVal()
-        }
-        // Add the last row
-        allRows.add(currentRow.toList())
-
-        // Step 4: Dump the rows
-        DumpsysTableLogger(
-                name,
-                headers,
-                allRows,
-            )
-            .printTableData(pw)
-    }
-
-    /**
      * A private implementation of [TableRowLogger].
      *
      * Used so that external clients can't modify [timestamp].
@@ -261,4 +204,8 @@ class TableLogBuffer(
 }
 
 val TABLE_LOG_DATE_FORMAT = SimpleDateFormat("MM-dd HH:mm:ss.SSS", Locale.US)
-private const val DEFAULT_COLUMN_VALUE = "UNKNOWN"
+
+private const val HEADER_PREFIX = "SystemUI StateChangeTableSection START: "
+private const val FOOTER_PREFIX = "SystemUI StateChangeTableSection END: "
+private const val SEPARATOR = "|"
+private const val VERSION = "1"
diff --git a/packages/SystemUI/src/com/android/systemui/log/table/TableLogBufferFactory.kt b/packages/SystemUI/src/com/android/systemui/log/table/TableLogBufferFactory.kt
index f1f906f46d2d..7a90a7470cd2 100644
--- a/packages/SystemUI/src/com/android/systemui/log/table/TableLogBufferFactory.kt
+++ b/packages/SystemUI/src/com/android/systemui/log/table/TableLogBufferFactory.kt
@@ -34,7 +34,7 @@ constructor(
         maxSize: Int,
     ): TableLogBuffer {
         val tableBuffer = TableLogBuffer(adjustMaxSize(maxSize), name, systemClock)
-        tableBuffer.registerDumpables(dumpManager)
+        dumpManager.registerNormalDumpable(name, tableBuffer)
         return tableBuffer
     }
 }
diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/wifi/data/model/WifiNetworkModel.kt b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/wifi/data/model/WifiNetworkModel.kt
index 8436b13d7038..d663fd6e17b2 100644
--- a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/wifi/data/model/WifiNetworkModel.kt
+++ b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/wifi/data/model/WifiNetworkModel.kt
@@ -121,7 +121,11 @@ sealed class WifiNetworkModel : Diffable<WifiNetworkModel> {
                 row.logChange(COL_VALIDATED, isValidated)
             }
             if (prevVal !is Active || prevVal.level != level) {
-                row.logChange(COL_LEVEL, level ?: LEVEL_DEFAULT)
+                if (level != null) {
+                    row.logChange(COL_LEVEL, level)
+                } else {
+                    row.logChange(COL_LEVEL, LEVEL_DEFAULT)
+                }
             }
             if (prevVal !is Active || prevVal.ssid != ssid) {
                 row.logChange(COL_SSID, ssid)
@@ -201,5 +205,5 @@ const val COL_PASSPOINT_ACCESS_POINT = "isPasspointAccessPoint"
 const val COL_ONLINE_SIGN_UP = "isOnlineSignUpForPasspointAccessPoint"
 const val COL_PASSPOINT_NAME = "passpointProviderFriendlyName"
 
-const val LEVEL_DEFAULT = -1
-const val NETWORK_ID_DEFAULT = -1
+val LEVEL_DEFAULT: String? = null
+val NETWORK_ID_DEFAULT: String? = null
diff --git a/packages/SystemUI/tests/src/com/android/systemui/log/table/TableLogBufferTest.kt b/packages/SystemUI/tests/src/com/android/systemui/log/table/TableLogBufferTest.kt
index 688c66ac80c9..ba21f1260b32 100644
--- a/packages/SystemUI/tests/src/com/android/systemui/log/table/TableLogBufferTest.kt
+++ b/packages/SystemUI/tests/src/com/android/systemui/log/table/TableLogBufferTest.kt
@@ -46,6 +46,93 @@ class TableLogBufferTest : SysuiTestCase() {
     }
 
     @Test
+    fun dumpChanges_hasHeader() {
+        val dumpedString = dumpChanges()
+
+        assertThat(logLines(dumpedString)[0]).isEqualTo(HEADER_PREFIX + NAME)
+    }
+
+    @Test
+    fun dumpChanges_hasVersion() {
+        val dumpedString = dumpChanges()
+
+        assertThat(logLines(dumpedString)[1]).isEqualTo("version $VERSION")
+    }
+
+    @Test
+    fun dumpChanges_hasFooter() {
+        val dumpedString = dumpChanges()
+
+        assertThat(logLines(dumpedString).last()).isEqualTo(FOOTER_PREFIX + NAME)
+    }
+
+    @Test(expected = IllegalArgumentException::class)
+    fun dumpChanges_str_separatorNotAllowedInPrefix() {
+        val next =
+            object : TestDiffable() {
+                override fun logDiffs(prevVal: TestDiffable, row: TableRowLogger) {
+                    row.logChange("columnName", "stringValue")
+                }
+            }
+        underTest.logDiffs("some${SEPARATOR}thing", TestDiffable(), next)
+    }
+
+    @Test(expected = IllegalArgumentException::class)
+    fun dumpChanges_bool_separatorNotAllowedInPrefix() {
+        val next =
+            object : TestDiffable() {
+                override fun logDiffs(prevVal: TestDiffable, row: TableRowLogger) {
+                    row.logChange("columnName", true)
+                }
+            }
+        underTest.logDiffs("some${SEPARATOR}thing", TestDiffable(), next)
+    }
+
+    @Test(expected = IllegalArgumentException::class)
+    fun dumpChanges_int_separatorNotAllowedInPrefix() {
+        val next =
+            object : TestDiffable() {
+                override fun logDiffs(prevVal: TestDiffable, row: TableRowLogger) {
+                    row.logChange("columnName", 567)
+                }
+            }
+        underTest.logDiffs("some${SEPARATOR}thing", TestDiffable(), next)
+    }
+
+    @Test(expected = IllegalArgumentException::class)
+    fun dumpChanges_str_separatorNotAllowedInColumnName() {
+        val next =
+            object : TestDiffable() {
+                override fun logDiffs(prevVal: TestDiffable, row: TableRowLogger) {
+                    row.logChange("column${SEPARATOR}Name", "stringValue")
+                }
+            }
+        underTest.logDiffs("prefix", TestDiffable(), next)
+    }
+
+    @Test(expected = IllegalArgumentException::class)
+    fun dumpChanges_bool_separatorNotAllowedInColumnName() {
+        val next =
+            object : TestDiffable() {
+                override fun logDiffs(prevVal: TestDiffable, row: TableRowLogger) {
+                    row.logChange("column${SEPARATOR}Name", true)
+                }
+            }
+        underTest.logDiffs("prefix", TestDiffable(), next)
+    }
+
+    @Test(expected = IllegalArgumentException::class)
+    fun dumpChanges_int_separatorNotAllowedInColumnName() {
+        val next =
+            object : TestDiffable() {
+                override fun logDiffs(prevVal: TestDiffable, row: TableRowLogger) {
+                    row.logChange("column${SEPARATOR}Name", 456)
+                }
+            }
+        underTest.logDiffs("prefix", TestDiffable(), next)
+    }
+
+    @Test
     fun dumpChanges_strChange_logsFromNext() {
         systemClock.setCurrentTimeMillis(100L)
 
@@ -66,11 +153,14 @@ class TableLogBufferTest : SysuiTestCase() {
 
         val dumpedString = dumpChanges()
 
-        assertThat(dumpedString).contains("prefix")
-        assertThat(dumpedString).contains("stringValChange")
-        assertThat(dumpedString).contains("newStringVal")
+        val expected =
+            TABLE_LOG_DATE_FORMAT.format(100L) +
+                SEPARATOR +
+                "prefix.stringValChange" +
+                SEPARATOR +
+                "newStringVal"
+        assertThat(dumpedString).contains(expected)
         assertThat(dumpedString).doesNotContain("prevStringVal")
-        assertThat(dumpedString).contains(TABLE_LOG_DATE_FORMAT.format(100L))
     }
 
     @Test
@@ -94,11 +184,14 @@ class TableLogBufferTest : SysuiTestCase() {
 
         val dumpedString = dumpChanges()
 
-        assertThat(dumpedString).contains("prefix")
-        assertThat(dumpedString).contains("booleanValChange")
-        assertThat(dumpedString).contains("true")
+        val expected =
+            TABLE_LOG_DATE_FORMAT.format(100L) +
+                SEPARATOR +
+                "prefix.booleanValChange" +
+                SEPARATOR +
+                "true"
+        assertThat(dumpedString).contains(expected)
         assertThat(dumpedString).doesNotContain("false")
-        assertThat(dumpedString).contains(TABLE_LOG_DATE_FORMAT.format(100L))
     }
 
     @Test
@@ -122,11 +215,14 @@ class TableLogBufferTest : SysuiTestCase() {
 
         val dumpedString = dumpChanges()
 
-        assertThat(dumpedString).contains("prefix")
-        assertThat(dumpedString).contains("intValChange")
-        assertThat(dumpedString).contains("67890")
+        val expected =
+            TABLE_LOG_DATE_FORMAT.format(100L) +
+                SEPARATOR +
+                "prefix.intValChange" +
+                SEPARATOR +
+                "67890"
+        assertThat(dumpedString).contains(expected)
         assertThat(dumpedString).doesNotContain("12345")
-        assertThat(dumpedString).contains(TABLE_LOG_DATE_FORMAT.format(100L))
     }
 
     @Test
@@ -152,9 +248,9 @@ class TableLogBufferTest : SysuiTestCase() {
         val dumpedString = dumpChanges()
 
         // THEN the dump still works
-        assertThat(dumpedString).contains("booleanValChange")
-        assertThat(dumpedString).contains("true")
-        assertThat(dumpedString).contains(TABLE_LOG_DATE_FORMAT.format(100L))
+        val expected =
+            TABLE_LOG_DATE_FORMAT.format(100L) + SEPARATOR + "booleanValChange" + SEPARATOR + "true"
+        assertThat(dumpedString).contains(expected)
     }
 
     @Test
@@ -186,15 +282,34 @@ class TableLogBufferTest : SysuiTestCase() {
 
         val dumpedString = dumpChanges()
 
-        assertThat(dumpedString).contains("valChange")
-        assertThat(dumpedString).contains("stateValue12")
-        assertThat(dumpedString).contains("stateValue20")
-        assertThat(dumpedString).contains("stateValue40")
-        assertThat(dumpedString).contains("stateValue45")
-        assertThat(dumpedString).contains(TABLE_LOG_DATE_FORMAT.format(12000L))
-        assertThat(dumpedString).contains(TABLE_LOG_DATE_FORMAT.format(20000L))
-        assertThat(dumpedString).contains(TABLE_LOG_DATE_FORMAT.format(40000L))
-        assertThat(dumpedString).contains(TABLE_LOG_DATE_FORMAT.format(45000L))
+        val expected1 =
+            TABLE_LOG_DATE_FORMAT.format(12000L) +
+                SEPARATOR +
+                "valChange" +
+                SEPARATOR +
+                "stateValue12"
+        val expected2 =
+            TABLE_LOG_DATE_FORMAT.format(20000L) +
+                SEPARATOR +
+                "valChange" +
+                SEPARATOR +
+                "stateValue20"
+        val expected3 =
+            TABLE_LOG_DATE_FORMAT.format(40000L) +
+                SEPARATOR +
+                "valChange" +
+                SEPARATOR +
+                "stateValue40"
+        val expected4 =
+            TABLE_LOG_DATE_FORMAT.format(45000L) +
+                SEPARATOR +
+                "valChange" +
+                SEPARATOR +
+                "stateValue45"
+        assertThat(dumpedString).contains(expected1)
+        assertThat(dumpedString).contains(expected2)
+        assertThat(dumpedString).contains(expected3)
+        assertThat(dumpedString).contains(expected4)
     }
 
     @Test
@@ -214,10 +329,11 @@ class TableLogBufferTest : SysuiTestCase() {
 
         val dumpedString = dumpChanges()
 
-        assertThat(dumpedString).contains("status")
-        assertThat(dumpedString).contains("in progress")
-        assertThat(dumpedString).contains("connected")
-        assertThat(dumpedString).contains("false")
+        val timestamp = TABLE_LOG_DATE_FORMAT.format(100L)
+        val expected1 = timestamp + SEPARATOR + "status" + SEPARATOR + "in progress"
+        val expected2 = timestamp + SEPARATOR + "connected" + SEPARATOR + "false"
+        assertThat(dumpedString).contains(expected1)
+        assertThat(dumpedString).contains(expected2)
     }
 
     @Test
@@ -247,14 +363,24 @@ class TableLogBufferTest : SysuiTestCase() {
     }
 
     private fun dumpChanges(): String {
-        underTest.dumpChanges(PrintWriter(outputWriter))
+        underTest.dump(PrintWriter(outputWriter), arrayOf())
         return outputWriter.toString()
     }
 
-    private abstract class TestDiffable : Diffable<TestDiffable> {
+    private fun logLines(string: String): List<String> {
+        return string.split("\n").filter { it.isNotBlank() }
+    }
+
+    private open class TestDiffable : Diffable<TestDiffable> {
         override fun logDiffs(prevVal: TestDiffable, row: TableRowLogger) {}
     }
 }
 
 private const val NAME = "TestTableBuffer"
 private const val MAX_SIZE = 10
+
+// Copying these here from [TableLogBuffer] so that we catch any accidental versioning change
+private const val HEADER_PREFIX = "SystemUI StateChangeTableSection START: "
+private const val FOOTER_PREFIX = "SystemUI StateChangeTableSection END: "
+private const val SEPARATOR = "|" // TBD
+private const val VERSION = "1"