From 1a479f696062fadb6fda102a53fb5f501cc6346a Mon Sep 17 00:00:00 2001 From: Bryce Lee Date: Wed, 3 Nov 2021 16:33:19 -0700 Subject: [PATCH 1/2] Marshal CommunalCoordinator callbacks to main thread. This changelist makes sure callbacks in CommunalCoordinator happen on the main thread, preventing thread assertion issues in the notification logic. Bug: 205047954 Test: atest CommunalCoordinatorTest Change-Id: I6e3c2f2ef5d81baa253913f91e0f915754cf3746 --- .../coordinator/CommunalCoordinator.java | 14 +++++++++++--- .../coordinator/CommunalCoordinatorTest.java | 13 ++++++++++++- 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/CommunalCoordinator.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/CommunalCoordinator.java index 992d898531b80..bd011c3847cee 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/CommunalCoordinator.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/CommunalCoordinator.java @@ -19,6 +19,7 @@ package com.android.systemui.statusbar.notification.collection.coordinator; import androidx.annotation.NonNull; import com.android.systemui.communal.CommunalStateController; +import com.android.systemui.dagger.qualifiers.Main; import com.android.systemui.statusbar.NotificationLockscreenUserManager; import com.android.systemui.statusbar.notification.NotificationEntryManager; import com.android.systemui.statusbar.notification.collection.NotifPipeline; @@ -26,6 +27,8 @@ import com.android.systemui.statusbar.notification.collection.NotificationEntry; import com.android.systemui.statusbar.notification.collection.coordinator.dagger.CoordinatorScope; import com.android.systemui.statusbar.notification.collection.listbuilder.pluggable.NotifFilter; +import java.util.concurrent.Executor; + import javax.inject.Inject; /** @@ -34,14 +37,17 @@ import javax.inject.Inject; */ @CoordinatorScope public class CommunalCoordinator implements Coordinator { + final Executor mExecutor; final CommunalStateController mCommunalStateController; final NotificationEntryManager mNotificationEntryManager; final NotificationLockscreenUserManager mNotificationLockscreenUserManager; @Inject - public CommunalCoordinator(NotificationEntryManager notificationEntryManager, + public CommunalCoordinator(@Main Executor executor, + NotificationEntryManager notificationEntryManager, NotificationLockscreenUserManager notificationLockscreenUserManager, CommunalStateController communalStateController) { + mExecutor = executor; mNotificationEntryManager = notificationEntryManager; mNotificationLockscreenUserManager = notificationLockscreenUserManager; mCommunalStateController = communalStateController; @@ -57,8 +63,10 @@ public class CommunalCoordinator implements Coordinator { final CommunalStateController.Callback mStateCallback = new CommunalStateController.Callback() { @Override public void onCommunalViewShowingChanged() { - mFilter.invalidateList(); - mNotificationEntryManager.updateNotifications("Communal mode state changed"); + mExecutor.execute(() -> { + mFilter.invalidateList(); + mNotificationEntryManager.updateNotifications("Communal mode state changed"); + }); } }; diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/coordinator/CommunalCoordinatorTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/coordinator/CommunalCoordinatorTest.java index 01e4cce0cc304..f4452bc248b15 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/coordinator/CommunalCoordinatorTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/coordinator/CommunalCoordinatorTest.java @@ -17,6 +17,7 @@ package com.android.systemui.statusbar.notification.collection.coordinator; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import android.test.suitebuilder.annotation.SmallTest; @@ -29,6 +30,8 @@ import com.android.systemui.statusbar.notification.collection.NotifPipeline; import com.android.systemui.statusbar.notification.collection.NotificationEntry; import com.android.systemui.statusbar.notification.collection.listbuilder.pluggable.NotifFilter; import com.android.systemui.statusbar.notification.collection.listbuilder.pluggable.Pluggable; +import com.android.systemui.util.concurrency.FakeExecutor; +import com.android.systemui.util.time.FakeSystemClock; import org.junit.Before; import org.junit.Test; @@ -39,6 +42,8 @@ import org.mockito.MockitoAnnotations; @SmallTest public class CommunalCoordinatorTest extends SysuiTestCase { + private final FakeExecutor mExecutor = new FakeExecutor(new FakeSystemClock()); + @Mock CommunalStateController mCommunalStateController; @Mock @@ -57,7 +62,7 @@ public class CommunalCoordinatorTest extends SysuiTestCase { @Before public void setup() { MockitoAnnotations.initMocks(this); - mCoordinator = new CommunalCoordinator(mNotificationEntryManager, + mCoordinator = new CommunalCoordinator(mExecutor, mNotificationEntryManager, mNotificationLockscreenUserManager, mCommunalStateController); } @@ -84,6 +89,12 @@ public class CommunalCoordinatorTest extends SysuiTestCase { // Verify that notifications are filtered out when communal is showing and that the filter // pipeline is notified. stateCallback.onCommunalViewShowingChanged(); + // Make sure callback depends on executor to run. + verify(mFilterListener, never()).onPluggableInvalidated(any()); + verify(mNotificationEntryManager, never()).updateNotifications(any()); + + mExecutor.runAllReady(); + verify(mFilterListener).onPluggableInvalidated(any()); verify(mNotificationEntryManager).updateNotifications(any()); assert (filter.shouldFilterOut(mNotificationEntry, 0)); From 56bb75b1ac9ea816d430e67a3716d6ab502c7ba8 Mon Sep 17 00:00:00 2001 From: Bryce Lee Date: Mon, 11 Oct 2021 18:25:31 -0700 Subject: [PATCH 2/2] CommunalSourcePrimer Introduction. This changelist adds CommunalSourcePrimer, a SystemUI component for loading a communal source. Bug: 202398773 Test: atest CommunalSourcePrimerTest Change-Id: Iba295c93545d837659973ec6b6c1cce844010cd8 --- .../systemui/communal/CommunalSource.java | 24 +++ .../communal/CommunalSourcePrimer.java | 161 +++++++++++++++++ .../communal/CommunalSourcePrimerTest.java | 169 ++++++++++++++++++ 3 files changed, 354 insertions(+) create mode 100644 packages/SystemUI/src/com/android/systemui/communal/CommunalSourcePrimer.java create mode 100644 packages/SystemUI/tests/src/com/android/systemui/communal/CommunalSourcePrimerTest.java diff --git a/packages/SystemUI/src/com/android/systemui/communal/CommunalSource.java b/packages/SystemUI/src/com/android/systemui/communal/CommunalSource.java index 99c311e37217d..53586f58c5d2c 100644 --- a/packages/SystemUI/src/com/android/systemui/communal/CommunalSource.java +++ b/packages/SystemUI/src/com/android/systemui/communal/CommunalSource.java @@ -23,12 +23,36 @@ import com.android.systemui.util.ViewController; import com.google.common.util.concurrent.ListenableFuture; +import java.util.Optional; + /** * {@link CommunalSource} defines an interface for working with a source for communal data. Clients * may request a communal surface that can be shown within a {@link android.view.SurfaceView}. * Callbacks may also be registered to listen to state changes. */ public interface CommunalSource { + /** + * {@link Connector} defines an interface for {@link CommunalSource} instances to be generated. + */ + interface Connector { + ListenableFuture> connect(); + } + + /** + * The {@link Observer} interface specifies an entity which {@link CommunalSource} listeners + * can be informed of changes to the source, which will require updating. Note that this deals + * with changes to the source itself, not content which will be updated through the + * {@link CommunalSource} interface. + */ + interface Observer { + interface Callback { + void onSourceChanged(); + } + + void addCallback(Callback callback); + void removeCallback(Callback callback); + } + /** * {@link CommunalViewResult} is handed back from {@link #requestCommunalView(Context)} and * contains the view to be displayed and its associated controller. diff --git a/packages/SystemUI/src/com/android/systemui/communal/CommunalSourcePrimer.java b/packages/SystemUI/src/com/android/systemui/communal/CommunalSourcePrimer.java new file mode 100644 index 0000000000000..3c2b79e25467e --- /dev/null +++ b/packages/SystemUI/src/com/android/systemui/communal/CommunalSourcePrimer.java @@ -0,0 +1,161 @@ +/* + * Copyright (C) 2021 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.communal; + +import android.content.Context; +import android.content.res.Resources; +import android.util.Log; + +import com.android.systemui.CoreStartable; +import com.android.systemui.R; +import com.android.systemui.dagger.SysUISingleton; +import com.android.systemui.dagger.qualifiers.Main; +import com.android.systemui.util.concurrency.DelayableExecutor; + +import com.google.common.util.concurrent.ListenableFuture; + +import java.util.Optional; + +import javax.inject.Inject; + +/** + * The {@link CommunalSourcePrimer} is responsible for priming SystemUI with a pre-configured + * Communal source. The SystemUI service binds to the component to retrieve the + * {@link CommunalSource}. {@link CommunalSourcePrimer} has no effect + * if there is no pre-defined value. + */ +@SysUISingleton +public class CommunalSourcePrimer extends CoreStartable { + private static final String TAG = "CommunalSourcePrimer"; + private static final boolean DEBUG = Log.isLoggable(TAG, Log.DEBUG); + + private final DelayableExecutor mMainExecutor; + private final CommunalSourceMonitor mMonitor; + private final int mBaseReconnectDelayMs; + private final int mMaxReconnectAttempts; + + private int mReconnectAttempts = 0; + private Runnable mCurrentReconnectCancelable; + private ListenableFuture> mGetSourceFuture; + + private final Optional mConnector; + private final Optional mObserver; + + private final Runnable mConnectRunnable = new Runnable() { + @Override + public void run() { + mCurrentReconnectCancelable = null; + connect(); + } + }; + + @Inject + public CommunalSourcePrimer(Context context, @Main Resources resources, + DelayableExecutor mainExecutor, + CommunalSourceMonitor monitor, + Optional connector, + Optional observer) { + super(context); + mMainExecutor = mainExecutor; + mMonitor = monitor; + mConnector = connector; + mObserver = observer; + + mMaxReconnectAttempts = resources.getInteger( + R.integer.config_communalSourceMaxReconnectAttempts); + mBaseReconnectDelayMs = resources.getInteger( + R.integer.config_communalSourceReconnectBaseDelay); + } + + @Override + public void start() { + } + + private void initiateConnectionAttempt() { + // Reset attempts + mReconnectAttempts = 0; + mMonitor.setSource(null); + + // The first attempt is always a direct invocation rather than delayed. + connect(); + } + + private void scheduleConnectionAttempt() { + // always clear cancelable if present. + if (mCurrentReconnectCancelable != null) { + mCurrentReconnectCancelable.run(); + mCurrentReconnectCancelable = null; + } + + if (mReconnectAttempts >= mMaxReconnectAttempts) { + if (DEBUG) { + Log.d(TAG, "exceeded max connection attempts."); + } + return; + } + + final long reconnectDelayMs = + (long) Math.scalb(mBaseReconnectDelayMs, mReconnectAttempts); + + if (DEBUG) { + Log.d(TAG, + "scheduling connection attempt in " + reconnectDelayMs + "milliseconds"); + } + + mCurrentReconnectCancelable = mMainExecutor.executeDelayed(mConnectRunnable, + reconnectDelayMs); + + mReconnectAttempts++; + } + + @Override + protected void onBootCompleted() { + if (mObserver.isPresent()) { + mObserver.get().addCallback(() -> initiateConnectionAttempt()); + } + initiateConnectionAttempt(); + } + + private void connect() { + if (DEBUG) { + Log.d(TAG, "attempting to communal to communal source"); + } + + if (mGetSourceFuture != null) { + if (DEBUG) { + Log.d(TAG, "canceling in-flight connection"); + } + mGetSourceFuture.cancel(true); + } + + mGetSourceFuture = mConnector.get().connect(); + mGetSourceFuture.addListener(() -> { + try { + Optional result = mGetSourceFuture.get(); + if (result.isPresent()) { + final CommunalSource source = result.get(); + source.addCallback(() -> initiateConnectionAttempt()); + mMonitor.setSource(source); + } else { + scheduleConnectionAttempt(); + } + } catch (Exception e) { + e.printStackTrace(); + } + }, mMainExecutor); + } +} diff --git a/packages/SystemUI/tests/src/com/android/systemui/communal/CommunalSourcePrimerTest.java b/packages/SystemUI/tests/src/com/android/systemui/communal/CommunalSourcePrimerTest.java new file mode 100644 index 0000000000000..659b1a31a7a7b --- /dev/null +++ b/packages/SystemUI/tests/src/com/android/systemui/communal/CommunalSourcePrimerTest.java @@ -0,0 +1,169 @@ +/* + * Copyright (C) 2021 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.communal; + +import static org.mockito.Mockito.clearInvocations; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import android.content.Context; +import android.content.res.Resources; +import android.testing.AndroidTestingRunner; + +import androidx.concurrent.futures.CallbackToFutureAdapter; +import androidx.test.filters.SmallTest; + +import com.android.systemui.R; +import com.android.systemui.SysuiTestCase; +import com.android.systemui.util.concurrency.FakeExecutor; +import com.android.systemui.util.time.FakeSystemClock; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.ArgumentCaptor; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.MockitoAnnotations; + +import java.util.Optional; + +@SmallTest +@RunWith(AndroidTestingRunner.class) +public class CommunalSourcePrimerTest extends SysuiTestCase { + private static final String TEST_COMPONENT_NAME = "com.google.tests/.CommunalService"; + private static final int MAX_RETRIES = 5; + private static final int RETRY_DELAY_MS = 1000; + + @Mock + private Context mContext; + + @Mock + private Resources mResources; + + private FakeExecutor mFakeExecutor = new FakeExecutor(new FakeSystemClock()); + + @Mock + private CommunalSource mSource; + + @Mock + private CommunalSourceMonitor mCommunalSourceMonitor; + + @Mock + private CommunalSource.Connector mConnector; + + @Mock + private CommunalSource.Observer mObserver; + + private CommunalSourcePrimer mPrimer; + + @Before + public void setup() { + MockitoAnnotations.initMocks(this); + when(mResources.getInteger(R.integer.config_communalSourceMaxReconnectAttempts)) + .thenReturn(MAX_RETRIES); + when(mResources.getInteger(R.integer.config_communalSourceReconnectBaseDelay)) + .thenReturn(RETRY_DELAY_MS); + when(mResources.getString(R.string.config_communalSourceComponent)) + .thenReturn(TEST_COMPONENT_NAME); + + mPrimer = new CommunalSourcePrimer(mContext, mResources, mFakeExecutor, + mCommunalSourceMonitor, Optional.of(mConnector), Optional.of(mObserver)); + } + + @Test + public void testConnect() { + when(mConnector.connect()).thenReturn( + CallbackToFutureAdapter.getFuture(completer -> { + completer.set(Optional.of(mSource)); + return "test"; + })); + + mPrimer.onBootCompleted(); + mFakeExecutor.runAllReady(); + verify(mCommunalSourceMonitor).setSource(mSource); + } + + @Test + public void testRetryOnBindFailure() throws Exception { + when(mConnector.connect()).thenReturn( + CallbackToFutureAdapter.getFuture(completer -> { + completer.set(Optional.empty()); + return "test"; + })); + + mPrimer.onBootCompleted(); + mFakeExecutor.runAllReady(); + + // Verify attempts happen. Note that we account for the retries plus initial attempt, which + // is not scheduled. + for (int attemptCount = 0; attemptCount < MAX_RETRIES + 1; attemptCount++) { + verify(mConnector, times(1)).connect(); + clearInvocations(mConnector); + mFakeExecutor.advanceClockToNext(); + mFakeExecutor.runAllReady(); + } + + verify(mCommunalSourceMonitor, never()).setSource(Mockito.notNull()); + } + + @Test + public void testAttemptOnPackageChange() { + when(mConnector.connect()).thenReturn( + CallbackToFutureAdapter.getFuture(completer -> { + completer.set(Optional.empty()); + return "test"; + })); + + mPrimer.onBootCompleted(); + mFakeExecutor.runAllReady(); + + final ArgumentCaptor callbackCaptor = + ArgumentCaptor.forClass(CommunalSource.Observer.Callback.class); + verify(mObserver).addCallback(callbackCaptor.capture()); + + clearInvocations(mConnector); + callbackCaptor.getValue().onSourceChanged(); + + verify(mConnector, times(1)).connect(); + } + + @Test + public void testDisconnect() { + final ArgumentCaptor callbackCaptor = + ArgumentCaptor.forClass(CommunalSource.Callback.class); + + when(mConnector.connect()).thenReturn( + CallbackToFutureAdapter.getFuture(completer -> { + completer.set(Optional.of(mSource)); + return "test"; + })); + + mPrimer.onBootCompleted(); + mFakeExecutor.runAllReady(); + verify(mCommunalSourceMonitor).setSource(mSource); + verify(mSource).addCallback(callbackCaptor.capture()); + + clearInvocations(mConnector); + callbackCaptor.getValue().onDisconnected(); + mFakeExecutor.runAllReady(); + + verify(mConnector).connect(); + } +}