From e964963e7496fae49fccd85488c628c845cde48f Mon Sep 17 00:00:00 2001 From: Alejandro Nijamkin Date: Wed, 7 Sep 2022 14:58:53 -0700 Subject: [PATCH] Fixes bug where the indication text is not padded. This is a fun one. When switching to a language where the "Unlock screen for access to all functions" indication area message (which only appears right after a reboot) is very long, the home controls quick affordance button can overlap with the message as per the linked bug. Normally, the padding of the indication area is controled based on the visibility of quick affordances. If at least one quick affordance button is visible, the KeyguardBottomAreaViewBinder pads the indication area to fit between the buttons. If none is visible, the padding is set to 0, so the indication area message can be as wide as possible. There are two separate issues here: 1. The home controls quick affordance config, which provides state to ultimately show the button does not emit an initial value. That makes the entire downstream flow never receive a value which means that the padding logic in the binder never runs. This CL addresses this issue by emitting an initially Hidden value for any quick affordance known to the quick affordance framework (as this bug can repeat itself if the next config does not emit an initial value). 2. The home controls quick affordance config is incorrectly being shown on the lock-screen before the first unlock after a device reboot. In the process of refactoring the code, the new implementation missed the need to include the ControlsComponent.getVisibility check that the old implementation was using. In a followup CL, we will address this too Fix: 244296596 Test: Unit tests. Manually made sure that, with a phone switched to Slovenian and rebooted, when the home controls button appears on the lock-screen before unlocking it (which is a bug as explained above), the long text is properly wrapped because the indication area is padded. Change-Id: I6fb612edb139113594040a5d45ee8e661f0a0e5c --- .../interactor/KeyguardQuickAffordanceInteractor.kt | 11 ++++++++++- .../usecase/KeyguardQuickAffordanceInteractorTest.kt | 9 +++++++++ .../ui/viewmodel/KeyguardBottomAreaViewModelTest.kt | 9 ++++++++- 3 files changed, 27 insertions(+), 2 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/keyguard/domain/interactor/KeyguardQuickAffordanceInteractor.kt b/packages/SystemUI/src/com/android/systemui/keyguard/domain/interactor/KeyguardQuickAffordanceInteractor.kt index 9a69e26488d97..95acc0b8564ef 100644 --- a/packages/SystemUI/src/com/android/systemui/keyguard/domain/interactor/KeyguardQuickAffordanceInteractor.kt +++ b/packages/SystemUI/src/com/android/systemui/keyguard/domain/interactor/KeyguardQuickAffordanceInteractor.kt @@ -32,6 +32,7 @@ import javax.inject.Inject import kotlin.reflect.KClass import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.combine +import kotlinx.coroutines.flow.onStart @SysUISingleton class KeyguardQuickAffordanceInteractor @@ -88,7 +89,15 @@ constructor( position: KeyguardQuickAffordancePosition ): Flow { val configs = registry.getAll(position) - return combine(configs.map { config -> config.state }) { states -> + return combine( + configs.map { config -> + // We emit an initial "Hidden" value to make sure that there's always an initial + // value and avoid subtle bugs where the downstream isn't receiving any values + // because one config implementation is not emitting an initial value. For example, + // see b/244296596. + config.state.onStart { emit(KeyguardQuickAffordanceConfig.State.Hidden) } + } + ) { states -> val index = states.indexOfFirst { it is KeyguardQuickAffordanceConfig.State.Visible } if (index != -1) { val visibleState = states[index] as KeyguardQuickAffordanceConfig.State.Visible diff --git a/packages/SystemUI/tests/src/com/android/systemui/keyguard/domain/usecase/KeyguardQuickAffordanceInteractorTest.kt b/packages/SystemUI/tests/src/com/android/systemui/keyguard/domain/usecase/KeyguardQuickAffordanceInteractorTest.kt index d3fc29f1a0f17..19d841222a01c 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/keyguard/domain/usecase/KeyguardQuickAffordanceInteractorTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/keyguard/domain/usecase/KeyguardQuickAffordanceInteractorTest.kt @@ -36,6 +36,7 @@ import com.google.common.truth.Truth.assertThat import kotlinx.coroutines.flow.launchIn import kotlinx.coroutines.flow.onEach import kotlinx.coroutines.test.runBlockingTest +import kotlinx.coroutines.yield import org.junit.Before import org.junit.Test import org.junit.runner.RunWith @@ -110,6 +111,10 @@ class KeyguardQuickAffordanceInteractorTest : SysuiTestCase() { .quickAffordance(KeyguardQuickAffordancePosition.BOTTOM_START) .onEach { latest = it } .launchIn(this) + // The interactor has an onStart { emit(Hidden) } to cover for upstream configs that don't + // produce an initial value. We yield to give the coroutine time to emit the first real + // value from our config. + yield() assertThat(latest).isInstanceOf(KeyguardQuickAffordanceModel.Visible::class.java) val visibleModel = latest as KeyguardQuickAffordanceModel.Visible @@ -136,6 +141,10 @@ class KeyguardQuickAffordanceInteractorTest : SysuiTestCase() { .quickAffordance(KeyguardQuickAffordancePosition.BOTTOM_END) .onEach { latest = it } .launchIn(this) + // The interactor has an onStart { emit(Hidden) } to cover for upstream configs that don't + // produce an initial value. We yield to give the coroutine time to emit the first real + // value from our config. + yield() assertThat(latest).isInstanceOf(KeyguardQuickAffordanceModel.Visible::class.java) val visibleModel = latest as KeyguardQuickAffordanceModel.Visible diff --git a/packages/SystemUI/tests/src/com/android/systemui/keyguard/ui/viewmodel/KeyguardBottomAreaViewModelTest.kt b/packages/SystemUI/tests/src/com/android/systemui/keyguard/ui/viewmodel/KeyguardBottomAreaViewModelTest.kt index 14b85b8b5e565..c612091382db7 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/keyguard/ui/viewmodel/KeyguardBottomAreaViewModelTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/keyguard/ui/viewmodel/KeyguardBottomAreaViewModelTest.kt @@ -224,7 +224,10 @@ class KeyguardBottomAreaViewModelTest : SysuiTestCase() { repository.setAnimateDozingTransitions(false) yield() - assertThat(values).isEqualTo(listOf(false, true, false)) + // Note the extra false value in the beginning. This is to cover for the initial value + // inserted by the quick affordance interactor which it does to cover for config + // implementations that don't emit an initial value. + assertThat(values).isEqualTo(listOf(false, false, true, false)) job.cancel() } @@ -372,6 +375,10 @@ class KeyguardBottomAreaViewModelTest : SysuiTestCase() { var latest: KeyguardQuickAffordanceViewModel? = null val job = underTest.startButton.onEach { latest = it }.launchIn(this) + // The interactor has an onStart { emit(Hidden) } to cover for upstream configs that don't + // produce an initial value. We yield to give the coroutine time to emit the first real + // value from our config. + yield() assertQuickAffordanceViewModel( viewModel = latest,