From f913becde75901eb825c1d6a436ddb91da07c88a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabi=C3=A1n=20Kozynski?= Date: Wed, 12 Jul 2023 15:41:28 -0400 Subject: [PATCH] Debounce messages in BrightnessController Prevent message for register/unregister to be posted more than once. Also, refactor BrightnessController for better testability. Fixes: 289042855 Test: atest BrightnessControllerTest QSPanelControllerTest Change-Id: Id72c0f2bfd6fd3720670a1a55c67526c63a043d4 --- .../systemui/qs/QSPanelController.java | 16 ++- .../brightness/BrightnessController.java | 110 +++++++++--------- .../settings/brightness/BrightnessDialog.java | 26 ++--- .../systemui/qs/QSPanelControllerTest.kt | 15 +++ .../brightness/BrightnessControllerTest.kt | 105 +++++++++++++++++ .../brightness/BrightnessDialogTest.kt | 22 +--- 6 files changed, 196 insertions(+), 98 deletions(-) create mode 100644 packages/SystemUI/tests/src/com/android/systemui/settings/brightness/BrightnessControllerTest.kt diff --git a/packages/SystemUI/src/com/android/systemui/qs/QSPanelController.java b/packages/SystemUI/src/com/android/systemui/qs/QSPanelController.java index 83b373d5e6268..856a92e85ad73 100644 --- a/packages/SystemUI/src/com/android/systemui/qs/QSPanelController.java +++ b/packages/SystemUI/src/com/android/systemui/qs/QSPanelController.java @@ -58,6 +58,7 @@ public class QSPanelController extends QSPanelControllerBase { private final BrightnessSliderController mBrightnessSliderController; private final BrightnessMirrorHandler mBrightnessMirrorHandler; private final StatusBarKeyguardViewManager mStatusBarKeyguardViewManager; + private boolean mListening; private View.OnTouchListener mTileLayoutTouchListener = new View.OnTouchListener() { @Override @@ -159,12 +160,15 @@ public class QSPanelController extends QSPanelControllerBase { public void setListening(boolean listening, boolean expanded) { setListening(listening && expanded); - // Set the listening as soon as the QS fragment starts listening regardless of the - //expansion, so it will update the current brightness before the slider is visible. - if (listening) { - mBrightnessController.registerCallbacks(); - } else { - mBrightnessController.unregisterCallbacks(); + if (listening != mListening) { + mListening = listening; + // Set the listening as soon as the QS fragment starts listening regardless of the + //expansion, so it will update the current brightness before the slider is visible. + if (listening) { + mBrightnessController.registerCallbacks(); + } else { + mBrightnessController.unregisterCallbacks(); + } } } diff --git a/packages/SystemUI/src/com/android/systemui/settings/brightness/BrightnessController.java b/packages/SystemUI/src/com/android/systemui/settings/brightness/BrightnessController.java index 9594ba374fc3e..6af9b739da527 100644 --- a/packages/SystemUI/src/com/android/systemui/settings/brightness/BrightnessController.java +++ b/packages/SystemUI/src/com/android/systemui/settings/brightness/BrightnessController.java @@ -22,7 +22,6 @@ import static com.android.settingslib.display.BrightnessUtils.convertLinearToGam import android.animation.ValueAnimator; import android.annotation.NonNull; -import android.content.ContentResolver; import android.content.Context; import android.database.ContentObserver; import android.hardware.display.BrightnessInfo; @@ -31,10 +30,10 @@ import android.net.Uri; import android.os.AsyncTask; import android.os.Handler; import android.os.HandlerExecutor; +import android.os.Looper; import android.os.Message; import android.os.PowerManager; import android.os.RemoteException; -import android.os.ServiceManager; import android.os.UserHandle; import android.os.UserManager; import android.provider.Settings; @@ -43,6 +42,8 @@ import android.service.vr.IVrStateCallbacks; import android.util.Log; import android.util.MathUtils; +import androidx.annotation.Nullable; + import com.android.internal.display.BrightnessSynchronizer; import com.android.internal.logging.MetricsLogger; import com.android.internal.logging.nano.MetricsProto.MetricsEvent; @@ -52,10 +53,13 @@ import com.android.systemui.dagger.qualifiers.Main; import com.android.systemui.settings.DisplayTracker; import com.android.systemui.settings.UserTracker; import com.android.systemui.statusbar.policy.BrightnessMirrorController; +import com.android.systemui.util.settings.SecureSettings; import java.util.concurrent.Executor; -import javax.inject.Inject; +import dagger.assisted.Assisted; +import dagger.assisted.AssistedFactory; +import dagger.assisted.AssistedInject; public class BrightnessController implements ToggleSlider.Listener, MirroredBrightnessController { private static final String TAG = "CentralSurfaces.BrightnessController"; @@ -75,8 +79,11 @@ public class BrightnessController implements ToggleSlider.Listener, MirroredBrig private final DisplayManager mDisplayManager; private final UserTracker mUserTracker; private final DisplayTracker mDisplayTracker; + @Nullable private final IVrManager mVrManager; + private final SecureSettings mSecureSettings; + private final Executor mMainExecutor; private final Handler mBackgroundHandler; private final BrightnessObserver mBrightnessObserver; @@ -106,6 +113,8 @@ public class BrightnessController implements ToggleSlider.Listener, MirroredBrig /** ContentObserver to watch brightness */ private class BrightnessObserver extends ContentObserver { + private boolean mObserving = false; + BrightnessObserver(Handler handler) { super(handler); } @@ -124,19 +133,17 @@ public class BrightnessController implements ToggleSlider.Listener, MirroredBrig } public void startObserving() { - final ContentResolver cr = mContext.getContentResolver(); - cr.unregisterContentObserver(this); - cr.registerContentObserver( - BRIGHTNESS_MODE_URI, - false, this, UserHandle.USER_ALL); - mDisplayTracker.addBrightnessChangeCallback(mBrightnessListener, - new HandlerExecutor(mHandler)); + if (!mObserving) { + mObserving = true; + mSecureSettings.registerContentObserverForUser( + BRIGHTNESS_MODE_URI, + false, this, UserHandle.USER_ALL); + } } public void stopObserving() { - final ContentResolver cr = mContext.getContentResolver(); - cr.unregisterContentObserver(this); - mDisplayTracker.removeCallback(mBrightnessListener); + mSecureSettings.unregisterContentObserver(this); + mObserving = false; } } @@ -159,6 +166,8 @@ public class BrightnessController implements ToggleSlider.Listener, MirroredBrig } mBrightnessObserver.startObserving(); + mDisplayTracker.addBrightnessChangeCallback(mBrightnessListener, + new HandlerExecutor(mMainHandler)); mUserTracker.addCallback(mUserChangedCallback, mMainExecutor); // Update the slider and mode before attaching the listener so we don't @@ -166,7 +175,7 @@ public class BrightnessController implements ToggleSlider.Listener, MirroredBrig mUpdateModeRunnable.run(); mUpdateSliderRunnable.run(); - mHandler.sendEmptyMessage(MSG_ATTACH_LISTENER); + mMainHandler.sendEmptyMessage(MSG_ATTACH_LISTENER); } }; @@ -187,9 +196,10 @@ public class BrightnessController implements ToggleSlider.Listener, MirroredBrig } mBrightnessObserver.stopObserving(); + mDisplayTracker.removeCallback(mBrightnessListener); mUserTracker.removeCallback(mUserChangedCallback); - mHandler.sendEmptyMessage(MSG_DETACH_LISTENER); + mMainHandler.sendEmptyMessage(MSG_DETACH_LISTENER); } }; @@ -225,7 +235,7 @@ public class BrightnessController implements ToggleSlider.Listener, MirroredBrig mBrightnessMin = info.brightnessMinimum; // Value is passed as intbits, since this is what the message takes. final int valueAsIntBits = Float.floatToIntBits(info.brightness); - mHandler.obtainMessage(MSG_UPDATE_SLIDER, valueAsIntBits, + mMainHandler.obtainMessage(MSG_UPDATE_SLIDER, valueAsIntBits, inVrMode ? 1 : 0).sendToTarget(); } }; @@ -233,14 +243,14 @@ public class BrightnessController implements ToggleSlider.Listener, MirroredBrig private final IVrStateCallbacks mVrStateCallbacks = new IVrStateCallbacks.Stub() { @Override public void onVrStateChanged(boolean enabled) { - mHandler.obtainMessage(MSG_VR_MODE_CHANGED, enabled ? 1 : 0, 0) + mMainHandler.obtainMessage(MSG_VR_MODE_CHANGED, enabled ? 1 : 0, 0) .sendToTarget(); } }; - private final Handler mHandler = new Handler() { + private final Handler.Callback mHandlerCallback = new Handler.Callback() { @Override - public void handleMessage(Message msg) { + public boolean handleMessage(Message msg) { mExternalChange = true; try { switch (msg.what) { @@ -257,14 +267,18 @@ public class BrightnessController implements ToggleSlider.Listener, MirroredBrig updateVrMode(msg.arg1 != 0); break; default: - super.handleMessage(msg); + return false; + } } finally { mExternalChange = false; } + return true; } }; + private final Handler mMainHandler; + private final UserTracker.Callback mUserChangedCallback = new UserTracker.Callback() { @Override @@ -274,12 +288,17 @@ public class BrightnessController implements ToggleSlider.Listener, MirroredBrig } }; + @AssistedInject public BrightnessController( Context context, - ToggleSlider control, + @Assisted ToggleSlider control, UserTracker userTracker, DisplayTracker displayTracker, + DisplayManager displayManager, + SecureSettings secureSettings, + @Nullable IVrManager iVrManager, @Main Executor mainExecutor, + @Main Looper mainLooper, @Background Handler bgHandler) { mContext = context; mControl = control; @@ -288,22 +307,23 @@ public class BrightnessController implements ToggleSlider.Listener, MirroredBrig mBackgroundHandler = bgHandler; mUserTracker = userTracker; mDisplayTracker = displayTracker; - mBrightnessObserver = new BrightnessObserver(mHandler); - + mSecureSettings = secureSettings; mDisplayId = mContext.getDisplayId(); - PowerManager pm = context.getSystemService(PowerManager.class); + mDisplayManager = displayManager; + mVrManager = iVrManager; - mDisplayManager = context.getSystemService(DisplayManager.class); - mVrManager = IVrManager.Stub.asInterface(ServiceManager.getService( - Context.VR_SERVICE)); + mMainHandler = new Handler(mainLooper, mHandlerCallback); + mBrightnessObserver = new BrightnessObserver(mMainHandler); } public void registerCallbacks() { + mBackgroundHandler.removeCallbacks(mStartListeningRunnable); mBackgroundHandler.post(mStartListeningRunnable); } /** Unregister all call backs, both to and from the controller */ public void unregisterCallbacks() { + mBackgroundHandler.removeCallbacks(mStopListeningRunnable); mBackgroundHandler.post(mStopListeningRunnable); mControlValueInitialized = false; } @@ -418,38 +438,12 @@ public class BrightnessController implements ToggleSlider.Listener, MirroredBrig mSliderAnimator.start(); } + + /** Factory for creating a {@link BrightnessController}. */ - public static class Factory { - private final Context mContext; - private final UserTracker mUserTracker; - private final DisplayTracker mDisplayTracker; - private final Executor mMainExecutor; - private final Handler mBackgroundHandler; - - @Inject - public Factory( - Context context, - UserTracker userTracker, - DisplayTracker displayTracker, - @Main Executor mainExecutor, - @Background Handler bgHandler) { - mContext = context; - mUserTracker = userTracker; - mDisplayTracker = displayTracker; - mMainExecutor = mainExecutor; - mBackgroundHandler = bgHandler; - } - + @AssistedFactory + public interface Factory { /** Create a {@link BrightnessController} */ - public BrightnessController create(ToggleSlider toggleSlider) { - return new BrightnessController( - mContext, - toggleSlider, - mUserTracker, - mDisplayTracker, - mMainExecutor, - mBackgroundHandler); - } + BrightnessController create(ToggleSlider toggleSlider); } - } diff --git a/packages/SystemUI/src/com/android/systemui/settings/brightness/BrightnessDialog.java b/packages/SystemUI/src/com/android/systemui/settings/brightness/BrightnessDialog.java index 182e4569b549f..38b1f14e45de8 100644 --- a/packages/SystemUI/src/com/android/systemui/settings/brightness/BrightnessDialog.java +++ b/packages/SystemUI/src/com/android/systemui/settings/brightness/BrightnessDialog.java @@ -23,7 +23,6 @@ import static android.view.WindowManagerPolicyConstants.EXTRA_FROM_BRIGHTNESS_KE import android.app.Activity; import android.graphics.Rect; import android.os.Bundle; -import android.os.Handler; import android.view.Gravity; import android.view.KeyEvent; import android.view.View; @@ -37,10 +36,7 @@ import com.android.internal.annotations.VisibleForTesting; import com.android.internal.logging.MetricsLogger; import com.android.internal.logging.nano.MetricsProto.MetricsEvent; import com.android.systemui.R; -import com.android.systemui.dagger.qualifiers.Background; import com.android.systemui.dagger.qualifiers.Main; -import com.android.systemui.settings.DisplayTracker; -import com.android.systemui.settings.UserTracker; import com.android.systemui.statusbar.policy.AccessibilityManagerWrapper; import com.android.systemui.util.concurrency.DelayableExecutor; @@ -56,26 +52,21 @@ public class BrightnessDialog extends Activity { private BrightnessController mBrightnessController; private final BrightnessSliderController.Factory mToggleSliderFactory; - private final UserTracker mUserTracker; - private final DisplayTracker mDisplayTracker; + private final BrightnessController.Factory mBrightnessControllerFactory; private final DelayableExecutor mMainExecutor; - private final Handler mBackgroundHandler; private final AccessibilityManagerWrapper mAccessibilityMgr; private Runnable mCancelTimeoutRunnable; @Inject public BrightnessDialog( - UserTracker userTracker, - DisplayTracker displayTracker, - BrightnessSliderController.Factory factory, + BrightnessSliderController.Factory brightnessSliderfactory, + BrightnessController.Factory brightnessControllerFactory, @Main DelayableExecutor mainExecutor, - @Background Handler bgHandler, - AccessibilityManagerWrapper accessibilityMgr) { - mUserTracker = userTracker; - mDisplayTracker = displayTracker; - mToggleSliderFactory = factory; + AccessibilityManagerWrapper accessibilityMgr + ) { + mToggleSliderFactory = brightnessSliderfactory; + mBrightnessControllerFactory = brightnessControllerFactory; mMainExecutor = mainExecutor; - mBackgroundHandler = bgHandler; mAccessibilityMgr = accessibilityMgr; } @@ -121,8 +112,7 @@ public class BrightnessDialog extends Activity { controller.init(); frame.addView(controller.getRootView(), MATCH_PARENT, WRAP_CONTENT); - mBrightnessController = new BrightnessController( - this, controller, mUserTracker, mDisplayTracker, mMainExecutor, mBackgroundHandler); + mBrightnessController = mBrightnessControllerFactory.create(controller); } @Override diff --git a/packages/SystemUI/tests/src/com/android/systemui/qs/QSPanelControllerTest.kt b/packages/SystemUI/tests/src/com/android/systemui/qs/QSPanelControllerTest.kt index a0d8f98a4ad16..9d9d0c7de2ad0 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/qs/QSPanelControllerTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/qs/QSPanelControllerTest.kt @@ -154,6 +154,21 @@ class QSPanelControllerTest : SysuiTestCase() { verify(qsPanel).setCanCollapse(true) } + @Test + fun multipleListeningOnlyCallsBrightnessControllerOnce() { + controller.setListening(true, true) + controller.setListening(true, false) + controller.setListening(true, true) + + verify(brightnessController).registerCallbacks() + + controller.setListening(false, true) + controller.setListening(false, false) + controller.setListening(false, true) + + verify(brightnessController).unregisterCallbacks() + } + private fun setShouldUseSplitShade(shouldUse: Boolean) { testableResources.addOverride(R.bool.config_use_split_notification_shade, shouldUse) } diff --git a/packages/SystemUI/tests/src/com/android/systemui/settings/brightness/BrightnessControllerTest.kt b/packages/SystemUI/tests/src/com/android/systemui/settings/brightness/BrightnessControllerTest.kt new file mode 100644 index 0000000000000..2b7840533df22 --- /dev/null +++ b/packages/SystemUI/tests/src/com/android/systemui/settings/brightness/BrightnessControllerTest.kt @@ -0,0 +1,105 @@ +/* + * Copyright (C) 2023 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.systemui.settings.brightness + +import android.hardware.display.DisplayManager +import android.os.Handler +import android.service.vr.IVrManager +import android.testing.AndroidTestingRunner +import android.testing.TestableLooper +import android.testing.TestableLooper.RunWithLooper +import androidx.test.filters.SmallTest +import com.android.systemui.SysuiTestCase +import com.android.systemui.settings.DisplayTracker +import com.android.systemui.settings.UserTracker +import com.android.systemui.util.concurrency.FakeExecutor +import com.android.systemui.util.mockito.any +import com.android.systemui.util.mockito.mock +import com.android.systemui.util.settings.FakeSettings +import com.android.systemui.util.time.FakeSystemClock +import com.google.common.truth.Truth.assertThat +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.mockito.Mock +import org.mockito.Mockito.verify +import org.mockito.MockitoAnnotations + +@SmallTest +@RunWith(AndroidTestingRunner::class) +@RunWithLooper +class BrightnessControllerTest : SysuiTestCase() { + + private val executor = FakeExecutor(FakeSystemClock()) + private val secureSettings = FakeSettings() + @Mock private lateinit var toggleSlider: ToggleSlider + @Mock private lateinit var userTracker: UserTracker + @Mock private lateinit var displayTracker: DisplayTracker + @Mock private lateinit var displayManager: DisplayManager + @Mock private lateinit var iVrManager: IVrManager + + private lateinit var testableLooper: TestableLooper + + private lateinit var underTest: BrightnessController + + @Before + fun setUp() { + MockitoAnnotations.initMocks(this) + testableLooper = TestableLooper.get(this) + + underTest = + BrightnessController( + context, + toggleSlider, + userTracker, + displayTracker, + displayManager, + secureSettings, + iVrManager, + executor, + mock(), + Handler(testableLooper.looper) + ) + } + + @Test + fun registerCallbacksMultipleTimes_onlyOneRegistration() { + val repeats = 100 + repeat(repeats) { underTest.registerCallbacks() } + val messagesProcessed = testableLooper.processMessagesNonBlocking(repeats) + + verify(displayTracker).addBrightnessChangeCallback(any(), any()) + verify(iVrManager).registerListener(any()) + + assertThat(messagesProcessed).isEqualTo(1) + } + + @Test + fun unregisterCallbacksMultipleTimes_onlyOneUnregistration() { + val repeats = 100 + underTest.registerCallbacks() + testableLooper.processAllMessages() + + repeat(repeats) { underTest.unregisterCallbacks() } + val messagesProcessed = testableLooper.processMessagesNonBlocking(repeats) + + verify(displayTracker).removeCallback(any()) + verify(iVrManager).unregisterListener(any()) + + assertThat(messagesProcessed).isEqualTo(1) + } +} diff --git a/packages/SystemUI/tests/src/com/android/systemui/settings/brightness/BrightnessDialogTest.kt b/packages/SystemUI/tests/src/com/android/systemui/settings/brightness/BrightnessDialogTest.kt index 5c35913f6e209..ed1397ff70132 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/settings/brightness/BrightnessDialogTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/settings/brightness/BrightnessDialogTest.kt @@ -18,7 +18,6 @@ package com.android.systemui.settings.brightness import android.content.Intent import android.graphics.Rect -import android.os.Handler import android.testing.AndroidTestingRunner import android.testing.TestableLooper import android.view.View @@ -29,8 +28,6 @@ import androidx.test.rule.ActivityTestRule import com.android.systemui.R import com.android.systemui.SysuiTestCase import com.android.systemui.activity.SingleActivityFactory -import com.android.systemui.settings.FakeDisplayTracker -import com.android.systemui.settings.UserTracker import com.android.systemui.statusbar.policy.AccessibilityManagerWrapper import com.android.systemui.util.concurrency.DelayableExecutor import com.android.systemui.util.concurrency.FakeExecutor @@ -53,28 +50,24 @@ import org.mockito.MockitoAnnotations @TestableLooper.RunWithLooper class BrightnessDialogTest : SysuiTestCase() { - @Mock private lateinit var userTracker: UserTracker @Mock private lateinit var brightnessSliderControllerFactory: BrightnessSliderController.Factory - @Mock private lateinit var backgroundHandler: Handler @Mock private lateinit var brightnessSliderController: BrightnessSliderController + @Mock private lateinit var brightnessControllerFactory: BrightnessController.Factory + @Mock private lateinit var brightnessController: BrightnessController @Mock private lateinit var accessibilityMgr: AccessibilityManagerWrapper private val clock = FakeSystemClock() private val mainExecutor = FakeExecutor(clock) - private var displayTracker = FakeDisplayTracker(mContext) - @Rule @JvmField var activityRule = ActivityTestRule( /* activityFactory= */ SingleActivityFactory { TestDialog( - userTracker, - displayTracker, brightnessSliderControllerFactory, + brightnessControllerFactory, mainExecutor, - backgroundHandler, accessibilityMgr ) }, @@ -88,6 +81,7 @@ class BrightnessDialogTest : SysuiTestCase() { `when`(brightnessSliderControllerFactory.create(any(), any())) .thenReturn(brightnessSliderController) `when`(brightnessSliderController.rootView).thenReturn(View(context)) + `when`(brightnessControllerFactory.create(any())).thenReturn(brightnessController) } @After @@ -178,19 +172,15 @@ class BrightnessDialogTest : SysuiTestCase() { } class TestDialog( - userTracker: UserTracker, - displayTracker: FakeDisplayTracker, brightnessSliderControllerFactory: BrightnessSliderController.Factory, + brightnessControllerFactory: BrightnessController.Factory, mainExecutor: DelayableExecutor, - backgroundHandler: Handler, accessibilityMgr: AccessibilityManagerWrapper ) : BrightnessDialog( - userTracker, - displayTracker, brightnessSliderControllerFactory, + brightnessControllerFactory, mainExecutor, - backgroundHandler, accessibilityMgr ) }