onNotificationExpansionChanged is called only when the expansion change...

is visible to users.

For example, if the notification is expanded in the background,
onNotificationExpansionChanged will be called only when the expanded
notification is visible to users.

Reason: we should only care about expansion change that is visible
to users, and at the moment that it is shown to user.
This also allows us to determine "has been visually expanded"
easily. With this change, our NotificationAssistantService can know
whether a notification has been visually expanded and calculate the CTR
of smart actions accordingly.

This should not break existing stuff in NMS.onNotificationExpansionChanged
because codes there checked isUserAction == True anyway,
which means the change must be also visible to users.

BUG: 120803809

Test: atest ExtServicesUnitTest
Test: atest ExpansionStateLoggerTest
Test: Manual test, try to expand and collapse notification.

Change-Id: Ibc6f939b2560b845de6a8a35b4557423b8a074f7
This commit is contained in:
Tony Mak
2019-01-07 14:40:39 +00:00
parent f6f46ce603
commit 202f25d62a
11 changed files with 319 additions and 64 deletions

View File

@@ -173,7 +173,8 @@ public abstract class NotificationAssistantService extends NotificationListenerS
}
/**
* Implement this to know when a notification is expanded / collapsed.
* Implement this to know when a notification change (expanded / collapsed) is visible to user.
*
* @param key the notification key
* @param isUserAction whether the expanded change is caused by user action.
* @param isExpanded whether the notification is expanded.

View File

@@ -344,7 +344,6 @@ public class Assistant extends NotificationAssistantService {
if (entry != null) {
entry.setSeen();
mAgingHelper.onNotificationSeen(entry);
mSmartActionsHelper.onNotificationSeen(entry);
}
}
} catch (Throwable e) {

View File

@@ -236,10 +236,6 @@ public class NotificationEntry {
return mSeen;
}
public boolean isExpanded() {
return mExpanded;
}
public boolean isShowActionEventLogged() {
return mIsShowActionEventLogged;
}

View File

@@ -152,18 +152,26 @@ public class SmartActionsHelper {
return replies;
}
void onNotificationSeen(@NonNull NotificationEntry entry) {
if (entry.isExpanded()) {
maybeSendActionShownEvent(entry);
}
}
void onNotificationExpansionChanged(@NonNull NotificationEntry entry, boolean isUserAction,
boolean isExpanded) {
// Notification can be expanded in the background, and thus the isUserAction check.
if (isUserAction && isExpanded) {
maybeSendActionShownEvent(entry);
if (!isExpanded) {
return;
}
String resultId = mNotificationKeyToResultIdCache.get(entry.getSbn().getKey());
if (resultId == null) {
return;
}
// Only report if this is the first time the user sees these suggestions.
if (entry.isShowActionEventLogged()) {
return;
}
entry.setShowActionEventLogged();
TextClassifierEvent textClassifierEvent =
createTextClassifierEventBuilder(TextClassifierEvent.TYPE_ACTIONS_SHOWN,
resultId)
.build();
// TODO: If possible, report which replies / actions are actually seen by user.
mTextClassifier.onTextClassifierEvent(textClassifierEvent);
}
void onNotificationDirectReplied(@NonNull String key) {
@@ -234,26 +242,6 @@ public class SmartActionsHelper {
.setResultId(resultId);
}
private void maybeSendActionShownEvent(@NonNull NotificationEntry entry) {
if (mTextClassifier == null) {
return;
}
String resultId = mNotificationKeyToResultIdCache.get(entry.getSbn().getKey());
if (resultId == null) {
return;
}
// Only report if this is the first time the user sees these suggestions.
if (entry.isShowActionEventLogged()) {
return;
}
entry.setShowActionEventLogged();
TextClassifierEvent textClassifierEvent =
createTextClassifierEventBuilder(TextClassifierEvent.TYPE_ACTIONS_SHOWN, resultId)
.build();
// TODO: If possible, report which replies / actions are actually seen by user.
mTextClassifier.onTextClassifierEvent(textClassifierEvent);
}
/**
* Returns whether a notification is eligible for action adjustments.
*

View File

@@ -273,24 +273,22 @@ public class SmartActionHelperTest {
final String message = "Where are you?";
Notification notification = mNotificationBuilder.setContentText(message).build();
when(mNotificationEntry.getNotification()).thenReturn(notification);
when(mNotificationEntry.isExpanded()).thenReturn(false);
mSmartActionsHelper.suggestReplies(mNotificationEntry);
mSmartActionsHelper.onNotificationSeen(mNotificationEntry);
mSmartActionsHelper.onNotificationExpansionChanged(mNotificationEntry, false, false);
verify(mTextClassifier, never()).onTextClassifierEvent(
Mockito.any(TextClassifierEvent.class));
}
@Test
public void testOnNotificationsSeen_expanded() {
public void testOnNotifications_expanded() {
final String message = "Where are you?";
Notification notification = mNotificationBuilder.setContentText(message).build();
when(mNotificationEntry.getNotification()).thenReturn(notification);
when(mNotificationEntry.isExpanded()).thenReturn(true);
mSmartActionsHelper.suggestReplies(mNotificationEntry);
mSmartActionsHelper.onNotificationSeen(mNotificationEntry);
mSmartActionsHelper.onNotificationExpansionChanged(mNotificationEntry, false, true);
ArgumentCaptor<TextClassifierEvent> argumentCaptor =
ArgumentCaptor.forClass(TextClassifierEvent.class);

View File

@@ -27,13 +27,10 @@ import android.content.Context;
import android.content.pm.ApplicationInfo;
import android.content.pm.PackageManager;
import android.os.Build;
import android.os.RemoteException;
import android.os.ServiceManager;
import android.service.notification.StatusBarNotification;
import android.util.Log;
import android.view.ViewGroup;
import com.android.internal.statusbar.IStatusBarService;
import com.android.internal.util.NotificationMessagingUtil;
import com.android.systemui.Dependency;
import com.android.systemui.R;
@@ -43,6 +40,7 @@ import com.android.systemui.statusbar.NotificationPresenter;
import com.android.systemui.statusbar.NotificationRemoteInputManager;
import com.android.systemui.statusbar.NotificationUiAdjustment;
import com.android.systemui.statusbar.notification.collection.NotificationEntry;
import com.android.systemui.statusbar.notification.logging.NotificationLogger;
import com.android.systemui.statusbar.notification.row.ExpandableNotificationRow;
import com.android.systemui.statusbar.notification.row.NotificationGutsManager;
import com.android.systemui.statusbar.notification.row.NotificationInflater;
@@ -71,7 +69,6 @@ public class NotificationRowBinder {
Dependency.get(NotificationInterruptionStateProvider.class);
private final Context mContext;
private final IStatusBarService mBarService;
private final NotificationMessagingUtil mMessagingUtil;
private final ExpandableNotificationRow.ExpansionLogger mExpansionLogger =
this::logNotificationExpansion;
@@ -85,6 +82,7 @@ public class NotificationRowBinder {
private ExpandableNotificationRow.OnAppOpsClickListener mOnAppOpsClickListener;
private BindRowCallback mBindRowCallback;
private NotificationClicker mNotificationClicker;
private final NotificationLogger mNotificationLogger = Dependency.get(NotificationLogger.class);
@Inject
public NotificationRowBinder(Context context,
@@ -92,8 +90,6 @@ public class NotificationRowBinder {
mContext = context;
mMessagingUtil = new NotificationMessagingUtil(context);
mAllowLongPress = allowLongPress;
mBarService = IStatusBarService.Stub.asInterface(
ServiceManager.getService(Context.STATUS_BAR_SERVICE));
}
private NotificationRemoteInputManager getRemoteInputManager() {
@@ -274,13 +270,7 @@ public class NotificationRowBinder {
}
private void logNotificationExpansion(String key, boolean userAction, boolean expanded) {
mUiOffloadThread.submit(() -> {
try {
mBarService.onNotificationExpansionChanged(key, userAction, expanded);
} catch (RemoteException e) {
// Ignore.
}
});
mNotificationLogger.onExpansionChanged(key, userAction, expanded);
}
/** Callback for when a row is bound to an entry. */

