From e9587bb557aaf3513dcf8d5ba4cbaeb46460025a Mon Sep 17 00:00:00 2001 From: Selim Cinek Date: Thu, 20 May 2021 19:06:44 +0200 Subject: [PATCH] Fixed an issue where the keyguard media visibility was wrong Because we were overriding the visibility to avoid some bugs, this accidentally had the effect that hidden media players would become visible. These visibility overrides were masking an issue where the hostview wasn't attached to neither of the containers, leading to the host not receiving any updates. The mediaHost was waiting until being attached to start listening, which is now also changed to avoid similar future issues. Fixes: 188530838 Test: atest SystemUITests Change-Id: I8f5aad143713c057cec068f42b5f68cffc370861 --- .../systemui/media/KeyguardMediaController.kt | 122 +++++++++++------- .../com/android/systemui/media/MediaHost.kt | 31 +++-- .../NotificationPanelViewController.java | 3 +- .../media/KeyguardMediaControllerTest.kt | 53 ++++++-- 4 files changed, 142 insertions(+), 67 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/media/KeyguardMediaController.kt b/packages/SystemUI/src/com/android/systemui/media/KeyguardMediaController.kt index 241c2a939f9a1..77d789292e5e0 100644 --- a/packages/SystemUI/src/com/android/systemui/media/KeyguardMediaController.kt +++ b/packages/SystemUI/src/com/android/systemui/media/KeyguardMediaController.kt @@ -16,16 +16,22 @@ package com.android.systemui.media +import android.content.Context +import android.content.res.Configuration import android.view.View import android.view.ViewGroup +import androidx.annotation.VisibleForTesting import com.android.systemui.dagger.SysUISingleton import com.android.systemui.media.dagger.MediaModule.KEYGUARD import com.android.systemui.plugins.statusbar.StatusBarStateController +import com.android.systemui.statusbar.FeatureFlags import com.android.systemui.statusbar.NotificationLockscreenUserManager import com.android.systemui.statusbar.StatusBarState import com.android.systemui.statusbar.SysuiStatusBarStateController import com.android.systemui.statusbar.notification.stack.MediaHeaderView import com.android.systemui.statusbar.phone.KeyguardBypassController +import com.android.systemui.statusbar.policy.ConfigurationController +import com.android.systemui.util.Utils import javax.inject.Inject import javax.inject.Named @@ -38,7 +44,10 @@ class KeyguardMediaController @Inject constructor( @param:Named(KEYGUARD) private val mediaHost: MediaHost, private val bypassController: KeyguardBypassController, private val statusBarStateController: SysuiStatusBarStateController, - private val notifLockscreenUserManager: NotificationLockscreenUserManager + private val notifLockscreenUserManager: NotificationLockscreenUserManager, + private val featureFlags: FeatureFlags, + private val context: Context, + configurationController: ConfigurationController ) { init { @@ -46,12 +55,10 @@ class KeyguardMediaController @Inject constructor( override fun onStateChanged(newState: Int) { refreshMediaPosition() } - - override fun onDozingChanged(isDozing: Boolean) { - if (!isDozing) { - mediaHost.visible = true - refreshMediaPosition() - } + }) + configurationController.addCallback(object : ConfigurationController.ConfigurationListener { + override fun onConfigChanged(newConfig: Configuration?) { + updateResources() } }) @@ -62,8 +69,24 @@ class KeyguardMediaController @Inject constructor( // Let's now initialize this view, which also creates the host view for us. mediaHost.init(MediaHierarchyManager.LOCATION_LOCKSCREEN) + updateResources() } + private fun updateResources() { + useSplitShade = Utils.shouldUseSplitNotificationShade(featureFlags, context.resources) + } + + @VisibleForTesting + var useSplitShade = false + set(value) { + if (field == value) { + return + } + field = value + reattachHostView() + refreshMediaPosition() + } + /** * Is the media player visible? */ @@ -78,34 +101,63 @@ class KeyguardMediaController @Inject constructor( var singlePaneContainer: MediaHeaderView? = null private set private var splitShadeContainer: ViewGroup? = null - private var useSplitShadeContainer: () -> Boolean = { false } /** * Attaches media container in single pane mode, situated at the top of the notifications list */ fun attachSinglePaneContainer(mediaView: MediaHeaderView?) { + val needsListener = singlePaneContainer == null singlePaneContainer = mediaView + if (needsListener) { + // On reinflation we don't want to add another listener + mediaHost.addVisibilityChangeListener(this::onMediaHostVisibilityChanged) + } + reattachHostView() + onMediaHostVisibilityChanged(mediaHost.visible) + } - // Required to show it for the first time, afterwards visibility is managed automatically - mediaHost.visible = true - mediaHost.addVisibilityChangeListener { visible -> - refreshMediaPosition() - if (visible) { - mediaHost.hostView.layoutParams.apply { - height = ViewGroup.LayoutParams.WRAP_CONTENT - width = ViewGroup.LayoutParams.MATCH_PARENT - } + /** + * Called whenever the media hosts visibility changes + */ + private fun onMediaHostVisibilityChanged(visible: Boolean) { + refreshMediaPosition() + if (visible) { + mediaHost.hostView.layoutParams.apply { + height = ViewGroup.LayoutParams.WRAP_CONTENT + width = ViewGroup.LayoutParams.MATCH_PARENT } } - refreshMediaPosition() } /** * Attaches media container in split shade mode, situated to the left of notifications */ - fun attachSplitShadeContainer(container: ViewGroup, useContainer: () -> Boolean) { + fun attachSplitShadeContainer(container: ViewGroup) { splitShadeContainer = container - useSplitShadeContainer = useContainer + reattachHostView() + refreshMediaPosition() + } + + private fun reattachHostView() { + val inactiveContainer: ViewGroup? + val activeContainer: ViewGroup? + if (useSplitShade) { + activeContainer = splitShadeContainer + inactiveContainer = singlePaneContainer + } else { + inactiveContainer = splitShadeContainer + activeContainer = singlePaneContainer + } + if (inactiveContainer?.childCount == 1) { + inactiveContainer.removeAllViews() + } + if (activeContainer?.childCount == 0) { + // Detach the hostView from its parent view if exists + mediaHost.hostView.parent?.let { + (it as? ViewGroup)?.removeView(mediaHost.hostView) + } + activeContainer.addView(mediaHost.hostView) + } } fun refreshMediaPosition() { @@ -124,35 +176,17 @@ class KeyguardMediaController @Inject constructor( } private fun showMediaPlayer() { - if (useSplitShadeContainer()) { - showMediaPlayer( - activeContainer = splitShadeContainer, - inactiveContainer = singlePaneContainer) + if (useSplitShade) { + setVisibility(splitShadeContainer, View.VISIBLE) + setVisibility(singlePaneContainer, View.GONE) } else { - showMediaPlayer( - activeContainer = singlePaneContainer, - inactiveContainer = splitShadeContainer) + setVisibility(singlePaneContainer, View.VISIBLE) + setVisibility(splitShadeContainer, View.GONE) } } - private fun showMediaPlayer(activeContainer: ViewGroup?, inactiveContainer: ViewGroup?) { - if (inactiveContainer?.childCount == 1) { - inactiveContainer.removeAllViews() - } - // might be called a few times for the same view, no need to add hostView again - if (activeContainer?.childCount == 0) { - // Detach the hostView from its parent view if exists - mediaHost.hostView.parent ?.let { - (it as? ViewGroup)?.removeView(mediaHost.hostView) - } - activeContainer.addView(mediaHost.hostView) - } - setVisibility(activeContainer, View.VISIBLE) - setVisibility(inactiveContainer, View.GONE) - } - private fun hideMediaPlayer() { - if (useSplitShadeContainer()) { + if (useSplitShade) { setVisibility(splitShadeContainer, View.GONE) } else { setVisibility(singlePaneContainer, View.GONE) diff --git a/packages/SystemUI/src/com/android/systemui/media/MediaHost.kt b/packages/SystemUI/src/com/android/systemui/media/MediaHost.kt index 7402107672937..fe20dcbe85641 100644 --- a/packages/SystemUI/src/com/android/systemui/media/MediaHost.kt +++ b/packages/SystemUI/src/com/android/systemui/media/MediaHost.kt @@ -27,6 +27,11 @@ class MediaHost constructor( private var inited: Boolean = false + /** + * Are we listening to media data changes? + */ + private var listeningToMediaData = false + /** * Get the current bounds on the screen. This makes sure the state is fresh and up to date */ @@ -97,18 +102,17 @@ class MediaHost constructor( this.location = location hostView = mediaHierarchyManager.register(this) + // Listen by default, as the host might not be attached by our clients, until + // they get a visibility change. We still want to stay up to date in that case! + setListeningToMediaData(true) hostView.addOnAttachStateChangeListener(object : OnAttachStateChangeListener { override fun onViewAttachedToWindow(v: View?) { - // we should listen to the combined state change, since otherwise there might - // be a delay until the views and the controllers are initialized, leaving us - // with either a blank view or the controllers not yet initialized and the - // measuring wrong - mediaDataManager.addListener(listener) + setListeningToMediaData(true) updateViewVisibility() } override fun onViewDetachedFromWindow(v: View?) { - mediaDataManager.removeListener(listener) + setListeningToMediaData(false) } }) @@ -135,8 +139,19 @@ class MediaHost constructor( updateViewVisibility() } + private fun setListeningToMediaData(listen: Boolean) { + if (listen != listeningToMediaData) { + listeningToMediaData = listen + if (listen) { + mediaDataManager.addListener(listener) + } else { + mediaDataManager.removeListener(listener) + } + } + } + private fun updateViewVisibility() { - visible = if (showsOnlyActiveMedia) { + state.visible = if (showsOnlyActiveMedia) { mediaDataManager.hasActiveMedia() } else { mediaDataManager.hasAnyMedia() @@ -300,7 +315,7 @@ interface MediaHostState { /** * If the view should be VISIBLE or GONE. */ - var visible: Boolean + val visible: Boolean /** * Does this host need any falsing protection? diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/phone/NotificationPanelViewController.java b/packages/SystemUI/src/com/android/systemui/statusbar/phone/NotificationPanelViewController.java index 9d8a9bfafe49c..e4e1be23f95d0 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/phone/NotificationPanelViewController.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/phone/NotificationPanelViewController.java @@ -1085,8 +1085,7 @@ public class NotificationPanelViewController extends PanelViewController { } private void attachSplitShadeMediaPlayerContainer(FrameLayout container) { - mKeyguardMediaController.attachSplitShadeContainer(container, - () -> mShouldUseSplitNotificationShade); + mKeyguardMediaController.attachSplitShadeContainer(container); } private void initBottomArea() { diff --git a/packages/SystemUI/tests/src/com/android/systemui/media/KeyguardMediaControllerTest.kt b/packages/SystemUI/tests/src/com/android/systemui/media/KeyguardMediaControllerTest.kt index 0d67d664c0789..25ae67baa9bed 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/media/KeyguardMediaControllerTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/media/KeyguardMediaControllerTest.kt @@ -22,13 +22,16 @@ import android.view.View.VISIBLE import android.widget.FrameLayout import androidx.test.filters.SmallTest import com.android.systemui.SysuiTestCase +import com.android.systemui.statusbar.FeatureFlags import com.android.systemui.statusbar.NotificationLockscreenUserManager import com.android.systemui.statusbar.StatusBarState import com.android.systemui.statusbar.SysuiStatusBarStateController import com.android.systemui.statusbar.notification.stack.MediaHeaderView import com.android.systemui.statusbar.phone.KeyguardBypassController +import com.android.systemui.statusbar.policy.ConfigurationController import com.android.systemui.util.animation.UniqueObjectHostView import com.google.common.truth.Truth.assertThat +import junit.framework.Assert.assertTrue import org.junit.Before import org.junit.Rule import org.junit.Test @@ -48,11 +51,16 @@ class KeyguardMediaControllerTest : SysuiTestCase() { @Mock private lateinit var statusBarStateController: SysuiStatusBarStateController @Mock + private lateinit var configurationController: ConfigurationController + @Mock + private lateinit var featureFlags: FeatureFlags + @Mock private lateinit var notificationLockscreenUserManager: NotificationLockscreenUserManager @JvmField @Rule val mockito = MockitoJUnit.rule() private val mediaHeaderView: MediaHeaderView = MediaHeaderView(context, null) + private val hostView = UniqueObjectHostView(context) private lateinit var keyguardMediaController: KeyguardMediaController @Before @@ -62,10 +70,17 @@ class KeyguardMediaControllerTest : SysuiTestCase() { whenever(statusBarStateController.state).thenReturn(StatusBarState.KEYGUARD) whenever(notificationLockscreenUserManager.shouldShowLockscreenNotifications()) .thenReturn(true) - whenever(mediaHost.hostView).thenReturn(UniqueObjectHostView(context)) + whenever(mediaHost.hostView).thenReturn(hostView) - keyguardMediaController = KeyguardMediaController(mediaHost, bypassController, - statusBarStateController, notificationLockscreenUserManager) + keyguardMediaController = KeyguardMediaController( + mediaHost, + bypassController, + statusBarStateController, + notificationLockscreenUserManager, + featureFlags, + context, + configurationController + ) keyguardMediaController.attachSinglePaneContainer(mediaHeaderView) } @@ -105,11 +120,8 @@ class KeyguardMediaControllerTest : SysuiTestCase() { @Test fun testActivatesSplitShadeContainerInSplitShadeMode() { val splitShadeContainer = FrameLayout(context) - keyguardMediaController.attachSplitShadeContainer( - splitShadeContainer, - useContainer = { true }) - - keyguardMediaController.refreshMediaPosition() + keyguardMediaController.attachSplitShadeContainer(splitShadeContainer) + keyguardMediaController.useSplitShade = true assertThat(splitShadeContainer.visibility).isEqualTo(VISIBLE) } @@ -117,13 +129,28 @@ class KeyguardMediaControllerTest : SysuiTestCase() { @Test fun testActivatesSinglePaneContainerInSinglePaneMode() { val splitShadeContainer = FrameLayout(context) - keyguardMediaController.attachSplitShadeContainer( - splitShadeContainer, - useContainer = { false }) - - keyguardMediaController.refreshMediaPosition() + keyguardMediaController.attachSplitShadeContainer(splitShadeContainer) assertThat(splitShadeContainer.visibility).isEqualTo(GONE) assertThat(mediaHeaderView.visibility).isEqualTo(VISIBLE) } + + @Test + fun testAttachedToSplitShade() { + val splitShadeContainer = FrameLayout(context) + keyguardMediaController.attachSplitShadeContainer(splitShadeContainer) + keyguardMediaController.useSplitShade = true + + assertTrue("HostView wasn't attached to the split pane container", + splitShadeContainer.childCount == 1) + } + + @Test + fun testAttachedToSinglePane() { + val splitShadeContainer = FrameLayout(context) + keyguardMediaController.attachSplitShadeContainer(splitShadeContainer) + + assertTrue("HostView wasn't attached to the single pane container", + mediaHeaderView.childCount == 1) + } }