Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit d25127dd authored by Jernej Virag's avatar Jernej Virag
Browse files

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
parent 225465e0

packages/SystemUI/src/com/android/systemui/media/controls/ui/MediaCarouselController.kt

+17 −0
Original line number Diff line number Diff line
@@ -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() {

packages/SystemUI/src/com/android/systemui/media/controls/ui/MediaControlPanel.java

+4 −13
Original line number Diff line number Diff line
@@ -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);

packages/SystemUI/tests/src/com/android/systemui/media/controls/ui/MediaCarouselControllerTest.kt

+20 −0
Original line number Diff line number Diff line
@@ -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<MediaDataManager.Listener>

    @Captor
@@ -120,6 +124,7 @@ class MediaCarouselControllerTest : SysuiTestCase() { 
    @Captor lateinit var visualStabilityCallback: ArgumentCaptor<OnReorderingAllowedListener>

    @Captor lateinit var keyguardCallback: ArgumentCaptor<KeyguardUpdateMonitorCallback>

    @Captor lateinit var hostStateCallback: ArgumentCaptor<MediaHostStatesManager.Callback>

    @Captor lateinit var settingsObserverCaptor: ArgumentCaptor<ContentObserver>



    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.

     *

packages/SystemUI/tests/src/com/android/systemui/media/controls/ui/MediaControlPanelTest.kt

+2 −11
Original line number Diff line number Diff line
@@ -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<ContentObserver>



    @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()