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

Commit 7b27cbe7 authored by Nicolo' Mazzucato's avatar Nicolo' Mazzucato
Browse files

Fix config change race in (legacy) shade window 

When ShadeWindowGoesAround flag is on, the shade window uses a separate context (windowContext), that is always associated with the display the shade is at.

When the configuration changes as result of fold/unfold, first the global configuration (associated with the default display) changes, and only afterwards resources used by the shade context changes as well.

So here there was a race between:
- SystemUiApplication: it was receving the config change for the default display, and propagating it to CentralSurfaceImpl that was propagating it to mQSPanelController and mShadeSurface. Those classes were getting the "split shade" boolean at this point using shade window resources, but those were not updated yet with the new configuration!
- WindowContext: The window context resources were updated soon after the global config, but later than SystemUiApplication was receiving the callback for the global config.

-> this resulted in mQSPanelController and mShadeSurface always having the opposite "splitShade" value after unfolding (as they were both reading it from the WindowContext resources, that were updated later)

After this change, mQSPanelController and mShadeSurface receive the config change only from the @ShadeDisplayAware ConfigurationController. From there, it's guaranteed resources have been updated before receiving the onConfigChanged call.

Note that this issue doesn't happen with flexiglass on, and the flag is supposed to ship only with flexiglass.

Bug: 362719719
Bug: 391929792
Test: CentralSurfacesImpl + fold/unfold comet with flexiglass off + QSPanelControllerBaseTest
Flag: com.android.systemui.shade_window_goes_around
Change-Id: Id25536661ac876863c33e2b149b1ee82aa3b929a
parent d635960c

packages/SystemUI/multivalentTests/src/com/android/systemui/qs/QSPanelControllerBaseSceneContainerTest.kt

+8 −2
Original line number Diff line number Diff line
@@ -36,6 +36,7 @@ import com.android.systemui.lifecycle.InstantTaskExecutorRule 
import com.android.systemui.media.controls.ui.view.MediaHost

import com.android.systemui.qs.customize.QSCustomizerController

import com.android.systemui.qs.logging.QSLogger

import com.android.systemui.statusbar.policy.ConfigurationController

import com.android.systemui.statusbar.policy.SplitShadeStateController

import com.android.systemui.testKosmos

import com.google.common.truth.Truth.assertThat
@@ -80,6 +81,7 @@ class QSPanelControllerBaseSceneContainerTest : SysuiTestCase() { 
    private val configuration = Configuration()

    @Mock private lateinit var viewTreeObserver: ViewTreeObserver

    @Mock private lateinit var mediaHost: MediaHost

    @Mock private lateinit var configurationController: ConfigurationController



    private var isSplitShade = false

    private val splitShadeStateController =
@@ -258,6 +260,7 @@ class QSPanelControllerBaseSceneContainerTest : SysuiTestCase() { 
            splitShadeStateController,

            longPressEffectProvider,

            mediaVisible,

            configurationController,

        )

    }
