From 960f193048fb4f2bcfd72264d379b8e26298a51b Mon Sep 17 00:00:00 2001 From: Matt Pietal Date: Tue, 19 May 2020 15:41:26 -0400 Subject: [PATCH] Controls UI - Multi-user switch updates Do not propogate events on a user switch. Do not clear all services either, as this can trigger unwanted downstream logic to believe the components were removed. Bug: 156758594 Test: atest ControlsListingControllerImplTest ControlsControllerImplTest Change-Id: Iee38f358dbcbbfc4a25866599fc40076b93ac533 --- .../controller/ControlsControllerImpl.kt | 20 ++++----- .../ControlsListingControllerImpl.kt | 41 +++++++++++-------- .../controller/ControlsControllerImplTest.kt | 18 -------- .../ControlsListingControllerImplTest.kt | 19 ++++++++- 4 files changed, 49 insertions(+), 49 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/controls/controller/ControlsControllerImpl.kt b/packages/SystemUI/src/com/android/systemui/controls/controller/ControlsControllerImpl.kt index 181170b0dee5f..f8f913e8573d3 100644 --- a/packages/SystemUI/src/com/android/systemui/controls/controller/ControlsControllerImpl.kt +++ b/packages/SystemUI/src/com/android/systemui/controls/controller/ControlsControllerImpl.kt @@ -72,6 +72,9 @@ class ControlsControllerImpl @Inject constructor ( private const val USER_CHANGE_RETRY_DELAY = 500L // ms private const val DEFAULT_ENABLED = 1 private const val PERMISSION_SELF = "com.android.systemui.permission.SELF" + + private fun isAvailable(userId: Int, cr: ContentResolver) = Settings.Secure.getIntForUser( + cr, CONTROLS_AVAILABLE, DEFAULT_ENABLED, userId) != 0 } private var userChanging: Boolean = true @@ -85,8 +88,7 @@ class ControlsControllerImpl @Inject constructor ( private val contentResolver: ContentResolver get() = context.contentResolver - override var available = Settings.Secure.getIntForUser( - contentResolver, CONTROLS_AVAILABLE, DEFAULT_ENABLED, currentUserId) != 0 + override var available = isAvailable(currentUserId, contentResolver) private set private val persistenceWrapper: ControlsFavoritePersistenceWrapper @@ -119,8 +121,7 @@ class ControlsControllerImpl @Inject constructor ( BackupManager(userStructure.userContext) ) auxiliaryPersistenceWrapper.changeFile(userStructure.auxiliaryFile) - available = Settings.Secure.getIntForUser(contentResolver, CONTROLS_AVAILABLE, - DEFAULT_ENABLED, newUser.identifier) != 0 + available = isAvailable(newUser.identifier, contentResolver) resetFavorites(available) bindingController.changeUser(newUser) listingController.changeUser(newUser) @@ -131,7 +132,6 @@ class ControlsControllerImpl @Inject constructor ( override fun onReceive(context: Context, intent: Intent) { if (intent.action == Intent.ACTION_USER_SWITCHED) { userChanging = true - listingController.removeCallback(listingCallback) val newUser = UserHandle.of(intent.getIntExtra(Intent.EXTRA_USER_HANDLE, sendingUserId)) if (currentUser == newUser) { @@ -151,7 +151,6 @@ class ControlsControllerImpl @Inject constructor ( executor.execute { Log.d(TAG, "Restore finished, storing auxiliary favorites") auxiliaryPersistenceWrapper.initialize() - listingController.removeCallback(listingCallback) persistenceWrapper.storeFavorites(auxiliaryPersistenceWrapper.favorites) resetFavorites(available) } @@ -172,8 +171,7 @@ class ControlsControllerImpl @Inject constructor ( if (userChanging || userId != currentUserId) { return } - available = Settings.Secure.getIntForUser(contentResolver, CONTROLS_AVAILABLE, - DEFAULT_ENABLED, currentUserId) != 0 + available = isAvailable(currentUserId, contentResolver) resetFavorites(available) } } @@ -244,6 +242,7 @@ class ControlsControllerImpl @Inject constructor ( null ) contentResolver.registerContentObserver(URI, false, settingObserver, UserHandle.USER_ALL) + listingController.addCallback(listingCallback) } fun destroy() { @@ -258,7 +257,6 @@ class ControlsControllerImpl @Inject constructor ( if (shouldLoad) { Favorites.load(persistenceWrapper.readFavorites()) - listingController.addCallback(listingCallback) } } @@ -569,12 +567,12 @@ class UserStructure(context: Context, user: UserHandle) { val userContext = context.createContextAsUser(user, 0) val file = Environment.buildPath( - context.filesDir, + userContext.filesDir, ControlsFavoritePersistenceWrapper.FILE_NAME ) val auxiliaryFile = Environment.buildPath( - context.filesDir, + userContext.filesDir, AuxiliaryPersistenceWrapper.AUXILIARY_FILE_NAME ) } diff --git a/packages/SystemUI/src/com/android/systemui/controls/management/ControlsListingControllerImpl.kt b/packages/SystemUI/src/com/android/systemui/controls/management/ControlsListingControllerImpl.kt index a2adcf953cc43..1cd9712cd01bf 100644 --- a/packages/SystemUI/src/com/android/systemui/controls/management/ControlsListingControllerImpl.kt +++ b/packages/SystemUI/src/com/android/systemui/controls/management/ControlsListingControllerImpl.kt @@ -29,6 +29,7 @@ import com.android.settingslib.widget.CandidateInfo import com.android.systemui.controls.ControlsServiceInfo import com.android.systemui.dagger.qualifiers.Background import java.util.concurrent.Executor +import java.util.concurrent.atomic.AtomicInteger import javax.inject.Inject import javax.inject.Singleton @@ -75,6 +76,7 @@ class ControlsListingControllerImpl @VisibleForTesting constructor( private var availableComponents = emptySet() private var availableServices = emptyList() + private var userChangeInProgress = AtomicInteger(0) override var currentUserId = ActivityManager.getCurrentUser() private set @@ -85,6 +87,7 @@ class ControlsListingControllerImpl @VisibleForTesting constructor( newServices.mapTo(mutableSetOf(), { s -> s.getComponentName() }) backgroundExecutor.execute { + if (userChangeInProgress.get() > 0) return@execute if (!newComponents.equals(availableComponents)) { Log.d(TAG, "ServiceConfig reloaded, count: ${newComponents.size}") availableComponents = newComponents @@ -105,22 +108,18 @@ class ControlsListingControllerImpl @VisibleForTesting constructor( } override fun changeUser(newUser: UserHandle) { + userChangeInProgress.incrementAndGet() + serviceListing.setListening(false) + backgroundExecutor.execute { - serviceListing.setListening(false) - - // Notify all callbacks in order to clear their existing state prior to attaching - // a new listener - availableServices = emptyList() - callbacks.forEach { - it.onServicesUpdated(emptyList()) + if (userChangeInProgress.decrementAndGet() == 0) { + currentUserId = newUser.identifier + val contextForUser = context.createContextAsUser(newUser, 0) + serviceListing = serviceListingBuilder(contextForUser) + serviceListing.addCallback(serviceListingCallback) + serviceListing.setListening(true) + serviceListing.reload() } - - currentUserId = newUser.identifier - val contextForUser = context.createContextAsUser(newUser, 0) - serviceListing = serviceListingBuilder(contextForUser) - serviceListing.addCallback(serviceListingCallback) - serviceListing.setListening(true) - serviceListing.reload() } } @@ -134,10 +133,16 @@ class ControlsListingControllerImpl @VisibleForTesting constructor( */ override fun addCallback(listener: ControlsListingController.ControlsListingCallback) { backgroundExecutor.execute { - val services = getCurrentServices() - Log.d(TAG, "Subscribing callback, service count: ${services.size}") - callbacks.add(listener) - listener.onServicesUpdated(services) + if (userChangeInProgress.get() > 0) { + // repost this event, as callers may rely on the initial callback from + // onServicesUpdated + addCallback(listener) + } else { + val services = getCurrentServices() + Log.d(TAG, "Subscribing callback, service count: ${services.size}") + callbacks.add(listener) + listener.onServicesUpdated(services) + } } } diff --git a/packages/SystemUI/tests/src/com/android/systemui/controls/controller/ControlsControllerImplTest.kt b/packages/SystemUI/tests/src/com/android/systemui/controls/controller/ControlsControllerImplTest.kt index e3f25c6b0c726..b3888875e82e4 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/controls/controller/ControlsControllerImplTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/controls/controller/ControlsControllerImplTest.kt @@ -796,24 +796,6 @@ class ControlsControllerImplTest : SysuiTestCase() { .getCachedFavoritesAndRemoveFor(TEST_COMPONENT_2) } - @Test - fun testListingCallbackNotListeningWhileReadingFavorites() { - val intent = Intent(Intent.ACTION_USER_SWITCHED).apply { - putExtra(Intent.EXTRA_USER_HANDLE, otherUser) - } - val pendingResult = mock(BroadcastReceiver.PendingResult::class.java) - `when`(pendingResult.sendingUserId).thenReturn(otherUser) - broadcastReceiverCaptor.value.pendingResult = pendingResult - - broadcastReceiverCaptor.value.onReceive(mContext, intent) - - val inOrder = inOrder(persistenceWrapper, listingController) - - inOrder.verify(listingController).removeCallback(listingCallbackCaptor.value) - inOrder.verify(persistenceWrapper).readFavorites() - inOrder.verify(listingController).addCallback(listingCallbackCaptor.value) - } - @Test fun testSeedFavoritesForComponent() { var succeeded = false diff --git a/packages/SystemUI/tests/src/com/android/systemui/controls/management/ControlsListingControllerImplTest.kt b/packages/SystemUI/tests/src/com/android/systemui/controls/management/ControlsListingControllerImplTest.kt index 128a7e856ec52..841b2553ebda9 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/controls/management/ControlsListingControllerImplTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/controls/management/ControlsListingControllerImplTest.kt @@ -65,7 +65,10 @@ class ControlsListingControllerImplTest : SysuiTestCase() { @Mock private lateinit var serviceInfo: ServiceInfo @Mock - private lateinit var componentName: ComponentName + private lateinit var serviceInfo2: ServiceInfo + + private var componentName = ComponentName("pkg1", "class1") + private var componentName2 = ComponentName("pkg2", "class2") private val executor = FakeExecutor(FakeSystemClock()) @@ -82,6 +85,7 @@ class ControlsListingControllerImplTest : SysuiTestCase() { MockitoAnnotations.initMocks(this) `when`(serviceInfo.componentName).thenReturn(componentName) + `when`(serviceInfo2.componentName).thenReturn(componentName2) val wrapper = object : ContextWrapper(mContext) { override fun createContextAsUser(user: UserHandle, flags: Int): Context { @@ -179,7 +183,7 @@ class ControlsListingControllerImplTest : SysuiTestCase() { } @Test - fun testChangeUserResetsExistingCallbackServices() { + fun testChangeUserSendsCorrectServiceUpdate() { val list = listOf(serviceInfo) controller.addCallback(mockCallback) @@ -197,10 +201,21 @@ class ControlsListingControllerImplTest : SysuiTestCase() { assertEquals(1, captor.value.size) reset(mockCallback) + reset(mockSL) + + val updatedList = listOf(serviceInfo) + serviceListingCallbackCaptor.value.onServicesReloaded(updatedList) controller.changeUser(UserHandle.of(otherUser)) executor.runAllReady() assertEquals(otherUser, controller.currentUserId) + // this event should was triggered just before the user change, and should + // be ignored + verify(mockCallback, never()).onServicesUpdated(any()) + + serviceListingCallbackCaptor.value.onServicesReloaded(emptyList()) + executor.runAllReady() + verify(mockCallback).onServicesUpdated(capture(captor)) assertEquals(0, captor.value.size) }