From e509515edbfb623374d8e16288e3b5880daa9f67 Mon Sep 17 00:00:00 2001 From: Evan Laird Date: Tue, 22 Aug 2023 17:08:59 -0400 Subject: [StatusBar] Request layout on vis change, and move to CREATED The flow that was being bound to the mobile icon group's visibility lived in STARTED (this caused tests to pass). It also was experiencing a race condition against the StatusIconContainer. In certain circumstances, the view was being created and sent to have an icon state of HIDDEN, despite having changed itself to be VISIBLE. This is all emergent behavior from StatusIconContainer, which decides which state to set on icons in its onMeasure(), but then applies that state in onLayout(). To test: remove battery percent from status bar; ensure that _no other icon_ is showing. This means disable Wi-Fi and make sure volume is set to vibrate or on, since those states have no icon. Once you are in a state where there is only battery and mobile showing, remove and re-insert the SIM card to reproduce. Test: ModernStatusBarMobileViewTest Test: manually removing and inserting SIM card Fixes: 291031862 (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:53657d1ff8ba10647c38d8ad5fd0486202b2dedd) Merged-In: I0e68037126a526fcf3614bf25683af3fa4e5aa96 Change-Id: I0e68037126a526fcf3614bf25683af3fa4e5aa96 --- .../pipeline/mobile/ui/binder/MobileIconBinder.kt | 181 +++++++++++---------- .../carrier/ShadeCarrierGroupControllerTest.java | 12 +- .../android/systemui/util/kotlin/FlowProvider.kt | 22 +++ 3 files changed, 130 insertions(+), 85 deletions(-) create mode 100644 packages/SystemUI/tests/src/com/android/systemui/util/kotlin/FlowProvider.kt diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/ui/binder/MobileIconBinder.kt b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/ui/binder/MobileIconBinder.kt index 4b2fb4373957..249ca35b610a 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/ui/binder/MobileIconBinder.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/ui/binder/MobileIconBinder.kt @@ -26,6 +26,7 @@ import android.widget.ImageView import android.widget.Space import androidx.core.view.isVisible import androidx.lifecycle.Lifecycle +import androidx.lifecycle.lifecycleScope import androidx.lifecycle.repeatOnLifecycle import com.android.settingslib.graph.SignalDrawable import com.android.systemui.R @@ -64,7 +65,7 @@ object MobileIconBinder { val roamingSpace = view.requireViewById(R.id.mobile_roaming_space) val dotView = view.requireViewById(R.id.status_bar_dot) - view.isVisible = true + view.isVisible = viewModel.isVisible.value iconView.isVisible = true // TODO(b/238425913): We should log this visibility state. @@ -77,108 +78,122 @@ object MobileIconBinder { var isCollecting = false view.repeatWhenAttached { - repeatOnLifecycle(Lifecycle.State.STARTED) { - logger.logCollectionStarted(view, viewModel) - isCollecting = true - - launch { - visibilityState.collect { state -> - when (state) { - STATE_ICON -> { - mobileGroupView.visibility = VISIBLE - dotView.visibility = GONE - } - STATE_DOT -> { - mobileGroupView.visibility = INVISIBLE - dotView.visibility = VISIBLE - } - STATE_HIDDEN -> { - mobileGroupView.visibility = INVISIBLE - dotView.visibility = INVISIBLE - } + lifecycleScope.launch { + repeatOnLifecycle(Lifecycle.State.CREATED) { + // isVisible controls the visibility state of the outer group, and thus it needs + // to run in the CREATED lifecycle so it can continue to watch while invisible + // See (b/291031862) for details + launch { + viewModel.isVisible.collect { isVisible -> + viewModel.verboseLogger?.logBinderReceivedVisibility( + view, + viewModel.subscriptionId, + isVisible + ) + view.isVisible = isVisible + // [StatusIconContainer] can get out of sync sometimes. Make sure to + // request another layout when this changes. + view.requestLayout() } } } + } - launch { - viewModel.isVisible.collect { isVisible -> - viewModel.verboseLogger?.logBinderReceivedVisibility( - view, - viewModel.subscriptionId, - isVisible - ) - view.isVisible = isVisible + lifecycleScope.launch { + repeatOnLifecycle(Lifecycle.State.STARTED) { + logger.logCollectionStarted(view, viewModel) + isCollecting = true + + launch { + visibilityState.collect { state -> + when (state) { + STATE_ICON -> { + mobileGroupView.visibility = VISIBLE + dotView.visibility = GONE + } + STATE_DOT -> { + mobileGroupView.visibility = INVISIBLE + dotView.visibility = VISIBLE + } + STATE_HIDDEN -> { + mobileGroupView.visibility = INVISIBLE + dotView.visibility = INVISIBLE + } + } + } } - } - // Set the icon for the triangle - launch { - viewModel.icon.distinctUntilChanged().collect { icon -> - viewModel.verboseLogger?.logBinderReceivedSignalIcon( - view, - viewModel.subscriptionId, - icon, - ) - mobileDrawable.level = icon.toSignalDrawableState() + // Set the icon for the triangle + launch { + viewModel.icon.distinctUntilChanged().collect { icon -> + viewModel.verboseLogger?.logBinderReceivedSignalIcon( + view, + viewModel.subscriptionId, + icon, + ) + mobileDrawable.level = icon.toSignalDrawableState() + } } - } - launch { - viewModel.contentDescription.distinctUntilChanged().collect { - ContentDescriptionViewBinder.bind(it, view) + launch { + viewModel.contentDescription.distinctUntilChanged().collect { + ContentDescriptionViewBinder.bind(it, view) + } } - } - // Set the network type icon - launch { - viewModel.networkTypeIcon.distinctUntilChanged().collect { dataTypeId -> - viewModel.verboseLogger?.logBinderReceivedNetworkTypeIcon( - view, - viewModel.subscriptionId, - dataTypeId, - ) - dataTypeId?.let { IconViewBinder.bind(dataTypeId, networkTypeView) } - networkTypeView.visibility = if (dataTypeId != null) VISIBLE else GONE + // Set the network type icon + launch { + viewModel.networkTypeIcon.distinctUntilChanged().collect { dataTypeId -> + viewModel.verboseLogger?.logBinderReceivedNetworkTypeIcon( + view, + viewModel.subscriptionId, + dataTypeId, + ) + dataTypeId?.let { IconViewBinder.bind(dataTypeId, networkTypeView) } + networkTypeView.visibility = if (dataTypeId != null) VISIBLE else GONE + } } - } - // Set the roaming indicator - launch { - viewModel.roaming.distinctUntilChanged().collect { isRoaming -> - roamingView.isVisible = isRoaming - roamingSpace.isVisible = isRoaming + // Set the roaming indicator + launch { + viewModel.roaming.distinctUntilChanged().collect { isRoaming -> + roamingView.isVisible = isRoaming + roamingSpace.isVisible = isRoaming + } } - } - // Set the activity indicators - launch { viewModel.activityInVisible.collect { activityIn.isVisible = it } } + // Set the activity indicators + launch { viewModel.activityInVisible.collect { activityIn.isVisible = it } } - launch { viewModel.activityOutVisible.collect { activityOut.isVisible = it } } + launch { viewModel.activityOutVisible.collect { activityOut.isVisible = it } } - launch { - viewModel.activityContainerVisible.collect { activityContainer.isVisible = it } - } + launch { + viewModel.activityContainerVisible.collect { + activityContainer.isVisible = it + } + } - // Set the tint - launch { - iconTint.collect { tint -> - val tintList = ColorStateList.valueOf(tint) - iconView.imageTintList = tintList - networkTypeView.imageTintList = tintList - roamingView.imageTintList = tintList - activityIn.imageTintList = tintList - activityOut.imageTintList = tintList - dotView.setDecorColor(tint) + // Set the tint + launch { + iconTint.collect { tint -> + val tintList = ColorStateList.valueOf(tint) + iconView.imageTintList = tintList + networkTypeView.imageTintList = tintList + roamingView.imageTintList = tintList + activityIn.imageTintList = tintList + activityOut.imageTintList = tintList + dotView.setDecorColor(tint) + } } - } - launch { decorTint.collect { tint -> dotView.setDecorColor(tint) } } + launch { decorTint.collect { tint -> dotView.setDecorColor(tint) } } - try { - awaitCancellation() - } finally { - isCollecting = false - logger.logCollectionStopped(view, viewModel) + try { + awaitCancellation() + } finally { + isCollecting = false + logger.logCollectionStopped(view, viewModel) + } } } } diff --git a/packages/SystemUI/tests/src/com/android/systemui/shade/carrier/ShadeCarrierGroupControllerTest.java b/packages/SystemUI/tests/src/com/android/systemui/shade/carrier/ShadeCarrierGroupControllerTest.java index 5fa6b3a15d35..e7056c7b0b9d 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/shade/carrier/ShadeCarrierGroupControllerTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/shade/carrier/ShadeCarrierGroupControllerTest.java @@ -58,6 +58,7 @@ import com.android.systemui.statusbar.pipeline.mobile.ui.MobileViewLogger; import com.android.systemui.statusbar.pipeline.mobile.ui.viewmodel.MobileIconsViewModel; import com.android.systemui.statusbar.pipeline.mobile.ui.viewmodel.ShadeCarrierGroupMobileIconViewModel; import com.android.systemui.util.CarrierConfigTracker; +import com.android.systemui.util.kotlin.FlowProviderKt; import com.android.systemui.utils.leaks.LeakCheckedTest; import com.android.systemui.utils.os.FakeHandler; @@ -72,6 +73,8 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import kotlinx.coroutines.flow.MutableStateFlow; + @RunWith(AndroidTestingRunner.class) @TestableLooper.RunWithLooper @SmallTest @@ -105,7 +108,8 @@ public class ShadeCarrierGroupControllerTest extends LeakCheckedTest { private ShadeCarrier mShadeCarrier3; private TestableLooper mTestableLooper; @Mock - private ShadeCarrierGroupController.OnSingleCarrierChangedListener mOnSingleCarrierChangedListener; + private ShadeCarrierGroupController.OnSingleCarrierChangedListener + mOnSingleCarrierChangedListener; @Mock private MobileUiAdapter mMobileUiAdapter; @Mock @@ -119,6 +123,9 @@ public class ShadeCarrierGroupControllerTest extends LeakCheckedTest { @Mock private StatusBarPipelineFlags mStatusBarPipelineFlags; + private final MutableStateFlow mIsVisibleFlow = + FlowProviderKt.getMutableStateFlow(true); + private FakeSlotIndexResolver mSlotIndexResolver; private ClickListenerTextView mNoCarrierTextView; @@ -170,7 +177,7 @@ public class ShadeCarrierGroupControllerTest extends LeakCheckedTest { mMobileUiAdapter, mMobileContextProvider, mStatusBarPipelineFlags - ) + ) .setShadeCarrierGroup(mShadeCarrierGroup) .build(); @@ -181,6 +188,7 @@ public class ShadeCarrierGroupControllerTest extends LeakCheckedTest { when(mStatusBarPipelineFlags.useNewShadeCarrierGroupMobileIcons()).thenReturn(true); when(mMobileContextProvider.getMobileContextForSub(anyInt(), any())).thenReturn(mContext); when(mMobileIconsViewModel.getLogger()).thenReturn(mMobileViewLogger); + when(mShadeCarrierGroupMobileIconViewModel.isVisible()).thenReturn(mIsVisibleFlow); when(mMobileIconsViewModel.viewModelForSub(anyInt(), any())) .thenReturn(mShadeCarrierGroupMobileIconViewModel); } diff --git a/packages/SystemUI/tests/src/com/android/systemui/util/kotlin/FlowProvider.kt b/packages/SystemUI/tests/src/com/android/systemui/util/kotlin/FlowProvider.kt new file mode 100644 index 000000000000..274acc9a8c23 --- /dev/null +++ b/packages/SystemUI/tests/src/com/android/systemui/util/kotlin/FlowProvider.kt @@ -0,0 +1,22 @@ +/* + * 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.util.kotlin + +import kotlinx.coroutines.flow.MutableStateFlow + +/** Wrapper for flow constructors that can be retrieved from java tests */ +fun getMutableStateFlow(value: T): MutableStateFlow = MutableStateFlow(value) -- cgit v1.2.3-59-g8ed1b