From 8f8b74fb734061598e6e3047d653adc46834a403 Mon Sep 17 00:00:00 2001 From: Alejandro Nijamkin Date: Fri, 30 Sep 2022 16:10:19 -0700 Subject: [PATCH] UserSwitcherController impl uses UserInteractor Based on a flag, UserSwitcherControllerImpl will use UserInteractor, GuestUserInteractor, and KeyguardInteractor to implement its interface. Once this is all cleaned up, we can remove the old implementation and even update users of UserSwitcherControllerImpl to directly use the interactors instead. Bug: 246631653 Test: Unit tests added for newly exposed functionality. Existing tests still pass. Manually verified the full-screen user switcher and the dropdown in the bouncer. Change-Id: Iff7c06f8c0e2fdd8adaab436ff895e9d892d2f20 --- .../policy/UserSwitcherControllerImpl.kt | 113 +++++++++---- .../domain/interactor/GuestUserInteractor.kt | 2 +- .../user/domain/interactor/UserInteractor.kt | 149 ++++++++++++------ .../legacyhelper/data/LegacyUserDataHelper.kt | 15 ++ .../interactor/GuestUserInteractorTest.kt | 60 ++++++- 5 files changed, 262 insertions(+), 77 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 4932a6544d05f..16926566105cf 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/policy/UserSwitcherControllerImpl.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/policy/UserSwitcherControllerImpl.kt @@ -17,12 +17,21 @@ package com.android.systemui.statusbar.policy +import android.content.Context import android.content.Intent import android.view.View +import com.android.systemui.dagger.SysUISingleton +import com.android.systemui.dagger.qualifiers.Application 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.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 import java.lang.ref.WeakReference @@ -30,11 +39,17 @@ import javax.inject.Inject import kotlinx.coroutines.flow.Flow /** Implementation of [UserSwitcherController]. */ +@SysUISingleton class UserSwitcherControllerImpl @Inject constructor( - private val flags: FeatureFlags, + @Application private val applicationContext: Context, + flags: FeatureFlags, @Suppress("DEPRECATION") private val oldImpl: Lazy, + private val userInteractorLazy: Lazy, + private val guestUserInteractorLazy: Lazy, + private val keyguardInteractorLazy: Lazy, + private val activityStarter: ActivityStarter, ) : UserSwitcherController { private val useInteractor: Boolean = @@ -42,15 +57,21 @@ constructor( !flags.isEnabled(Flags.USER_INTERACTOR_AND_REPO_USE_CONTROLLER) private val _oldImpl: UserSwitcherControllerOldImpl get() = oldImpl.get() + private val userInteractor: UserInteractor by lazy { userInteractorLazy.get() } + private val guestUserInteractor: GuestUserInteractor by lazy { guestUserInteractorLazy.get() } + private val keyguardInteractor: KeyguardInteractor by lazy { keyguardInteractorLazy.get() } - private fun notYetImplemented(): Nothing { - error("Not yet implemented!") + private val callbackCompatMap = + mutableMapOf() + + private fun notSupported(): Nothing { + error("Not supported in the new implementation!") } override val users: ArrayList get() = if (useInteractor) { - notYetImplemented() + userInteractor.userRecords.value } else { _oldImpl.users } @@ -58,15 +79,13 @@ constructor( override val isSimpleUserSwitcher: Boolean get() = if (useInteractor) { - notYetImplemented() + userInteractor.isSimpleUserSwitcher } else { _oldImpl.isSimpleUserSwitcher } override fun init(view: View) { - if (useInteractor) { - notYetImplemented() - } else { + if (!useInteractor) { _oldImpl.init(view) } } @@ -74,7 +93,7 @@ constructor( override val currentUserRecord: UserRecord? get() = if (useInteractor) { - notYetImplemented() + userInteractor.selectedUserRecord.value } else { _oldImpl.currentUserRecord } @@ -82,7 +101,14 @@ constructor( override val currentUserName: String? get() = if (useInteractor) { - notYetImplemented() + currentUserRecord?.let { + LegacyUserUiHelper.getUserRecordName( + context = applicationContext, + record = it, + isGuestUserAutoCreated = userInteractor.isGuestUserAutoCreated, + isGuestUserResetting = userInteractor.isGuestUserResetting, + ) + } } else { _oldImpl.currentUserName } @@ -92,7 +118,7 @@ constructor( dialogShower: UserSwitchDialogController.DialogShower? ) { if (useInteractor) { - notYetImplemented() + userInteractor.selectUser(userId) } else { _oldImpl.onUserSelected(userId, dialogShower) } @@ -101,7 +127,7 @@ constructor( override val isAddUsersFromLockScreenEnabled: Flow get() = if (useInteractor) { - notYetImplemented() + notSupported() } else { _oldImpl.isAddUsersFromLockScreenEnabled } @@ -109,7 +135,7 @@ constructor( override val isGuestUserAutoCreated: Boolean get() = if (useInteractor) { - notYetImplemented() + userInteractor.isGuestUserAutoCreated } else { _oldImpl.isGuestUserAutoCreated } @@ -117,7 +143,7 @@ constructor( override val isGuestUserResetting: Boolean get() = if (useInteractor) { - notYetImplemented() + userInteractor.isGuestUserResetting } else { _oldImpl.isGuestUserResetting } @@ -126,7 +152,7 @@ constructor( dialogShower: UserSwitchDialogController.DialogShower?, ) { if (useInteractor) { - notYetImplemented() + notSupported() } else { _oldImpl.createAndSwitchToGuestUser(dialogShower) } @@ -134,7 +160,7 @@ constructor( override fun showAddUserDialog(dialogShower: UserSwitchDialogController.DialogShower?) { if (useInteractor) { - notYetImplemented() + notSupported() } else { _oldImpl.showAddUserDialog(dialogShower) } @@ -142,23 +168,31 @@ constructor( override fun startSupervisedUserActivity() { if (useInteractor) { - notYetImplemented() + notSupported() } else { _oldImpl.startSupervisedUserActivity() } } override fun onDensityOrFontScaleChanged() { - if (useInteractor) { - notYetImplemented() - } else { + if (!useInteractor) { _oldImpl.onDensityOrFontScaleChanged() } } override fun addAdapter(adapter: WeakReference) { if (useInteractor) { - notYetImplemented() + userInteractor.addCallback( + object : UserInteractor.UserCallback { + override fun isEvictable(): Boolean { + return adapter.get() == null + } + + override fun onUserStateChanged() { + adapter.get()?.notifyDataSetChanged() + } + } + ) } else { _oldImpl.addAdapter(adapter) } @@ -169,7 +203,11 @@ constructor( dialogShower: UserSwitchDialogController.DialogShower?, ) { if (useInteractor) { - notYetImplemented() + if (LegacyUserDataHelper.isUser(record)) { + userInteractor.selectUser(record.resolveId()) + } else { + userInteractor.executeAction(LegacyUserDataHelper.toUserActionModel(record)) + } } else { _oldImpl.onUserListItemClicked(record, dialogShower) } @@ -177,7 +215,10 @@ constructor( override fun removeGuestUser(guestUserId: Int, targetUserId: Int) { if (useInteractor) { - notYetImplemented() + userInteractor.removeGuestUser( + guestUserId = guestUserId, + targetUserId = targetUserId, + ) } else { _oldImpl.removeGuestUser(guestUserId, targetUserId) } @@ -189,7 +230,7 @@ constructor( forceRemoveGuestOnExit: Boolean ) { if (useInteractor) { - notYetImplemented() + userInteractor.exitGuestUser(guestUserId, targetUserId, forceRemoveGuestOnExit) } else { _oldImpl.exitGuestUser(guestUserId, targetUserId, forceRemoveGuestOnExit) } @@ -197,7 +238,7 @@ constructor( override fun schedulePostBootGuestCreation() { if (useInteractor) { - notYetImplemented() + guestUserInteractor.onDeviceBootCompleted() } else { _oldImpl.schedulePostBootGuestCreation() } @@ -206,14 +247,14 @@ constructor( override val isKeyguardShowing: Boolean get() = if (useInteractor) { - notYetImplemented() + keyguardInteractor.isKeyguardShowing() } else { _oldImpl.isKeyguardShowing } override fun startActivity(intent: Intent) { if (useInteractor) { - notYetImplemented() + activityStarter.startActivity(intent, /* dismissShade= */ false) } else { _oldImpl.startActivity(intent) } @@ -221,7 +262,7 @@ constructor( override fun refreshUsers(forcePictureLoadForId: Int) { if (useInteractor) { - notYetImplemented() + userInteractor.refreshUsers() } else { _oldImpl.refreshUsers(forcePictureLoadForId) } @@ -229,7 +270,14 @@ constructor( override fun addUserSwitchCallback(callback: UserSwitcherController.UserSwitchCallback) { if (useInteractor) { - notYetImplemented() + val interactorCallback = + object : UserInteractor.UserCallback { + override fun onUserStateChanged() { + callback.onUserSwitched() + } + } + callbackCompatMap[callback] = interactorCallback + userInteractor.addCallback(interactorCallback) } else { _oldImpl.addUserSwitchCallback(callback) } @@ -237,7 +285,10 @@ constructor( override fun removeUserSwitchCallback(callback: UserSwitcherController.UserSwitchCallback) { if (useInteractor) { - notYetImplemented() + val interactorCallback = callbackCompatMap.remove(callback) + if (interactorCallback != null) { + userInteractor.removeCallback(interactorCallback) + } } else { _oldImpl.removeUserSwitchCallback(callback) } @@ -245,7 +296,7 @@ constructor( override fun dump(pw: PrintWriter, args: Array) { if (useInteractor) { - notYetImplemented() + userInteractor.dump(pw) } else { _oldImpl.dump(pw, args) } diff --git a/packages/SystemUI/src/com/android/systemui/user/domain/interactor/GuestUserInteractor.kt b/packages/SystemUI/src/com/android/systemui/user/domain/interactor/GuestUserInteractor.kt index 27748128a557c..07e5cf9d9df27 100644 --- a/packages/SystemUI/src/com/android/systemui/user/domain/interactor/GuestUserInteractor.kt +++ b/packages/SystemUI/src/com/android/systemui/user/domain/interactor/GuestUserInteractor.kt @@ -173,7 +173,7 @@ constructor( } /** Removes the guest user from the device. */ - private suspend fun remove( + suspend fun remove( @UserIdInt guestUserId: Int, @UserIdInt targetUserId: Int, showDialog: (ShowDialogRequestModel) -> Unit, 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 e6bb9bcbc2645..a84238c1559a2 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 @@ -52,8 +52,7 @@ import com.android.systemui.user.legacyhelper.data.LegacyUserDataHelper import com.android.systemui.user.shared.model.UserActionModel import com.android.systemui.user.shared.model.UserModel import com.android.systemui.util.kotlin.pairwise -import java.util.Collections -import java.util.WeakHashMap +import java.io.PrintWriter import javax.inject.Inject import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.CoroutineScope @@ -70,6 +69,9 @@ import kotlinx.coroutines.flow.launchIn import kotlinx.coroutines.flow.map import kotlinx.coroutines.flow.onEach import kotlinx.coroutines.flow.stateIn +import kotlinx.coroutines.launch +import kotlinx.coroutines.sync.Mutex +import kotlinx.coroutines.sync.withLock import kotlinx.coroutines.withContext /** Encapsulates business logic to interact with user data and systems. */ @@ -96,7 +98,9 @@ constructor( * Defines interface for classes that can be notified when the state of users on the device is * changed. */ - fun interface UserCallback { + interface UserCallback { + /** Returns `true` if this callback can be cleaned-up. */ + fun isEvictable(): Boolean = false /** Notifies that the state of users on the device has changed. */ fun onUserStateChanged() } @@ -110,7 +114,8 @@ constructor( com.android.internal.R.string.config_supervisedUserCreationPackage ) - private val callbacks = Collections.newSetFromMap(WeakHashMap()) + private val callbackMutex = Mutex() + private val callbacks = mutableSetOf() /** List of current on-device users to select from. */ val users: Flow> @@ -263,6 +268,7 @@ constructor( } ) } + .onEach { notifyCallbacks() } .stateIn( scope = applicationScope, started = SharingStarted.Eagerly, @@ -307,30 +313,6 @@ constructor( error("Not supported in the old implementation!") } - fun addCallback(callback: UserCallback) { - callbacks.add(callback) - } - - fun removeCallback(callback: UserCallback) { - callbacks.remove(callback) - } - - fun onDialogShown() { - _dialogShowRequests.value = null - } - - fun onDialogDismissed() { - _dialogDismissRequests.value = null - } - - private fun showDialog(request: ShowDialogRequestModel) { - _dialogShowRequests.value = request - } - - private fun dismissDialog() { - _dialogDismissRequests.value = Unit - } - init { if (isNewImpl) { refreshUsersScheduler.refreshIfNotPaused() @@ -364,6 +346,46 @@ constructor( } } + fun addCallback(callback: UserCallback) { + applicationScope.launch { callbackMutex.withLock { callbacks.add(callback) } } + } + + fun removeCallback(callback: UserCallback) { + applicationScope.launch { callbackMutex.withLock { callbacks.remove(callback) } } + } + + fun refreshUsers() { + refreshUsersScheduler.refreshIfNotPaused() + } + + fun onDialogShown() { + _dialogShowRequests.value = null + } + + fun onDialogDismissed() { + _dialogDismissRequests.value = null + } + + fun dump(pw: PrintWriter) { + pw.println("UserInteractor state:") + pw.println(" lastSelectedNonGuestUserId=${repository.lastSelectedNonGuestUserId}") + + val users = userRecords.value.filter { it.info != null } + pw.println(" userCount=${userRecords.value.count { LegacyUserDataHelper.isUser(it) }}") + for (i in users.indices) { + pw.println(" ${users[i]}") + } + + val actions = userRecords.value.filter { it.info == null } + pw.println(" actionCount=${userRecords.value.count { !LegacyUserDataHelper.isUser(it) }}") + for (i in actions.indices) { + pw.println(" ${actions[i]}") + } + + pw.println("isSimpleUserSwitcher=$isSimpleUserSwitcher") + pw.println("isGuestUserAutoCreated=$isGuestUserAutoCreated") + } + fun onDeviceBootCompleted() { guestUserInteractor.onDeviceBootCompleted() } @@ -460,6 +482,60 @@ constructor( } } + fun exitGuestUser( + @UserIdInt guestUserId: Int, + @UserIdInt targetUserId: Int, + forceRemoveGuestOnExit: Boolean, + ) { + guestUserInteractor.exit( + guestUserId = guestUserId, + targetUserId = targetUserId, + forceRemoveGuestOnExit = forceRemoveGuestOnExit, + showDialog = this::showDialog, + dismissDialog = this::dismissDialog, + switchUser = this::switchUser, + ) + } + + fun removeGuestUser( + @UserIdInt guestUserId: Int, + @UserIdInt targetUserId: Int, + ) { + applicationScope.launch { + guestUserInteractor.remove( + guestUserId = guestUserId, + targetUserId = targetUserId, + ::showDialog, + ::dismissDialog, + ::selectUser, + ) + } + } + + private fun showDialog(request: ShowDialogRequestModel) { + _dialogShowRequests.value = request + } + + private fun dismissDialog() { + _dialogDismissRequests.value = Unit + } + + private fun notifyCallbacks() { + applicationScope.launch { + callbackMutex.withLock { + val iterator = callbacks.iterator() + while (iterator.hasNext()) { + val callback = iterator.next() + if (!callback.isEvictable()) { + callback.onUserStateChanged() + } else { + iterator.remove() + } + } + } + } + } + private suspend fun toRecord( userInfo: UserInfo, selectedUserId: Int, @@ -498,21 +574,6 @@ constructor( ) } - private fun exitGuestUser( - @UserIdInt guestUserId: Int, - @UserIdInt targetUserId: Int, - forceRemoveGuestOnExit: Boolean, - ) { - guestUserInteractor.exit( - guestUserId = guestUserId, - targetUserId = targetUserId, - forceRemoveGuestOnExit = forceRemoveGuestOnExit, - showDialog = this::showDialog, - dismissDialog = this::dismissDialog, - switchUser = this::switchUser, - ) - } - private fun switchUser(userId: Int) { // TODO(b/246631653): track jank and lantecy like in the old impl. refreshUsersScheduler.pause() @@ -533,7 +594,7 @@ constructor( dismissDialog() val selectedUserId = intent.getIntExtra(Intent.EXTRA_USER_HANDLE, -1) if (previousUserInfo?.id != selectedUserId) { - callbacks.forEach { it.onUserStateChanged() } + notifyCallbacks() restartSecondaryService(selectedUserId) } if (guestUserInteractor.isGuestUserAutoCreated) { diff --git a/packages/SystemUI/src/com/android/systemui/user/legacyhelper/data/LegacyUserDataHelper.kt b/packages/SystemUI/src/com/android/systemui/user/legacyhelper/data/LegacyUserDataHelper.kt index 8f6662f7b192f..137de1544b2d4 100644 --- a/packages/SystemUI/src/com/android/systemui/user/legacyhelper/data/LegacyUserDataHelper.kt +++ b/packages/SystemUI/src/com/android/systemui/user/legacyhelper/data/LegacyUserDataHelper.kt @@ -83,6 +83,21 @@ object LegacyUserDataHelper { ) } + fun toUserActionModel(record: UserRecord): UserActionModel { + check(!isUser(record)) + + return when { + record.isAddUser -> UserActionModel.ADD_USER + record.isAddSupervisedUser -> UserActionModel.ADD_SUPERVISED_USER + record.isGuest -> UserActionModel.ENTER_GUEST_MODE + else -> error("Not a known action: $record") + } + } + + fun isUser(record: UserRecord): Boolean { + return record.info != null + } + private fun getEnforcedAdmin( context: Context, selectedUserId: Int, diff --git a/packages/SystemUI/tests/src/com/android/systemui/user/domain/interactor/GuestUserInteractorTest.kt b/packages/SystemUI/tests/src/com/android/systemui/user/domain/interactor/GuestUserInteractorTest.kt index 6b4c9ed38b475..120bf791c4625 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/user/domain/interactor/GuestUserInteractorTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/user/domain/interactor/GuestUserInteractorTest.kt @@ -289,15 +289,73 @@ class GuestUserInteractorTest : SysuiTestCase() { verifyDidNotExit() } + @Test + fun `remove - returns to target user`() = + runBlocking(IMMEDIATE) { + whenever(manager.markGuestForDeletion(anyInt())).thenReturn(true) + repository.setSelectedUserInfo(GUEST_USER_INFO) + + val targetUserId = NON_GUEST_USER_INFO.id + underTest.remove( + guestUserId = GUEST_USER_INFO.id, + targetUserId = targetUserId, + showDialog = showDialog, + dismissDialog = dismissDialog, + switchUser = switchUser, + ) + + verify(manager).markGuestForDeletion(GUEST_USER_INFO.id) + verify(manager).removeUser(GUEST_USER_INFO.id) + verify(switchUser).invoke(targetUserId) + } + + @Test + fun `remove - selected different from guest user - do nothing`() = + runBlocking(IMMEDIATE) { + whenever(manager.markGuestForDeletion(anyInt())).thenReturn(true) + repository.setSelectedUserInfo(NON_GUEST_USER_INFO) + + underTest.remove( + guestUserId = GUEST_USER_INFO.id, + targetUserId = 123, + showDialog = showDialog, + dismissDialog = dismissDialog, + switchUser = switchUser, + ) + + verifyDidNotRemove() + } + + @Test + fun `remove - selected is actually not a guest user - do nothing`() = + runBlocking(IMMEDIATE) { + whenever(manager.markGuestForDeletion(anyInt())).thenReturn(true) + repository.setSelectedUserInfo(NON_GUEST_USER_INFO) + + underTest.remove( + guestUserId = NON_GUEST_USER_INFO.id, + targetUserId = 123, + showDialog = showDialog, + dismissDialog = dismissDialog, + switchUser = switchUser, + ) + + verifyDidNotRemove() + } + private fun setAllowedToAdd(isAllowed: Boolean = true) { whenever(deviceProvisionedController.isDeviceProvisioned).thenReturn(isAllowed) whenever(devicePolicyManager.isDeviceManaged).thenReturn(!isAllowed) } private fun verifyDidNotExit() { + verifyDidNotRemove() verify(manager, never()).getUserInfo(anyInt()) - verify(manager, never()).markGuestForDeletion(anyInt()) verify(uiEventLogger, never()).log(any()) + } + + private fun verifyDidNotRemove() { + verify(manager, never()).markGuestForDeletion(anyInt()) verify(showDialog, never()).invoke(any()) verify(dismissDialog, never()).invoke() verify(switchUser, never()).invoke(anyInt())