From 56223dafd065db4f024842b645b45c6b2e9a2370 Mon Sep 17 00:00:00 2001 From: Alejandro Nijamkin Date: Mon, 10 Oct 2022 10:49:54 -0700 Subject: [PATCH 1/3] Fix bug where switching to guest didn't work. The cause was that we were using UserRecord.resolveId() which would return UserHandle.USER_NULL when the record was for a guest. We now use the raw ID in the UserRecord in this case. Also, the logic is being moved from UserSwitcherControllerImpl which is a temporary class meant to help us migrate from the old implementation to the new and is thus not unit tested to UserInteractor, where we can comfortably add unit tests in this CL. Fix: 251740146, 251376342 Test: Issue no longer reproduces after this CL Change-Id: Ib0cbb23d0f6111af7a2eb98278796c34a3e69209 --- .../policy/UserSwitcherControllerImpl.kt | 7 +-- .../user/domain/interactor/UserInteractor.kt | 11 ++++ .../UserInteractorRefactoredTest.kt | 56 +++++++++++++++++++ 3 files changed, 68 insertions(+), 6 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/policy/UserSwitcherControllerImpl.kt b/packages/SystemUI/src/com/android/systemui/statusbar/policy/UserSwitcherControllerImpl.kt index 16926566105cf..086b6dd7c7288 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/policy/UserSwitcherControllerImpl.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/policy/UserSwitcherControllerImpl.kt @@ -30,7 +30,6 @@ import com.android.systemui.qs.user.UserSwitchDialogController import com.android.systemui.user.data.source.UserRecord import com.android.systemui.user.domain.interactor.GuestUserInteractor import com.android.systemui.user.domain.interactor.UserInteractor -import com.android.systemui.user.legacyhelper.data.LegacyUserDataHelper import com.android.systemui.user.legacyhelper.ui.LegacyUserUiHelper import dagger.Lazy import java.io.PrintWriter @@ -203,11 +202,7 @@ constructor( dialogShower: UserSwitchDialogController.DialogShower?, ) { if (useInteractor) { - if (LegacyUserDataHelper.isUser(record)) { - userInteractor.selectUser(record.resolveId()) - } else { - userInteractor.executeAction(LegacyUserDataHelper.toUserActionModel(record)) - } + userInteractor.onRecordSelected(record) } else { _oldImpl.onUserListItemClicked(record, dialogShower) } diff --git a/packages/SystemUI/src/com/android/systemui/user/domain/interactor/UserInteractor.kt b/packages/SystemUI/src/com/android/systemui/user/domain/interactor/UserInteractor.kt index a84238c1559a2..7dc893b7b6c0f 100644 --- a/packages/SystemUI/src/com/android/systemui/user/domain/interactor/UserInteractor.kt +++ b/packages/SystemUI/src/com/android/systemui/user/domain/interactor/UserInteractor.kt @@ -390,6 +390,17 @@ constructor( guestUserInteractor.onDeviceBootCompleted() } + /** Switches to the user or executes the action represented by the given record. */ + fun onRecordSelected(record: UserRecord) { + if (LegacyUserDataHelper.isUser(record)) { + // It's safe to use checkNotNull around record.info because isUser only returns true + // if record.info is not null. + selectUser(checkNotNull(record.info).id) + } else { + executeAction(LegacyUserDataHelper.toUserActionModel(record)) + } + } + /** Switches to the user with the given user ID. */ fun selectUser( newlySelectedUserId: Int, diff --git a/packages/SystemUI/tests/src/com/android/systemui/user/domain/interactor/UserInteractorRefactoredTest.kt b/packages/SystemUI/tests/src/com/android/systemui/user/domain/interactor/UserInteractorRefactoredTest.kt index 3d5695a09ebca..05d21e1b16a86 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/user/domain/interactor/UserInteractorRefactoredTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/user/domain/interactor/UserInteractorRefactoredTest.kt @@ -47,6 +47,7 @@ import org.junit.Before import org.junit.Test import org.junit.runner.RunWith import org.junit.runners.JUnit4 +import org.mockito.ArgumentMatchers.anyBoolean import org.mockito.ArgumentMatchers.anyInt import org.mockito.Mockito.verify @@ -72,6 +73,61 @@ class UserInteractorRefactoredTest : UserInteractorTest() { whenever(manager.canAddMoreUsers(any())).thenReturn(true) } + @Test + fun `onRecordSelected - user`() = + runBlocking(IMMEDIATE) { + val userInfos = createUserInfos(count = 3, includeGuest = false) + userRepository.setUserInfos(userInfos) + userRepository.setSelectedUserInfo(userInfos[0]) + userRepository.setSettings(UserSwitcherSettingsModel(isUserSwitcherEnabled = true)) + + underTest.onRecordSelected(UserRecord(info = userInfos[1])) + + verify(activityManager).switchUser(userInfos[1].id) + Unit + } + + @Test + fun `onRecordSelected - switch to guest user`() = + runBlocking(IMMEDIATE) { + val userInfos = createUserInfos(count = 3, includeGuest = true) + userRepository.setUserInfos(userInfos) + userRepository.setSelectedUserInfo(userInfos[0]) + userRepository.setSettings(UserSwitcherSettingsModel(isUserSwitcherEnabled = true)) + + underTest.onRecordSelected(UserRecord(info = userInfos.last())) + + verify(activityManager).switchUser(userInfos.last().id) + Unit + } + + @Test + fun `onRecordSelected - enter guest mode`() = + runBlocking(IMMEDIATE) { + val userInfos = createUserInfos(count = 3, includeGuest = false) + userRepository.setUserInfos(userInfos) + userRepository.setSelectedUserInfo(userInfos[0]) + userRepository.setSettings(UserSwitcherSettingsModel(isUserSwitcherEnabled = true)) + + underTest.onRecordSelected(UserRecord(isGuest = true)) + + verify(manager).createGuest(any()) + Unit + } + + @Test + fun `onRecordSelected - action`() = + runBlocking(IMMEDIATE) { + val userInfos = createUserInfos(count = 3, includeGuest = true) + userRepository.setUserInfos(userInfos) + userRepository.setSelectedUserInfo(userInfos[0]) + userRepository.setSettings(UserSwitcherSettingsModel(isUserSwitcherEnabled = true)) + + underTest.onRecordSelected(UserRecord(isAddSupervisedUser = true)) + + verify(activityStarter).startActivity(any(), anyBoolean()) + } + @Test fun `users - switcher enabled`() = runBlocking(IMMEDIATE) { From dc6c3039830bf70357d1fe837afb92932ff30317 Mon Sep 17 00:00:00 2001 From: Alejandro Nijamkin Date: Mon, 10 Oct 2022 12:20:12 -0700 Subject: [PATCH 2/3] Fix bug where switching users didn't dismiss UI. There's a bug where, with the new implementation, when the user switches to a different user on a phone, the user switcher dialog is not properly dismissed. This CL takes care of that by properly dismissing the DialogShower, if one is passed in. Fix: 251733467 Test: Tests included. Verified that the bug no longer reproduces after the fix. Change-Id: I053eec01051a863e375906c17a64932aa865df7e --- .../policy/UserSwitcherControllerImpl.kt | 4 +-- .../user/domain/interactor/UserInteractor.kt | 27 +++++++++++++------ .../UserInteractorRefactoredTest.kt | 24 ++++++++++++----- .../domain/interactor/UserInteractorTest.kt | 2 ++ 4 files changed, 41 insertions(+), 16 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/policy/UserSwitcherControllerImpl.kt b/packages/SystemUI/src/com/android/systemui/statusbar/policy/UserSwitcherControllerImpl.kt index 086b6dd7c7288..af39eeed26b06 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/policy/UserSwitcherControllerImpl.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/policy/UserSwitcherControllerImpl.kt @@ -117,7 +117,7 @@ constructor( dialogShower: UserSwitchDialogController.DialogShower? ) { if (useInteractor) { - userInteractor.selectUser(userId) + userInteractor.selectUser(userId, dialogShower) } else { _oldImpl.onUserSelected(userId, dialogShower) } @@ -202,7 +202,7 @@ constructor( dialogShower: UserSwitchDialogController.DialogShower?, ) { if (useInteractor) { - userInteractor.onRecordSelected(record) + userInteractor.onRecordSelected(record, dialogShower) } else { _oldImpl.onUserListItemClicked(record, dialogShower) } diff --git a/packages/SystemUI/src/com/android/systemui/user/domain/interactor/UserInteractor.kt b/packages/SystemUI/src/com/android/systemui/user/domain/interactor/UserInteractor.kt index 7dc893b7b6c0f..142a328b2bc4c 100644 --- a/packages/SystemUI/src/com/android/systemui/user/domain/interactor/UserInteractor.kt +++ b/packages/SystemUI/src/com/android/systemui/user/domain/interactor/UserInteractor.kt @@ -43,6 +43,7 @@ import com.android.systemui.flags.FeatureFlags import com.android.systemui.flags.Flags import com.android.systemui.keyguard.domain.interactor.KeyguardInteractor import com.android.systemui.plugins.ActivityStarter +import com.android.systemui.qs.user.UserSwitchDialogController import com.android.systemui.statusbar.policy.UserSwitcherController import com.android.systemui.telephony.domain.interactor.TelephonyInteractor import com.android.systemui.user.data.repository.UserRepository @@ -391,19 +392,23 @@ constructor( } /** Switches to the user or executes the action represented by the given record. */ - fun onRecordSelected(record: UserRecord) { + fun onRecordSelected( + record: UserRecord, + dialogShower: UserSwitchDialogController.DialogShower? = null, + ) { if (LegacyUserDataHelper.isUser(record)) { // It's safe to use checkNotNull around record.info because isUser only returns true // if record.info is not null. - selectUser(checkNotNull(record.info).id) + selectUser(checkNotNull(record.info).id, dialogShower) } else { - executeAction(LegacyUserDataHelper.toUserActionModel(record)) + executeAction(LegacyUserDataHelper.toUserActionModel(record), dialogShower) } } /** Switches to the user with the given user ID. */ fun selectUser( newlySelectedUserId: Int, + dialogShower: UserSwitchDialogController.DialogShower? = null, ) { if (isNewImpl) { val currentlySelectedUserInfo = repository.getSelectedUserInfo() @@ -439,22 +444,28 @@ constructor( return } + dialogShower?.dismiss() + switchUser(newlySelectedUserId) } else { - controller.onUserSelected(newlySelectedUserId, /* dialogShower= */ null) + controller.onUserSelected(newlySelectedUserId, dialogShower) } } /** Executes the given action. */ - fun executeAction(action: UserActionModel) { + fun executeAction( + action: UserActionModel, + dialogShower: UserSwitchDialogController.DialogShower? = null, + ) { if (isNewImpl) { when (action) { UserActionModel.ENTER_GUEST_MODE -> guestUserInteractor.createAndSwitchTo( this::showDialog, this::dismissDialog, - this::selectUser, - ) + ) { userId -> + selectUser(userId, dialogShower) + } UserActionModel.ADD_USER -> { val currentUser = repository.getSelectedUserInfo() showDialog( @@ -586,7 +597,7 @@ constructor( } private fun switchUser(userId: Int) { - // TODO(b/246631653): track jank and lantecy like in the old impl. + // TODO(b/246631653): track jank and latency like in the old impl. refreshUsersScheduler.pause() try { activityManager.switchUser(userId) diff --git a/packages/SystemUI/tests/src/com/android/systemui/user/domain/interactor/UserInteractorRefactoredTest.kt b/packages/SystemUI/tests/src/com/android/systemui/user/domain/interactor/UserInteractorRefactoredTest.kt index 05d21e1b16a86..37c378c9a530e 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/user/domain/interactor/UserInteractorRefactoredTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/user/domain/interactor/UserInteractorRefactoredTest.kt @@ -49,6 +49,7 @@ import org.junit.runner.RunWith import org.junit.runners.JUnit4 import org.mockito.ArgumentMatchers.anyBoolean import org.mockito.ArgumentMatchers.anyInt +import org.mockito.Mockito.never import org.mockito.Mockito.verify @SmallTest @@ -81,8 +82,9 @@ class UserInteractorRefactoredTest : UserInteractorTest() { userRepository.setSelectedUserInfo(userInfos[0]) userRepository.setSettings(UserSwitcherSettingsModel(isUserSwitcherEnabled = true)) - underTest.onRecordSelected(UserRecord(info = userInfos[1])) + underTest.onRecordSelected(UserRecord(info = userInfos[1]), dialogShower) + verify(dialogShower).dismiss() verify(activityManager).switchUser(userInfos[1].id) Unit } @@ -108,9 +110,12 @@ class UserInteractorRefactoredTest : UserInteractorTest() { userRepository.setUserInfos(userInfos) userRepository.setSelectedUserInfo(userInfos[0]) userRepository.setSettings(UserSwitcherSettingsModel(isUserSwitcherEnabled = true)) + val guestUserInfo = createUserInfo(id = 1337, name = "guest", isGuest = true) + whenever(manager.createGuest(any())).thenReturn(guestUserInfo) - underTest.onRecordSelected(UserRecord(isGuest = true)) + underTest.onRecordSelected(UserRecord(isGuest = true), dialogShower) + verify(dialogShower).dismiss() verify(manager).createGuest(any()) Unit } @@ -123,8 +128,9 @@ class UserInteractorRefactoredTest : UserInteractorTest() { userRepository.setSelectedUserInfo(userInfos[0]) userRepository.setSettings(UserSwitcherSettingsModel(isUserSwitcherEnabled = true)) - underTest.onRecordSelected(UserRecord(isAddSupervisedUser = true)) + underTest.onRecordSelected(UserRecord(isAddSupervisedUser = true), dialogShower) + verify(dialogShower, never()).dismiss() verify(activityStarter).startActivity(any(), anyBoolean()) } @@ -392,10 +398,14 @@ class UserInteractorRefactoredTest : UserInteractorTest() { var dialogRequest: ShowDialogRequestModel? = null val job = underTest.dialogShowRequests.onEach { dialogRequest = it }.launchIn(this) - underTest.selectUser(newlySelectedUserId = guestUserInfo.id) + underTest.selectUser( + newlySelectedUserId = guestUserInfo.id, + dialogShower = dialogShower, + ) assertThat(dialogRequest) .isInstanceOf(ShowDialogRequestModel.ShowExitGuestDialog::class.java) + verify(dialogShower, never()).dismiss() job.cancel() } @@ -411,10 +421,11 @@ class UserInteractorRefactoredTest : UserInteractorTest() { var dialogRequest: ShowDialogRequestModel? = null val job = underTest.dialogShowRequests.onEach { dialogRequest = it }.launchIn(this) - underTest.selectUser(newlySelectedUserId = userInfos[0].id) + underTest.selectUser(newlySelectedUserId = userInfos[0].id, dialogShower = dialogShower) assertThat(dialogRequest) .isInstanceOf(ShowDialogRequestModel.ShowExitGuestDialog::class.java) + verify(dialogShower, never()).dismiss() job.cancel() } @@ -428,10 +439,11 @@ class UserInteractorRefactoredTest : UserInteractorTest() { var dialogRequest: ShowDialogRequestModel? = null val job = underTest.dialogShowRequests.onEach { dialogRequest = it }.launchIn(this) - underTest.selectUser(newlySelectedUserId = userInfos[1].id) + underTest.selectUser(newlySelectedUserId = userInfos[1].id, dialogShower = dialogShower) assertThat(dialogRequest).isNull() verify(activityManager).switchUser(userInfos[1].id) + verify(dialogShower).dismiss() job.cancel() } diff --git a/packages/SystemUI/tests/src/com/android/systemui/user/domain/interactor/UserInteractorTest.kt b/packages/SystemUI/tests/src/com/android/systemui/user/domain/interactor/UserInteractorTest.kt index 8465f4f46d62b..1680c36cef871 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/user/domain/interactor/UserInteractorTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/user/domain/interactor/UserInteractorTest.kt @@ -27,6 +27,7 @@ import com.android.systemui.flags.Flags import com.android.systemui.keyguard.data.repository.FakeKeyguardRepository import com.android.systemui.keyguard.domain.interactor.KeyguardInteractor import com.android.systemui.plugins.ActivityStarter +import com.android.systemui.qs.user.UserSwitchDialogController import com.android.systemui.statusbar.policy.DeviceProvisionedController import com.android.systemui.statusbar.policy.UserSwitcherController import com.android.systemui.telephony.data.repository.FakeTelephonyRepository @@ -46,6 +47,7 @@ abstract class UserInteractorTest : SysuiTestCase() { @Mock protected lateinit var deviceProvisionedController: DeviceProvisionedController @Mock protected lateinit var devicePolicyManager: DevicePolicyManager @Mock protected lateinit var uiEventLogger: UiEventLogger + @Mock protected lateinit var dialogShower: UserSwitchDialogController.DialogShower protected lateinit var underTest: UserInteractor From 640931d5f4cd6dbcef04d170674580ca086bdd5a Mon Sep 17 00:00:00 2001 From: Alejandro Nijamkin Date: Mon, 10 Oct 2022 15:19:13 -0700 Subject: [PATCH 3/3] Sorts users in switchers by creation time. This was discovered by QA but is less of a bug and more of a feature request. The new implementation will display the users in creation order with the oldest users first. Fix: 251365125 Test: Manually verified order and also added a unit test Change-Id: Ib3989fc24fa4cfd9bf27936855434edf69670bd4 --- .../user/data/repository/UserRepository.kt | 2 +- .../UserRepositoryImplRefactoredTest.kt | 21 +++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/packages/SystemUI/src/com/android/systemui/user/data/repository/UserRepository.kt b/packages/SystemUI/src/com/android/systemui/user/data/repository/UserRepository.kt index 3014f39c17f83..919e699652bcf 100644 --- a/packages/SystemUI/src/com/android/systemui/user/data/repository/UserRepository.kt +++ b/packages/SystemUI/src/com/android/systemui/user/data/repository/UserRepository.kt @@ -220,7 +220,7 @@ constructor( val result = withContext(backgroundDispatcher) { manager.aliveUsers } if (result != null) { - _userInfos.value = result + _userInfos.value = result.sortedBy { it.creationTime } } } } diff --git a/packages/SystemUI/tests/src/com/android/systemui/user/data/repository/UserRepositoryImplRefactoredTest.kt b/packages/SystemUI/tests/src/com/android/systemui/user/data/repository/UserRepositoryImplRefactoredTest.kt index 4a8e0552d7785..d951f366c5952 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/user/data/repository/UserRepositoryImplRefactoredTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/user/data/repository/UserRepositoryImplRefactoredTest.kt @@ -120,6 +120,27 @@ class UserRepositoryImplRefactoredTest : UserRepositoryImplTest() { assertThat(underTest.lastSelectedNonGuestUserId).isEqualTo(selectedNonGuestUserId) } + @Test + fun `refreshUsers - sorts by creation time`() = runSelfCancelingTest { + underTest = create(this) + val unsortedUsers = + setUpUsers( + count = 3, + selectedIndex = 0, + ) + unsortedUsers[0].creationTime = 900 + unsortedUsers[1].creationTime = 700 + unsortedUsers[2].creationTime = 999 + val expectedUsers = listOf(unsortedUsers[1], unsortedUsers[0], unsortedUsers[2]) + var userInfos: List? = null + var selectedUserInfo: UserInfo? = null + underTest.userInfos.onEach { userInfos = it }.launchIn(this) + underTest.selectedUserInfo.onEach { selectedUserInfo = it }.launchIn(this) + + underTest.refreshUsers() + assertThat(userInfos).isEqualTo(expectedUsers) + } + private fun setUpUsers( count: Int, hasGuest: Boolean = false,