View File

@@ -23,9 +23,12 @@ import android.os.SystemClock;
import android.service.notification.NotificationListenerService;
import android.service.notification.NotificationStats;
import android.service.notification.StatusBarNotification;
import android.util.ArrayMap;
import android.util.ArraySet;
import android.util.Log;
import androidx.annotation.Nullable;
import com.android.internal.annotations.VisibleForTesting;
import com.android.internal.statusbar.IStatusBarService;
import com.android.internal.statusbar.NotificationVisibility;
@@ -42,6 +45,7 @@ import com.android.systemui.statusbar.policy.HeadsUpManager;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.Map;
import javax.inject.Inject;
import javax.inject.Singleton;
@@ -66,6 +70,7 @@ public class NotificationLogger implements StateListener {
private final UiOffloadThread mUiOffloadThread;
private final NotificationEntryManager mEntryManager;
private HeadsUpManager mHeadsUpManager;
private final ExpansionStateLogger mExpansionStateLogger;
protected Handler mHandler = new Handler();
protected IStatusBarService mBarService;
@@ -144,6 +149,9 @@ public class NotificationLogger implements StateListener {
recycleAllVisibilityObjects(mCurrentlyVisibleNotifications);
mCurrentlyVisibleNotifications.addAll(mTmpCurrentlyVisibleNotifications);
mExpansionStateLogger.onVisibilityChanged(
mTmpCurrentlyVisibleNotifications, mTmpCurrentlyVisibleNotifications);
recycleAllVisibilityObjects(mTmpNoLongerVisibleNotifications);
mTmpCurrentlyVisibleNotifications.clear();
mTmpNewlyVisibleNotifications.clear();
@@ -155,12 +163,14 @@ public class NotificationLogger implements StateListener {
public NotificationLogger(NotificationListener notificationListener,
UiOffloadThread uiOffloadThread,
NotificationEntryManager entryManager,
StatusBarStateController statusBarStateController) {
StatusBarStateController statusBarStateController,
ExpansionStateLogger expansionStateLogger) {
mNotificationListener = notificationListener;
mUiOffloadThread = uiOffloadThread;
mEntryManager = entryManager;
mBarService = IStatusBarService.Stub.asInterface(
ServiceManager.getService(Context.STATUS_BAR_SERVICE));
mExpansionStateLogger = expansionStateLogger;
// Not expected to be destroyed, don't need to unsubscribe
statusBarStateController.addCallback(this);
@@ -173,6 +183,7 @@ public class NotificationLogger implements StateListener {
if (removedByUser && visibility != null) {
logNotificationClear(entry.key, entry.notification, visibility);
}
mExpansionStateLogger.onEntryRemoved(entry.key);
}
@Override
@@ -319,8 +330,8 @@ public class NotificationLogger implements StateListener {
}
}
private NotificationVisibility[] cloneVisibilitiesAsArr(Collection<NotificationVisibility> c) {
private static NotificationVisibility[] cloneVisibilitiesAsArr(
Collection<NotificationVisibility> c) {
final NotificationVisibility[] array = new NotificationVisibility[c.size()];
int i = 0;
for(NotificationVisibility nv: c) {
@@ -347,10 +358,134 @@ public class NotificationLogger implements StateListener {
setDozing(isDozing);
}
/**
* Called when the notification is expanded / collapsed.
*/
public void onExpansionChanged(String key, boolean isUserAction, boolean isExpanded) {
mExpansionStateLogger.onExpansionChanged(key, isUserAction, isExpanded);
}
/**
* A listener that is notified when some child locations might have changed.
*/
public interface OnChildLocationsChangedListener {
void onChildLocationsChanged();
}
/**
* Logs the expansion state change when the notification is visible.
*/
public static class ExpansionStateLogger {
/** Notification key -> state, should be accessed in UI offload thread only. */
private final Map<String, State> mExpansionStates = new ArrayMap<>();
/**
* Notification key -> last logged expansion state, should be accessed in UI thread only.
*/
private final Map<String, Boolean> mLoggedExpansionState = new ArrayMap<>();
private final UiOffloadThread mUiOffloadThread;
@VisibleForTesting
IStatusBarService mBarService;
@Inject
public ExpansionStateLogger(UiOffloadThread uiOffloadThread) {
mUiOffloadThread = uiOffloadThread;
mBarService =
IStatusBarService.Stub.asInterface(
ServiceManager.getService(Context.STATUS_BAR_SERVICE));
}
@VisibleForTesting
void onExpansionChanged(String key, boolean isUserAction, boolean isExpanded) {
State state = getState(key);
state.mIsUserAction = isUserAction;
state.mIsExpanded = isExpanded;
maybeNotifyOnNotificationExpansionChanged(key, state);
}
@VisibleForTesting
void onVisibilityChanged(
Collection<NotificationVisibility> newlyVisible,
Collection<NotificationVisibility> noLongerVisible) {
final NotificationVisibility[] newlyVisibleAr =
cloneVisibilitiesAsArr(newlyVisible);
final NotificationVisibility[] noLongerVisibleAr =
cloneVisibilitiesAsArr(noLongerVisible);
for (NotificationVisibility nv : newlyVisibleAr) {
State state = getState(nv.key);
state.mIsVisible = true;
maybeNotifyOnNotificationExpansionChanged(nv.key, state);
}
for (NotificationVisibility nv : noLongerVisibleAr) {
State state = getState(nv.key);
state.mIsVisible = false;
}
}
@VisibleForTesting
void onEntryRemoved(String key) {
mExpansionStates.remove(key);
mLoggedExpansionState.remove(key);
}
private State getState(String key) {
State state = mExpansionStates.get(key);
if (state == null) {
state = new State();
mExpansionStates.put(key, state);
}
return state;
}
private void maybeNotifyOnNotificationExpansionChanged(final String key, State state) {
if (!state.isFullySet()) {
return;
}
if (!state.mIsVisible) {
return;
}
Boolean loggedExpansionState = mLoggedExpansionState.get(key);
// Consider notification is initially collapsed, so only expanded is logged in the
// first time.
if (loggedExpansionState == null && !state.mIsExpanded) {
return;
}
if (loggedExpansionState != null
&& state.mIsExpanded == loggedExpansionState) {
return;
}
mLoggedExpansionState.put(key, state.mIsExpanded);
final State stateToBeLogged = new State(state);
mUiOffloadThread.submit(() -> {
try {
mBarService.onNotificationExpansionChanged(
key, stateToBeLogged.mIsUserAction, stateToBeLogged.mIsExpanded);
} catch (RemoteException e) {
Log.e(TAG, "Failed to call onNotificationExpansionChanged: ", e);
}
});
}
private static class State {
@Nullable
Boolean mIsUserAction;
@Nullable
Boolean mIsExpanded;
@Nullable
Boolean mIsVisible;
private State() {}
private State(State state) {
this.mIsUserAction = state.mIsUserAction;
this.mIsExpanded = state.mIsExpanded;
this.mIsVisible = state.mIsVisible;
}
private boolean isFullySet() {
return mIsUserAction != null && mIsExpanded != null && mIsVisible != null;
}
}
}
}

View File

@@ -0,0 +1,145 @@
/**
* Copyright (C) 2018 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.statusbar.notification.logging;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyBoolean;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.verify;
import android.os.RemoteException;
import android.support.test.filters.SmallTest;
import android.testing.AndroidTestingRunner;
import android.testing.TestableLooper;
import com.android.internal.statusbar.IStatusBarService;
import com.android.internal.statusbar.NotificationVisibility;
import com.android.systemui.Dependency;
import com.android.systemui.SysuiTestCase;
import com.android.systemui.UiOffloadThread;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.Mock;
import org.mockito.Mockito;
import org.mockito.MockitoAnnotations;
import java.util.Collections;
@SmallTest
@RunWith(AndroidTestingRunner.class)
@TestableLooper.RunWithLooper
public class ExpansionStateLoggerTest extends SysuiTestCase {
private static final String NOTIFICATION_KEY = "notin_key";
private NotificationLogger.ExpansionStateLogger mLogger;
@Mock
private IStatusBarService mBarService;
@Before
public void setUp() {
MockitoAnnotations.initMocks(this);
mLogger = new NotificationLogger.ExpansionStateLogger(
Dependency.get(UiOffloadThread.class));
mLogger.mBarService = mBarService;
}
@Test
public void testVisible() throws RemoteException {
mLogger.onVisibilityChanged(
Collections.singletonList(createNotificationVisibility(NOTIFICATION_KEY, true)),
Collections.emptyList());
waitForUiOffloadThread();
verify(mBarService, Mockito.never()).onNotificationExpansionChanged(
eq(NOTIFICATION_KEY), anyBoolean(), anyBoolean());
}
@Test
public void testExpanded() throws RemoteException {
mLogger.onExpansionChanged(NOTIFICATION_KEY, false, true);
waitForUiOffloadThread();
verify(mBarService, Mockito.never()).onNotificationExpansionChanged(
eq(NOTIFICATION_KEY), anyBoolean(), anyBoolean());
}
@Test
public void testVisibleAndNotExpanded() throws RemoteException {
mLogger.onExpansionChanged(NOTIFICATION_KEY, true, false);
mLogger.onVisibilityChanged(
Collections.singletonList(createNotificationVisibility(NOTIFICATION_KEY, true)),
Collections.emptyList());
waitForUiOffloadThread();
verify(mBarService, Mockito.never()).onNotificationExpansionChanged(
eq(NOTIFICATION_KEY), anyBoolean(), anyBoolean());
}
@Test
public void testVisibleAndExpanded() throws RemoteException {
mLogger.onExpansionChanged(NOTIFICATION_KEY, true, true);
mLogger.onVisibilityChanged(
Collections.singletonList(createNotificationVisibility(NOTIFICATION_KEY, true)),
Collections.emptyList());
waitForUiOffloadThread();
verify(mBarService).onNotificationExpansionChanged(
NOTIFICATION_KEY, true, true);
}
@Test
public void testExpandedAndVisible_expandedBeforeVisible() throws RemoteException {
mLogger.onExpansionChanged(NOTIFICATION_KEY, false, true);
mLogger.onVisibilityChanged(
Collections.singletonList(createNotificationVisibility(NOTIFICATION_KEY, true)),
Collections.emptyList());
waitForUiOffloadThread();
verify(mBarService).onNotificationExpansionChanged(
NOTIFICATION_KEY, false, true);
}
@Test
public void testExpandedAndVisible_visibleBeforeExpanded() throws RemoteException {
mLogger.onVisibilityChanged(
Collections.singletonList(createNotificationVisibility(NOTIFICATION_KEY, true)),
Collections.emptyList());
mLogger.onExpansionChanged(NOTIFICATION_KEY, false, true);
waitForUiOffloadThread();
verify(mBarService).onNotificationExpansionChanged(
NOTIFICATION_KEY, false, true);
}
@Test
public void testExpandedAndVisible_logOnceOnly() throws RemoteException {
mLogger.onVisibilityChanged(
Collections.singletonList(createNotificationVisibility(NOTIFICATION_KEY, true)),
Collections.emptyList());
mLogger.onExpansionChanged(NOTIFICATION_KEY, false, true);
mLogger.onExpansionChanged(NOTIFICATION_KEY, false, true);
waitForUiOffloadThread();
verify(mBarService).onNotificationExpansionChanged(
NOTIFICATION_KEY, false, true);
}
private NotificationVisibility createNotificationVisibility(String key, boolean visibility) {
return NotificationVisibility.obtain(key, 0, 0, visibility);
}
}

View File

@@ -72,6 +72,7 @@ public class NotificationLoggerTest extends SysuiTestCase {
@Mock private IStatusBarService mBarService;
@Mock private NotificationData mNotificationData;
@Mock private ExpandableNotificationRow mRow;
@Mock private NotificationLogger.ExpansionStateLogger mExpansionStateLogger;
// Dependency mocks:
@Mock private NotificationEntryManager mEntryManager;
@@ -98,7 +99,8 @@ public class NotificationLoggerTest extends SysuiTestCase {
mEntry.setRow(mRow);
mLogger = new TestableNotificationLogger(mListener, Dependency.get(UiOffloadThread.class),
mEntryManager, mock(StatusBarStateController.class), mBarService);
mEntryManager, mock(StatusBarStateController.class), mBarService,
mExpansionStateLogger);
mLogger.setUpWithContainer(mListContainer);
verify(mEntryManager).addNotificationEntryListener(mEntryListenerCaptor.capture());
mNotificationEntryListener = mEntryListenerCaptor.getValue();
@@ -166,8 +168,10 @@ public class NotificationLoggerTest extends SysuiTestCase {
UiOffloadThread uiOffloadThread,
NotificationEntryManager entryManager,
StatusBarStateController statusBarStateController,
IStatusBarService barService) {
super(notificationListener, uiOffloadThread, entryManager, statusBarStateController);
IStatusBarService barService,
ExpansionStateLogger expansionStateLogger) {
super(notificationListener, uiOffloadThread, entryManager, statusBarStateController,
expansionStateLogger);
mBarService = barService;
// Make this on the current thread so we can wait for it during tests.
mHandler = Handler.createAsync(Looper.myLooper());

View File

@@ -159,6 +159,8 @@ public class StatusBarTest extends SysuiTestCase {
private NotificationFilter mNotificationFilter;
@Mock
private NotificationAlertingManager mNotificationAlertingManager;
@Mock
private NotificationLogger.ExpansionStateLogger mExpansionStateLogger;
private TestableStatusBar mStatusBar;
private FakeMetricsLogger mMetricsLogger;
@@ -207,7 +209,8 @@ public class StatusBarTest extends SysuiTestCase {
mDependency.injectTestDependency(MetricsLogger.class, mMetricsLogger);
mEntryManager = new TestableNotificationEntryManager(mContext);
mNotificationLogger = new NotificationLogger(mNotificationListener,
Dependency.get(UiOffloadThread.class), mEntryManager, mStatusBarStateController);
Dependency.get(UiOffloadThread.class), mEntryManager, mStatusBarStateController,
mExpansionStateLogger);
mDependency.injectTestDependency(NotificationLogger.class, mNotificationLogger);
DozeLog.traceDozing(mContext, false /* dozing */);

View File

@@ -878,7 +878,6 @@ public class NotificationManagerService extends SystemService {
if (r.hasBeenVisiblyExpanded()) {
logSmartSuggestionsVisible(r);
}
final long now = System.currentTimeMillis();
if (userAction) {
MetricsLogger.action(r.getItemLogMaker()
.setType(expanded ? MetricsEvent.TYPE_DETAIL
@@ -888,9 +887,6 @@ public class NotificationManagerService extends SystemService {
r.recordExpanded();
reportUserInteraction(r);
}
EventLogTags.writeNotificationExpansion(key,
userAction ? 1 : 0, expanded ? 1 : 0,
r.getLifespanMs(now), r.getFreshnessMs(now), r.getExposureMs(now));
mAssistants.notifyAssistantExpansionChangedLocked(r.sbn, userAction, expanded);
}
}