From badc6f2483ce3b6acbf84c44ff70856aa650479f Mon Sep 17 00:00:00 2001 From: Caitlin Cassidy Date: Mon, 25 Oct 2021 14:48:37 +0000 Subject: [PATCH] [Disable Flags Logging] Fix CollapsedStatusBarFragmentLogger to log new+newAfterLocalModification instead of old+new. The paramaters passed into CollapsedStatusBarFragmentLogger were the newly received disable flags and the disable flags after they've been locally modified by CollapsedSBFragment. However, it was being logged as the previously received flags and then the newly received flags. This CL fixes that. Old string: "Old: enaihbcrso.qingr | New: eNaIhbCrsO.qingr (NICO.)" New string: "New: enaihbcrso.qingr | New after local modification: eNaIhbCrsO.qingr (NICO.)" Test: dumped buffer and verified output Test: atest DisableFlagsLoggerTest Bug: 197851207 Change-Id: Ifc36b1b6b85f5cfc08c2dff5078ed63f73899e9c --- .../systemui/statusbar/DisableFlagsLogger.kt | 20 +++++++++------ .../phone/CollapsedStatusBarFragment.java | 4 +-- .../phone/CollapsedStatusBarFragmentLogger.kt | 25 +++++++++++++------ .../statusbar/DisableFlagsLoggerTest.kt | 17 +++++++++++++ .../CollapsedStatusBarFragmentLoggerTest.kt | 6 +++-- 5 files changed, 53 insertions(+), 19 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/DisableFlagsLogger.kt b/packages/SystemUI/src/com/android/systemui/statusbar/DisableFlagsLogger.kt index cf34db233b065..4272bb14ff3a6 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/DisableFlagsLogger.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/DisableFlagsLogger.kt @@ -85,24 +85,30 @@ 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. + * @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, + old: DisableState? = null, new: DisableState, newAfterLocalModification: DisableState? = null ): String { val builder = StringBuilder("Received new disable state. ") - builder.append("Old: ") - builder.append(getFlagsString(old)) - builder.append(" | New: ") - if (old != new) { + + old?.let { + 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(getFlagsString(old)) + builder.append(getFlagsString(new)) } if (newAfterLocalModification != null && new != newAfterLocalModification) { diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/phone/CollapsedStatusBarFragment.java b/packages/SystemUI/src/com/android/systemui/statusbar/phone/CollapsedStatusBarFragment.java index 989f6b85668b4..d5912e0ddbe78 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/phone/CollapsedStatusBarFragment.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/phone/CollapsedStatusBarFragment.java @@ -253,8 +253,8 @@ public class CollapsedStatusBarFragment extends Fragment implements CommandQueue state1 = adjustDisableFlags(state1); mCollapsedStatusBarFragmentLogger.logDisableFlagChange( - new DisableState(state1BeforeAdjustment, state2), - new DisableState(state1, state2)); + /* new= */ new DisableState(state1BeforeAdjustment, state2), + /* newAfterLocalModification= */ new DisableState(state1, state2)); final int old1 = mDisabled1; final int diff1 = state1 ^ old1; diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/phone/CollapsedStatusBarFragmentLogger.kt b/packages/SystemUI/src/com/android/systemui/statusbar/phone/CollapsedStatusBarFragmentLogger.kt index 3c2b555eea68f..4d472e47a387c 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/phone/CollapsedStatusBarFragmentLogger.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/phone/CollapsedStatusBarFragmentLogger.kt @@ -28,22 +28,31 @@ class CollapsedStatusBarFragmentLogger @Inject constructor( private val disableFlagsLogger: DisableFlagsLogger, ) { - /** Logs a string representing the old and new disable flag states to [buffer]. */ + /** + * Logs a string representing the new state received by [CollapsedStatusBarFragment] and any + * modifications that were made to the flags locally. + * + * @param new see [DisableFlagsLogger.getDisableFlagsString] + * @param newAfterLocalModification see [DisableFlagsLogger.getDisableFlagsString] + */ fun logDisableFlagChange( - oldState: DisableFlagsLogger.DisableState, - newState: DisableFlagsLogger.DisableState) { + new: DisableFlagsLogger.DisableState, + newAfterLocalModification: DisableFlagsLogger.DisableState + ) { buffer.log( TAG, LogLevel.INFO, { - int1 = oldState.disable1 - int2 = oldState.disable2 - long1 = newState.disable1.toLong() - long2 = newState.disable2.toLong() + int1 = new.disable1 + int2 = new.disable2 + long1 = newAfterLocalModification.disable1.toLong() + long2 = newAfterLocalModification.disable2.toLong() }, { disableFlagsLogger.getDisableFlagsString( - DisableFlagsLogger.DisableState(int1, int2), + old = null, + new = DisableFlagsLogger.DisableState(int1, int2), + newAfterLocalModification = DisableFlagsLogger.DisableState(long1.toInt(), long2.toInt()) ) } 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 096efad506152..38ad6b85f8aae 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/DisableFlagsLoggerTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/DisableFlagsLoggerTest.kt @@ -85,6 +85,23 @@ class DisableFlagsLoggerTest : SysuiTestCase() { assertThat(result).contains("(.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("New: abC.mN") + // We have no state to diff on, so we shouldn't see any diff in parentheses + assertThat(result).doesNotContain("(") + assertThat(result).doesNotContain(")") + } + @Test fun getDisableFlagsString_nullLocalModification_localModNotLogged() { val result = disableFlagsLogger.getDisableFlagsString( diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/CollapsedStatusBarFragmentLoggerTest.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/CollapsedStatusBarFragmentLoggerTest.kt index f3136c7be9675..bf8cc37697b4f 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/CollapsedStatusBarFragmentLoggerTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/CollapsedStatusBarFragmentLoggerTest.kt @@ -40,7 +40,7 @@ class CollapsedStatusBarFragmentLoggerTest : SysuiTestCase() { private val logger = CollapsedStatusBarFragmentLogger(buffer, disableFlagsLogger) @Test - fun logToBuffer_bufferHasStates() { + fun logDisableFlagChange_bufferHasStates() { val state = DisableFlagsLogger.DisableState(0, 1) logger.logDisableFlagChange(state, state) @@ -48,7 +48,9 @@ class CollapsedStatusBarFragmentLoggerTest : SysuiTestCase() { val stringWriter = StringWriter() buffer.dump(PrintWriter(stringWriter), tailLength = 0) val actualString = stringWriter.toString() - val expectedLogString = disableFlagsLogger.getDisableFlagsString(state, state) + val expectedLogString = disableFlagsLogger.getDisableFlagsString( + old = null, new = state, newAfterLocalModification = state + ) assertThat(actualString).contains(expectedLogString) }