@@ -272,7 +275,8 @@ class QSPanelControllerBaseSceneContainerTest : SysuiTestCase() { 
        dumpManager: DumpManager,

        splitShadeStateController: SplitShadeStateController,

        longPressEffectProvider: Provider<QSLongPressEffect>,

        private val mediaVisibleFlow: StateFlow<Boolean>

        private val mediaVisibleFlow: StateFlow<Boolean>,

        configurationController: ConfigurationController,

    ) :

        QSPanelControllerBase<QSPanel>(

            view,
@@ -285,12 +289,14 @@ class QSPanelControllerBaseSceneContainerTest : SysuiTestCase() { 
            qsLogger,

            dumpManager,

            splitShadeStateController,

            longPressEffectProvider

            longPressEffectProvider,

            configurationController,

        ) {



        init {

            whenever(view.dumpableTag).thenReturn(hashCode().toString())

        }



        override fun getMediaVisibleFlow(): StateFlow<Boolean> {

            return mediaVisibleFlow

        }

packages/SystemUI/multivalentTests/src/com/android/systemui/qs/QSPanelControllerBaseTest.java

+19 −1
Original line number Diff line number Diff line
@@ -40,6 +40,7 @@ import static kotlinx.coroutines.flow.StateFlowKt.MutableStateFlow; 


import android.content.res.Configuration;

import android.content.res.Resources;

import android.platform.test.annotations.EnableFlags;

import android.platform.test.flag.junit.FlagsParameterization;

import android.testing.TestableLooper.RunWithLooper;

import android.view.ContextThemeWrapper;
@@ -51,6 +52,7 @@ import com.android.internal.logging.MetricsLogger; 
import com.android.internal.logging.UiEventLogger;

import com.android.internal.logging.nano.MetricsProto.MetricsEvent;

import com.android.internal.logging.testing.UiEventLoggerFake;

import com.android.systemui.Flags;

import com.android.systemui.SysuiTestCase;

import com.android.systemui.dump.DumpManager;

import com.android.systemui.flags.DisableSceneContainer;
@@ -64,6 +66,7 @@ import com.android.systemui.qs.logging.QSLogger; 
import com.android.systemui.qs.tileimpl.QSTileImpl;

import com.android.systemui.res.R;

import com.android.systemui.scene.shared.flag.SceneContainerFlag;

import com.android.systemui.statusbar.policy.ConfigurationController;

import com.android.systemui.statusbar.policy.ResourcesSplitShadeStateController;

import com.android.systemui.util.animation.DisappearParameters;
@@ -134,6 +137,8 @@ public class QSPanelControllerBaseTest extends SysuiTestCase { 
    Runnable mHorizontalLayoutListener;

    @Mock

    private ViewTreeObserver mViewTreeObserver;

    @Mock

    ConfigurationController mConfigurationController;



    private boolean mPagedTileLayoutListening = false;
@@ -151,7 +156,7 @@ public class QSPanelControllerBaseTest extends SysuiTestCase { 
            super(view, host, qsCustomizerController, usingMediaPlayer(),

                    mediaHost, metricsLogger, uiEventLogger,

                    qsLogger, dumpManager, new ResourcesSplitShadeStateController(),

                    mLongPressEffectProvider);

                    mLongPressEffectProvider, mConfigurationController);

        }



        private MutableStateFlow<Boolean> mMediaVisible = MutableStateFlow(false);
@@ -633,6 +638,19 @@ public class QSPanelControllerBaseTest extends SysuiTestCase { 
        );

    }



    @Test

    @EnableFlags(Flags.FLAG_SHADE_WINDOW_GOES_AROUND)

    public void onViewAttached_registersConfigListener() {

        reset(mConfigurationController);

        mController.onViewAttached();



        verify(mConfigurationController).addCallback(any());



        mController.onViewDetached();



        verify(mConfigurationController).removeCallback(any());

    }





    private boolean usingMediaPlayer() {

        return !SceneContainerFlag.isEnabled();

packages/SystemUI/multivalentTests/src/com/android/systemui/qs/QSPanelControllerTest.kt

+3 −0
Original line number Diff line number Diff line
@@ -22,6 +22,7 @@ import com.android.systemui.scene.shared.flag.SceneContainerFlag 
import com.android.systemui.settings.brightness.BrightnessController

import com.android.systemui.settings.brightness.BrightnessSliderController

import com.android.systemui.statusbar.phone.StatusBarKeyguardViewManager

import com.android.systemui.statusbar.policy.ConfigurationController

import com.android.systemui.statusbar.policy.ResourcesSplitShadeStateController

import com.android.systemui.tuner.TunerService

import com.google.common.truth.Truth.assertThat
@@ -66,6 +67,7 @@ class QSPanelControllerTest : SysuiTestCase() { 
    @Mock private lateinit var pagedTileLayout: PagedTileLayout

    @Mock private lateinit var longPressEffectProvider: Provider<QSLongPressEffect>

    @Mock private lateinit var mediaCarouselInteractor: MediaCarouselInteractor

    @Mock private lateinit var configurationController: ConfigurationController



    private val usingMediaPlayer: Boolean by lazy { !SceneContainerFlag.isEnabled }
@@ -108,6 +110,7 @@ class QSPanelControllerTest : SysuiTestCase() { 
                ResourcesSplitShadeStateController(),

                longPressEffectProvider,

                mediaCarouselInteractor,

                configurationController,

            )

    }

packages/SystemUI/multivalentTests/src/com/android/systemui/qs/QuickQSPanelControllerTest.kt

+5 −0
Original line number Diff line number Diff line
@@ -32,6 +32,7 @@ import com.android.systemui.plugins.qs.QSTile 
import com.android.systemui.qs.customize.QSCustomizerController

import com.android.systemui.qs.logging.QSLogger

import com.android.systemui.res.R

import com.android.systemui.statusbar.policy.ConfigurationController

import com.android.systemui.statusbar.policy.ResourcesSplitShadeStateController

import com.android.systemui.util.leak.RotationUtils

import javax.inject.Provider
@@ -64,6 +65,7 @@ class QuickQSPanelControllerTest : SysuiTestCase() { 
    @Captor private lateinit var captor: ArgumentCaptor<QSPanel.OnConfigurationChangedListener>

    @Mock private lateinit var longPressEffectProvider: Provider<QSLongPressEffect>

    @Mock private lateinit var mediaCarouselInteractor: MediaCarouselInteractor

    @Mock private lateinit var configurationController: ConfigurationController



    private val usingMediaPlayer: Boolean

        get() = false
@@ -100,6 +102,7 @@ class QuickQSPanelControllerTest : SysuiTestCase() { 
                dumpManager,

                longPressEffectProvider,

                mediaCarouselInteractor,

                configurationController,

            )



        controller.init()
@@ -171,6 +174,7 @@ class QuickQSPanelControllerTest : SysuiTestCase() { 
        dumpManager: DumpManager,

        longPressEffectProvider: Provider<QSLongPressEffect>,

        mediaCarouselInteractor: MediaCarouselInteractor,

        configurationController: ConfigurationController,

    ) :

        QuickQSPanelController(

            view,
@@ -186,6 +190,7 @@ class QuickQSPanelControllerTest : SysuiTestCase() { 
            ResourcesSplitShadeStateController(),

            longPressEffectProvider,

            mediaCarouselInteractor,

            configurationController,

        ) {



        private var rotation = RotationUtils.ROTATION_NONE

packages/SystemUI/src/com/android/systemui/qs/QSPanelController.java

+5 −2
Original line number Diff line number Diff line
@@ -43,7 +43,9 @@ import com.android.systemui.settings.brightness.BrightnessController; 
import com.android.systemui.settings.brightness.BrightnessMirrorHandler;

import com.android.systemui.settings.brightness.BrightnessSliderController;

import com.android.systemui.settings.brightness.MirrorController;

import com.android.systemui.shade.ShadeDisplayAware;

import com.android.systemui.statusbar.phone.StatusBarKeyguardViewManager;

import com.android.systemui.statusbar.policy.ConfigurationController;

import com.android.systemui.statusbar.policy.SplitShadeStateController;

import com.android.systemui.tuner.TunerService;
@@ -101,10 +103,11 @@ public class QSPanelController extends QSPanelControllerBase<QSPanel> { 
            StatusBarKeyguardViewManager statusBarKeyguardViewManager,

            SplitShadeStateController splitShadeStateController,

            Provider<QSLongPressEffect> longPRessEffectProvider,

            MediaCarouselInteractor mediaCarouselInteractor) {

            MediaCarouselInteractor mediaCarouselInteractor,

            @ShadeDisplayAware ConfigurationController configurationController) {

        super(view, qsHost, qsCustomizerController, usingMediaPlayer, mediaHost,

                metricsLogger, uiEventLogger, qsLogger, dumpManager, splitShadeStateController,

                longPRessEffectProvider);

                longPRessEffectProvider, configurationController);

        mTunerService = tunerService;

        mQsCustomizerController = qsCustomizerController;

        mQsTileRevealControllerFactory = qsTileRevealControllerFactory;