From ebddcea829cac9d5975aa52f22b002f2180b432e Mon Sep 17 00:00:00 2001 From: Caitlin Shkuratov Date: Tue, 13 Jun 2023 14:32:25 +0000 Subject: [PATCH] [Central Surfaces][Disable] Remove `old` from the disable logger. 2/3 locations were passing in `null` for `old`, and the third location in CentralSurfacesCommandQueueCallbacks was using a simple Log.d that we never look at in bug reports. Bug: 277764509 Test: atest DisableFlagsLoggerTest Change-Id: Ib2bc98053c801bb9af6e948945169481e3b16afa --- .../qs/QSFragmentDisableFlagsLogger.kt | 1 - .../disableflags/DisableFlagsLogger.kt | 35 ++------ .../CentralSurfacesCommandQueueCallbacks.java | 2 - .../CollapsedStatusBarFragmentLogger.kt | 4 +- .../qs/QSFragmentDisableFlagsLoggerTest.kt | 2 +- .../disableflags/DisableFlagsLoggerTest.kt | 86 ++++--------------- .../CollapsedStatusBarFragmentLoggerTest.kt | 1 - 7 files changed, 26 insertions(+), 105 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/qs/QSFragmentDisableFlagsLogger.kt b/packages/SystemUI/src/com/android/systemui/qs/QSFragmentDisableFlagsLogger.kt index cd52ec29177d7..ac6aabb2e5bde 100644 --- a/packages/SystemUI/src/com/android/systemui/qs/QSFragmentDisableFlagsLogger.kt +++ b/packages/SystemUI/src/com/android/systemui/qs/QSFragmentDisableFlagsLogger.kt @@ -34,7 +34,6 @@ class QSFragmentDisableFlagsLogger @Inject constructor( }, { disableFlagsLogger.getDisableFlagsString( - old = null, new = DisableFlagsLogger.DisableState(int1, int2), newAfterLocalModification = DisableFlagsLogger.DisableState(long1.toInt(), long2.toInt()) diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/disableflags/DisableFlagsLogger.kt b/packages/SystemUI/src/com/android/systemui/statusbar/disableflags/DisableFlagsLogger.kt index f04159cd631fa..dc868695490b1 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/disableflags/DisableFlagsLogger.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/disableflags/DisableFlagsLogger.kt @@ -71,15 +71,14 @@ 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. + * Returns a string representing the new and new-after-modification disable flag states, + * as well as the differences between them (if there are any). * - * 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 [new] and [newAfterLocalModification] are different: + * New: EnaihBcRso.qiNGR | New after local modification: EnaihBCRso.qInGR (changed: C.In) * - * Example if [old] and [new] are the same: - * EnaihBcRso.qiNGR (unchanged) + * Example if [new] and [newAfterLocalModification] are the same: + * New: EnaihBcRso.qiNGR * * 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. @@ -88,37 +87,17 @@ class DisableFlagsLogger constructor( * is no difference. the new-after-modification state also won't be included if there's no * difference from the new state. * - * @param old the disable state that had been previously sent. Null if we don't need to log the - * previously sent state. * @param new the new disable state that has just been sent. * @param newAfterLocalModification the new disable states after a class has locally modified * them. Null if the class does not locally modify. */ fun getDisableFlagsString( - old: DisableState? = null, new: DisableState, newAfterLocalModification: DisableState? = null ): String { val builder = StringBuilder("Received new disable state: ") - // 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: ") - 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. - } + builder.append(getFlagsString(new)) if (newAfterLocalModification != null && new != newAfterLocalModification) { builder.append(" | New after local modification: ") diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/phone/CentralSurfacesCommandQueueCallbacks.java b/packages/SystemUI/src/com/android/systemui/statusbar/phone/CentralSurfacesCommandQueueCallbacks.java index 5e0cfd6e4c335..555a3c2c439fb 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/phone/CentralSurfacesCommandQueueCallbacks.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/phone/CentralSurfacesCommandQueueCallbacks.java @@ -255,8 +255,6 @@ public class CentralSurfacesCommandQueueCallbacks implements CommandQueue.Callba state2 = mRemoteInputQuickSettingsDisabler.adjustDisableFlags(state2); Log.d(CentralSurfaces.TAG, mDisableFlagsLogger.getDisableFlagsString( - /* old= */ new DisableFlagsLogger.DisableState( - mCentralSurfaces.getDisabled1(), mCentralSurfaces.getDisabled2()), /* new= */ new DisableFlagsLogger.DisableState( state1, state2BeforeAdjustment), /* newStateAfterLocalModification= */ new DisableFlagsLogger.DisableState( diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/phone/fragment/CollapsedStatusBarFragmentLogger.kt b/packages/SystemUI/src/com/android/systemui/statusbar/phone/fragment/CollapsedStatusBarFragmentLogger.kt index 8c19fb4f43c94..f4ab408cc275f 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/phone/fragment/CollapsedStatusBarFragmentLogger.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/phone/fragment/CollapsedStatusBarFragmentLogger.kt @@ -33,7 +33,6 @@ class CollapsedStatusBarFragmentLogger @Inject constructor( * modifications that were made to the flags locally. * * @param new see [DisableFlagsLogger.getDisableFlagsString] - * @param newAfterLocalModification see [DisableFlagsLogger.getDisableFlagsString] */ fun logDisableFlagChange( new: DisableFlagsLogger.DisableState, @@ -47,8 +46,7 @@ class CollapsedStatusBarFragmentLogger @Inject constructor( }, { disableFlagsLogger.getDisableFlagsString( - old = null, - new = DisableFlagsLogger.DisableState(int1, int2), + DisableFlagsLogger.DisableState(int1, int2), ) } ) diff --git a/packages/SystemUI/tests/src/com/android/systemui/qs/QSFragmentDisableFlagsLoggerTest.kt b/packages/SystemUI/tests/src/com/android/systemui/qs/QSFragmentDisableFlagsLoggerTest.kt index aacc695ef301b..93f316e9a6199 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/qs/QSFragmentDisableFlagsLoggerTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/qs/QSFragmentDisableFlagsLoggerTest.kt @@ -49,7 +49,7 @@ class QSFragmentDisableFlagsLoggerTest : SysuiTestCase() { buffer.dump(PrintWriter(stringWriter), tailLength = 0) val actualString = stringWriter.toString() val expectedLogString = disableFlagsLogger.getDisableFlagsString( - old = null, new = state, newAfterLocalModification = state + new = state, newAfterLocalModification = state ) assertThat(actualString).contains(expectedLogString) diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/disableflags/DisableFlagsLoggerTest.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/disableflags/DisableFlagsLoggerTest.kt index 4b5d0a37b023a..f1c7956a1bc75 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/disableflags/DisableFlagsLoggerTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/disableflags/DisableFlagsLoggerTest.kt @@ -36,74 +36,9 @@ class DisableFlagsLoggerTest : SysuiTestCase() { private val disableFlagsLogger = DisableFlagsLogger(disable1Flags, disable2Flags) - @Test - fun getDisableFlagsString_oldAndNewSame_newAndUnchangedLoggedOldNotLogged() { - val state = DisableFlagsLogger.DisableState( - 0b111, // ABC - 0b01 // mN - ) - - val result = disableFlagsLogger.getDisableFlagsString(state, state) - - assertThat(result).doesNotContain("Old") - assertThat(result).contains("ABC.mN") - assertThat(result).contains("(unchanged)") - } - - @Test - fun getDisableFlagsString_oldAndNewDifferent_statesAndDiffLogged() { - val result = disableFlagsLogger.getDisableFlagsString( - DisableFlagsLogger.DisableState( - 0b111, // ABC - 0b01, // mN - ), - DisableFlagsLogger.DisableState( - 0b001, // abC - 0b10 // Mn - ) - ) - - assertThat(result).contains("Old: ABC.mN") - assertThat(result).contains("New: abC.Mn") - assertThat(result).contains("(changed: ab.Mn)") - } - - @Test - fun getDisableFlagsString_onlyDisable2Different_diffLoggedCorrectly() { - val result = disableFlagsLogger.getDisableFlagsString( - DisableFlagsLogger.DisableState( - 0b001, // abC - 0b01, // mN - ), - DisableFlagsLogger.DisableState( - 0b001, // abC - 0b00 // mn - ) - ) - - assertThat(result).contains("(changed: .n)") - } - - @Test - fun getDisableFlagsString_nullOld_onlyNewStateLogged() { - val result = disableFlagsLogger.getDisableFlagsString( - old = null, - new = DisableFlagsLogger.DisableState( - 0b001, // abC - 0b01, // mN - ), - ) - - assertThat(result).doesNotContain("Old") - assertThat(result).contains("abC.mN") - assertThat(result).doesNotContain("(") - assertThat(result).doesNotContain(")") - } - @Test fun getDisableFlagsString_nullLocalModification_localModNotLogged() { val result = disableFlagsLogger.getDisableFlagsString( - DisableFlagsLogger.DisableState(0, 0), DisableFlagsLogger.DisableState(1, 1), newAfterLocalModification = null ) @@ -118,9 +53,7 @@ class DisableFlagsLoggerTest : SysuiTestCase() { 0b10 // mn ) - val result = disableFlagsLogger.getDisableFlagsString( - DisableFlagsLogger.DisableState(0, 0), newState, newState - ) + val result = disableFlagsLogger.getDisableFlagsString(newState, newState) assertThat(result).doesNotContain("local modification") } @@ -128,7 +61,6 @@ class DisableFlagsLoggerTest : SysuiTestCase() { @Test fun getDisableFlagsString_newAfterLocalModificationDifferent_localModAndDiffLogged() { val result = disableFlagsLogger.getDisableFlagsString( - old = DisableFlagsLogger.DisableState(0, 0), new = DisableFlagsLogger.DisableState( 0b000, // abc 0b00 // mn @@ -142,6 +74,22 @@ class DisableFlagsLoggerTest : SysuiTestCase() { assertThat(result).contains("local modification: Abc.Mn (changed: A.M)") } + @Test + fun getDisableFlagsString_onlyDisable2Different_diffLoggedCorrectly() { + val result = disableFlagsLogger.getDisableFlagsString( + DisableFlagsLogger.DisableState( + 0b001, // abC + 0b01, // mN + ), + DisableFlagsLogger.DisableState( + 0b001, // abC + 0b00 // mn + ) + ) + + assertThat(result).contains("(changed: .n)") + } + @Test fun constructor_defaultDisableFlags_noException() { // Just creating the logger with the default params will trigger the exception if there diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/fragment/CollapsedStatusBarFragmentLoggerTest.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/fragment/CollapsedStatusBarFragmentLoggerTest.kt index 9bc49ae02436b..87d813c6d19fb 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/fragment/CollapsedStatusBarFragmentLoggerTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/fragment/CollapsedStatusBarFragmentLoggerTest.kt @@ -50,7 +50,6 @@ class CollapsedStatusBarFragmentLoggerTest : SysuiTestCase() { val actualString = stringWriter.toString() val expectedLogString = disableFlagsLogger.getDisableFlagsString( - old = null, new = state, newAfterLocalModification = null, )