From 4715bc3646a7c05e79d863a696947b6bf682ace8 Mon Sep 17 00:00:00 2001 From: Caitlin Cassidy Date: Wed, 29 Dec 2021 21:09:34 +0000 Subject: [PATCH] [Disable Flags] Update logging to make it clear when there's been no change. Fixes: 211696714 Test: DisableFlagsLoggerTest Test: manual Change-Id: I72eb5a92723bc3477a8d56af1910364d13d2d86f --- .../systemui/statusbar/DisableFlagsLogger.kt | 58 +++++++++---------- .../statusbar/DisableFlagsLoggerTest.kt | 18 +++--- 2 files changed, 37 insertions(+), 39 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/DisableFlagsLogger.kt b/packages/SystemUI/src/com/android/systemui/statusbar/DisableFlagsLogger.kt index 4272bb14ff3a6..66591b3c7d911 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/DisableFlagsLogger.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/DisableFlagsLogger.kt @@ -74,9 +74,12 @@ class DisableFlagsLogger constructor( * Returns a string representing the, old, new, and new-after-modification disable flag states, * as well as the differences between each of the states. * - * Example: - * Old: EnaiHbcRso.qINgr | New: EnaihBcRso.qiNGR (hB.iGR) | New after local modification: - * EnaihBcRso.qInGR (.n) + * Example if [old], [new], and [newAfterLocalModification] are all different: + * Old: EnaiHbcRso.qINgr | New: EnaihBcRso.qiNGR (changed: hB.iGR) | New after local + * modification: EnaihBcRso.qInGR (changed: .n) + * + * Example if [old] and [new] are the same: + * EnaihBcRso.qiNGR (unchanged) * * A capital character signifies the flag is set and a lowercase character signifies that the * flag isn't set. The flag states will be logged in the same order as the passed-in lists. @@ -96,54 +99,51 @@ class DisableFlagsLogger constructor( new: DisableState, newAfterLocalModification: DisableState? = null ): String { - val builder = StringBuilder("Received new disable state. ") + val builder = StringBuilder("Received new disable state: ") - old?.let { + // This if/else has slightly repetitive code but is easier to read. + if (old != null && old != new) { builder.append("Old: ") builder.append(getFlagsString(old)) builder.append(" | ") - } - - builder.append("New: ") - if (old != null && old != new) { - builder.append(getFlagsStringWithDiff(old, new)) - } else { + builder.append("New: ") builder.append(getFlagsString(new)) + builder.append(" ") + builder.append(getDiffString(old, new)) + } else if (old != null && old == new) { + // If old and new are the same, we only need to print one of them. + builder.append(getFlagsString(new)) + builder.append(" ") + builder.append(getDiffString(old, new)) + } else { // old == null + builder.append(getFlagsString(new)) + // Don't get a diff string because we have no [old] to compare with. } if (newAfterLocalModification != null && new != newAfterLocalModification) { builder.append(" | New after local modification: ") - builder.append(getFlagsStringWithDiff(new, newAfterLocalModification)) + builder.append(getFlagsString(newAfterLocalModification)) + builder.append(" ") + builder.append(getDiffString(new, newAfterLocalModification)) } return builder.toString() } /** - * Returns a string representing [new] state, as well as the difference from [old] to [new] - * (if there is one). - */ - private fun getFlagsStringWithDiff(old: DisableState, new: DisableState): String { - val builder = StringBuilder() - builder.append(getFlagsString(new)) - builder.append(" ") - builder.append(getDiffString(old, new)) - return builder.toString() - } - - /** - * Returns a string representing the difference between [old] and [new], or an empty string if - * there is no difference. + * Returns a string representing the difference between [old] and [new]. * - * For example, if old was "abc.DE" and new was "aBC.De", the difference returned would be - * "(BC.e)". + * - If [old] was "abc.DE" and [new] was "aBC.De", the difference returned would be + * "(changed: BC.e)". + * - If [old] and [new] are the same, the difference returned would be "(unchanged)". */ private fun getDiffString(old: DisableState, new: DisableState): String { if (old == new) { - return "" + return "(unchanged)" } val builder = StringBuilder("(") + builder.append("changed: ") disable1FlagsList.forEach { val newSymbol = it.getFlagStatus(new.disable1) if (it.getFlagStatus(old.disable1) != newSymbol) { diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/DisableFlagsLoggerTest.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/DisableFlagsLoggerTest.kt index 38ad6b85f8aae..c0d1155b2700b 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/DisableFlagsLoggerTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/DisableFlagsLoggerTest.kt @@ -37,7 +37,7 @@ class DisableFlagsLoggerTest : SysuiTestCase() { private val disableFlagsLogger = DisableFlagsLogger(disable1Flags, disable2Flags) @Test - fun getDisableFlagsString_oldAndNewSame_statesLoggedButDiffsNotLogged() { + fun getDisableFlagsString_oldAndNewSame_newAndUnchangedLoggedOldNotLogged() { val state = DisableFlagsLogger.DisableState( 0b111, // ABC 0b01 // mN @@ -45,10 +45,9 @@ class DisableFlagsLoggerTest : SysuiTestCase() { val result = disableFlagsLogger.getDisableFlagsString(state, state) - assertThat(result).contains("Old: ABC.mN") - assertThat(result).contains("New: ABC.mN") - assertThat(result).doesNotContain("(") - assertThat(result).doesNotContain(")") + assertThat(result).doesNotContain("Old") + assertThat(result).contains("ABC.mN") + assertThat(result).contains("(unchanged)") } @Test @@ -66,7 +65,7 @@ class DisableFlagsLoggerTest : SysuiTestCase() { assertThat(result).contains("Old: ABC.mN") assertThat(result).contains("New: abC.Mn") - assertThat(result).contains("(ab.Mn)") + assertThat(result).contains("(changed: ab.Mn)") } @Test @@ -82,7 +81,7 @@ class DisableFlagsLoggerTest : SysuiTestCase() { ) ) - assertThat(result).contains("(.n)") + assertThat(result).contains("(changed: .n)") } @Test @@ -96,8 +95,7 @@ class DisableFlagsLoggerTest : SysuiTestCase() { ) assertThat(result).doesNotContain("Old") - assertThat(result).contains("New: abC.mN") - // We have no state to diff on, so we shouldn't see any diff in parentheses + assertThat(result).contains("abC.mN") assertThat(result).doesNotContain("(") assertThat(result).doesNotContain(")") } @@ -141,7 +139,7 @@ class DisableFlagsLoggerTest : SysuiTestCase() { ) ) - assertThat(result).contains("local modification: Abc.Mn (A.M)") + assertThat(result).contains("local modification: Abc.Mn (changed: A.M)") } @Test