From d25127dd56075ea14e6ce76ee416fd96e317e988 Mon Sep 17 00:00:00 2001 From: Jernej Virag Date: Wed, 21 Jun 2023 16:20:40 +0200 Subject: [PATCH] Fix memory leak of MediaControlPanel MediaControlPanel instances were being leak through the ContentObserver registration. This fixes the leak by moving ContentObserver into owning singleton which then notifies active MediaControlPanel instances. Bug: 288244587 Test: updated unit tests Change-Id: Idb1f3a750e6e87c56c645f53ce678a62e7181f11 --- .../controls/ui/MediaCarouselController.kt | 17 ++++++++++++++++ .../media/controls/ui/MediaControlPanel.java | 17 ++++------------ .../ui/MediaCarouselControllerTest.kt | 20 +++++++++++++++++++ .../controls/ui/MediaControlPanelTest.kt | 13 ++---------- 4 files changed, 43 insertions(+), 24 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/media/controls/ui/MediaCarouselController.kt b/packages/SystemUI/src/com/android/systemui/media/controls/ui/MediaCarouselController.kt index 1e9a466ccdce8..70b5e75e6048d 100644 --- a/packages/SystemUI/src/com/android/systemui/media/controls/ui/MediaCarouselController.kt +++ b/packages/SystemUI/src/com/android/systemui/media/controls/ui/MediaCarouselController.kt @@ -21,6 +21,8 @@ import android.content.Context import android.content.Intent import android.content.res.ColorStateList import android.content.res.Configuration +import android.database.ContentObserver +import android.provider.Settings import android.provider.Settings.ACTION_MEDIA_CONTROLS_SETTINGS import android.util.Log import android.util.MathUtils @@ -64,6 +66,7 @@ import com.android.systemui.util.Utils import com.android.systemui.util.animation.UniqueObjectHostView import com.android.systemui.util.animation.requiresRemeasuring import com.android.systemui.util.concurrency.DelayableExecutor +import com.android.systemui.util.settings.GlobalSettings import com.android.systemui.util.time.SystemClock import com.android.systemui.util.traceSection import java.io.PrintWriter @@ -105,6 +108,7 @@ constructor( private val mediaFlags: MediaFlags, private val keyguardUpdateMonitor: KeyguardUpdateMonitor, private val keyguardTransitionInteractor: KeyguardTransitionInteractor, + private val globalSettings: GlobalSettings, ) : Dumpable { /** The current width of the carousel */ var currentCarouselWidth: Int = 0 @@ -169,6 +173,13 @@ constructor( private var carouselLocale: Locale? = null + private val animationScaleObserver: ContentObserver = + object : ContentObserver(null) { + override fun onChange(selfChange: Boolean) { + MediaPlayerData.players().forEach { it.updateAnimatorDurationScale() } + } + } + /** Whether the media card currently has the "expanded" layout */ @VisibleForTesting var currentlyExpanded = true @@ -529,6 +540,12 @@ constructor( listenForAnyStateToGoneKeyguardTransition(this) } } + + // Notifies all active players about animation scale changes. + globalSettings.registerContentObserver( + Settings.Global.getUriFor(Settings.Global.ANIMATOR_DURATION_SCALE), + animationScaleObserver + ) } private fun inflateSettingsButton() { diff --git a/packages/SystemUI/src/com/android/systemui/media/controls/ui/MediaControlPanel.java b/packages/SystemUI/src/com/android/systemui/media/controls/ui/MediaControlPanel.java index 0819d0d36e9ad..35082fd5122a3 100644 --- a/packages/SystemUI/src/com/android/systemui/media/controls/ui/MediaControlPanel.java +++ b/packages/SystemUI/src/com/android/systemui/media/controls/ui/MediaControlPanel.java @@ -34,7 +34,6 @@ import android.content.pm.PackageManager; import android.content.res.ColorStateList; import android.content.res.Configuration; import android.content.res.Resources; -import android.database.ContentObserver; import android.graphics.Bitmap; import android.graphics.BlendMode; import android.graphics.Color; @@ -252,13 +251,6 @@ public class MediaControlPanel { private boolean mWasPlaying = false; private boolean mButtonClicked = false; - private ContentObserver mAnimationScaleObserver = new ContentObserver(null) { - @Override - public void onChange(boolean selfChange) { - updateAnimatorDurationScale(); - } - }; - /** * Initialize a new control panel * @@ -318,10 +310,6 @@ public class MediaControlPanel { mFeatureFlags = featureFlags; mGlobalSettings = globalSettings; - mGlobalSettings.registerContentObserver( - Settings.Global.getUriFor(Settings.Global.ANIMATOR_DURATION_SCALE), - mAnimationScaleObserver - ); updateAnimatorDurationScale(); } @@ -405,7 +393,10 @@ public class MediaControlPanel { updateSeekBarVisibility(); } - private void updateAnimatorDurationScale() { + /** + * Reloads animator duration scale. + */ + void updateAnimatorDurationScale() { if (mSeekBarObserver != null) { mSeekBarObserver.setAnimationEnabled( mGlobalSettings.getFloat(Settings.Global.ANIMATOR_DURATION_SCALE, 1f) > 0f); diff --git a/packages/SystemUI/tests/src/com/android/systemui/media/controls/ui/MediaCarouselControllerTest.kt b/packages/SystemUI/tests/src/com/android/systemui/media/controls/ui/MediaCarouselControllerTest.kt index 2aff90c471891..5b8272b04bfb7 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/media/controls/ui/MediaCarouselControllerTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/media/controls/ui/MediaCarouselControllerTest.kt @@ -19,7 +19,9 @@ package com.android.systemui.media.controls.ui import android.app.PendingIntent import android.content.res.ColorStateList import android.content.res.Configuration +import android.database.ContentObserver import android.os.LocaleList +import android.provider.Settings import android.testing.AndroidTestingRunner import android.testing.TestableLooper import android.util.MathUtils.abs @@ -56,6 +58,7 @@ import com.android.systemui.util.concurrency.DelayableExecutor import com.android.systemui.util.mockito.any import com.android.systemui.util.mockito.capture import com.android.systemui.util.mockito.eq +import com.android.systemui.util.settings.GlobalSettings import com.android.systemui.util.time.FakeSystemClock import java.util.Locale import javax.inject.Provider @@ -113,6 +116,7 @@ class MediaCarouselControllerTest : SysuiTestCase() { @Mock lateinit var mediaFlags: MediaFlags @Mock lateinit var keyguardUpdateMonitor: KeyguardUpdateMonitor @Mock lateinit var keyguardTransitionInteractor: KeyguardTransitionInteractor + @Mock lateinit var globalSettings: GlobalSettings private lateinit var transitionRepository: FakeKeyguardTransitionRepository @Captor lateinit var listener: ArgumentCaptor @Captor @@ -120,6 +124,7 @@ class MediaCarouselControllerTest : SysuiTestCase() { @Captor lateinit var visualStabilityCallback: ArgumentCaptor @Captor lateinit var keyguardCallback: ArgumentCaptor @Captor lateinit var hostStateCallback: ArgumentCaptor + @Captor lateinit var settingsObserverCaptor: ArgumentCaptor private val clock = FakeSystemClock() private lateinit var mediaCarouselController: MediaCarouselController @@ -148,6 +153,7 @@ class MediaCarouselControllerTest : SysuiTestCase() { mediaFlags, keyguardUpdateMonitor, KeyguardTransitionInteractor(transitionRepository, TestScope().backgroundScope), + globalSettings ) verify(configurationController).addCallback(capture(configListener)) verify(mediaDataManager).addListener(capture(listener)) @@ -160,6 +166,11 @@ class MediaCarouselControllerTest : SysuiTestCase() { whenever(mediaDataManager.smartspaceMediaData).thenReturn(smartspaceMediaData) whenever(mediaFlags.isPersistentSsCardEnabled()).thenReturn(false) MediaPlayerData.clear() + verify(globalSettings) + .registerContentObserver( + eq(Settings.Global.getUriFor(Settings.Global.ANIMATOR_DURATION_SCALE)), + settingsObserverCaptor.capture() + ) } @Test @@ -873,6 +884,15 @@ class MediaCarouselControllerTest : SysuiTestCase() { assertTrue(stateUpdated) } + @Test + fun testAnimationScaleChanged_mediaControlPanelsNotified() { + MediaPlayerData.addMediaPlayer("key", DATA, panel, clock, isSsReactivated = false) + + globalSettings.putFloat(Settings.Global.ANIMATOR_DURATION_SCALE, 0f) + settingsObserverCaptor.value!!.onChange(false) + verify(panel).updateAnimatorDurationScale() + } + /** * Helper method when a configuration change occurs. * diff --git a/packages/SystemUI/tests/src/com/android/systemui/media/controls/ui/MediaControlPanelTest.kt b/packages/SystemUI/tests/src/com/android/systemui/media/controls/ui/MediaControlPanelTest.kt index f6075add6afe9..f902be3c1a29a 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/media/controls/ui/MediaControlPanelTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/media/controls/ui/MediaControlPanelTest.kt @@ -25,7 +25,6 @@ import android.content.Intent import android.content.pm.ApplicationInfo import android.content.pm.PackageManager import android.content.res.Configuration -import android.database.ContentObserver import android.graphics.Bitmap import android.graphics.Canvas import android.graphics.Color @@ -113,7 +112,6 @@ import org.junit.runner.RunWith import org.mockito.ArgumentCaptor import org.mockito.ArgumentMatchers.anyInt import org.mockito.ArgumentMatchers.anyLong -import org.mockito.Captor import org.mockito.Mock import org.mockito.Mockito.anyString import org.mockito.Mockito.mock @@ -239,7 +237,6 @@ public class MediaControlPanelTest : SysuiTestCase() { this.set(Flags.MEDIA_RECOMMENDATION_CARD_UPDATE, false) } @Mock private lateinit var globalSettings: GlobalSettings - @Captor private lateinit var settingsObserverCaptor: ArgumentCaptor @JvmField @Rule val mockito = MockitoJUnit.rule() @@ -281,7 +278,7 @@ public class MediaControlPanelTest : SysuiTestCase() { lockscreenUserManager, broadcastDialogController, fakeFeatureFlag, - globalSettings, + globalSettings ) { override fun loadAnimator( animId: Int, @@ -292,12 +289,6 @@ public class MediaControlPanelTest : SysuiTestCase() { } } - verify(globalSettings) - .registerContentObserver( - eq(Settings.Global.getUriFor(Settings.Global.ANIMATOR_DURATION_SCALE)), - settingsObserverCaptor.capture() - ) - initGutsViewHolderMocks() initMediaViewHolderMocks() @@ -986,7 +977,7 @@ public class MediaControlPanelTest : SysuiTestCase() { // When the setting changes, globalSettings.putFloat(Settings.Global.ANIMATOR_DURATION_SCALE, 0f) - settingsObserverCaptor.value!!.onChange(false) + player.updateAnimatorDurationScale() // Then the seekbar is set to not animate assertThat(seekBarObserver.animationEnabled).isFalse()