From 405e0b705eea943dea9daa58d5a1b9c191472701 Mon Sep 17 00:00:00 2001 From: Lyn Han Date: Tue, 13 Aug 2019 16:07:55 -0700 Subject: [PATCH] Dedup smart reply updates for bubble flyout Smart reply updates are not visually interruptive and bubbles should not show flyout for them, since flyout text remains the same. 1) Modify NoMan.isVisuallyInterruptive to skip evaluation of fields irrelevant to bubbles 2) Modify NotificationComparator to rank interruptive notifs higher 3) Pipe bool (isInterruptive) from system_process to SysUI NoMan --- set bool on notif record and ranking Ranking --- parcel bool for cross-process transport SysUI notif entry --- get bool from ranking SysUI bubble data --- on notif entry update, suppress flyout if isInterruptive=false Considered adding "isInterruptive" bool to StatusBarNotification. Did not because "visually-interruptive" is additional information that the system figured out and SBNs should be limited to info from the app. 4) NoMan --- schedule ranking update if interruptive changes for bubble Fixes: 138755533 Test: manual - send one sms => flyout appears once Test: manual - send multiple sms in a row => flyout appears for each one Test: atest FrameworksUiServicesTests Test: atest NotificationComparatorTest Test: atest SystemUITests Change-Id: Id4b855054689ee73a109bb7cd18004531b41f28c (cherry picked from commit 0dddc61824d091e48962e36939828cae3cde2aa9) --- .../NotificationListenerService.java | 15 +++++- .../android/systemui/bubbles/BubbleData.java | 4 ++ .../collection/NotificationEntry.java | 8 ++++ .../systemui/statusbar/RankingBuilder.java | 4 +- .../NotificationEntryManagerTest.java | 4 +- .../collection/NotificationDataTest.java | 5 +- .../notification/NotificationComparator.java | 6 +++ .../NotificationManagerService.java | 46 ++++++++++++++----- .../NotificationComparatorTest.java | 15 ++++++ .../NotificationListenerServiceTest.java | 15 ++++-- 10 files changed, 101 insertions(+), 21 deletions(-) diff --git a/core/java/android/service/notification/NotificationListenerService.java b/core/java/android/service/notification/NotificationListenerService.java index 78e30ab8cdc31..85f13d552fcfe 100644 --- a/core/java/android/service/notification/NotificationListenerService.java +++ b/core/java/android/service/notification/NotificationListenerService.java @@ -1522,6 +1522,7 @@ public abstract class NotificationListenerService extends Service { private ArrayList mSmartActions; private ArrayList mSmartReplies; private boolean mCanBubble; + private boolean mVisuallyInterruptive; private static final int PARCEL_VERSION = 2; @@ -1553,6 +1554,7 @@ public abstract class NotificationListenerService extends Service { out.writeTypedList(mSmartActions, flags); out.writeCharSequenceList(mSmartReplies); out.writeBoolean(mCanBubble); + out.writeBoolean(mVisuallyInterruptive); } /** @hide */ @@ -1585,6 +1587,7 @@ public abstract class NotificationListenerService extends Service { mSmartActions = in.createTypedArrayList(Notification.Action.CREATOR); mSmartReplies = in.readCharSequenceList(); mCanBubble = in.readBoolean(); + mVisuallyInterruptive = in.readBoolean(); } @@ -1771,6 +1774,11 @@ public abstract class NotificationListenerService extends Service { return mCanBubble; } + /** @hide */ + public boolean visuallyInterruptive() { + return mVisuallyInterruptive; + } + /** @hide */ public boolean isNoisy() { return mNoisy; @@ -1787,7 +1795,8 @@ public abstract class NotificationListenerService extends Service { ArrayList snoozeCriteria, boolean showBadge, int userSentiment, boolean hidden, long lastAudiblyAlertedMs, boolean noisy, ArrayList smartActions, - ArrayList smartReplies, boolean canBubble) { + ArrayList smartReplies, boolean canBubble, + boolean visuallyInterruptive) { mKey = key; mRank = rank; mIsAmbient = importance < NotificationManager.IMPORTANCE_LOW; @@ -1808,6 +1817,7 @@ public abstract class NotificationListenerService extends Service { mSmartActions = smartActions; mSmartReplies = smartReplies; mCanBubble = canBubble; + mVisuallyInterruptive = visuallyInterruptive; } /** @@ -1832,7 +1842,8 @@ public abstract class NotificationListenerService extends Service { other.mNoisy, other.mSmartActions, other.mSmartReplies, - other.mCanBubble); + other.mCanBubble, + other.mVisuallyInterruptive); } /** diff --git a/packages/SystemUI/src/com/android/systemui/bubbles/BubbleData.java b/packages/SystemUI/src/com/android/systemui/bubbles/BubbleData.java index 81c8da8247eca..dbc915a09a76f 100644 --- a/packages/SystemUI/src/com/android/systemui/bubbles/BubbleData.java +++ b/packages/SystemUI/src/com/android/systemui/bubbles/BubbleData.java @@ -184,6 +184,8 @@ public class BubbleData { Log.d(TAG, "notificationEntryUpdated: " + entry); } Bubble bubble = getBubbleWithKey(entry.key); + suppressFlyout = !entry.isVisuallyInterruptive || suppressFlyout; + if (bubble == null) { // Create a new bubble bubble = new Bubble(mContext, entry); @@ -193,8 +195,10 @@ public class BubbleData { } else { // Updates an existing bubble bubble.updateEntry(entry); + bubble.setSuppressFlyout(suppressFlyout); doUpdate(bubble); } + if (bubble.shouldAutoExpand()) { setSelectedBubbleInternal(bubble); if (!mExpanded) { diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotificationEntry.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotificationEntry.java index a33d23c0b5d51..c3211e3078456 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotificationEntry.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotificationEntry.java @@ -140,6 +140,12 @@ public final class NotificationEntry { */ private boolean hasSentReply; + /** + * Whether this notification has changed in visual appearance since the previous post. + * New notifications are interruptive by default. + */ + public boolean isVisuallyInterruptive; + /** * Whether this notification is shown to the user as a high priority notification: visible on * the lock screen/status bar and in the top section in the shade. @@ -205,6 +211,7 @@ public final class NotificationEntry { + " doesn't match existing key " + key); } mRanking = ranking; + isVisuallyInterruptive = ranking.visuallyInterruptive(); } public NotificationChannel getChannel() { @@ -244,6 +251,7 @@ public final class NotificationEntry { return mRanking.canBubble(); } + public @NonNull List getSmartActions() { return mRanking.getSmartActions(); } diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/RankingBuilder.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/RankingBuilder.java index 05f179ed4620a..820f4652e685b 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/RankingBuilder.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/RankingBuilder.java @@ -51,6 +51,7 @@ public class RankingBuilder { private ArrayList mSmartActions = new ArrayList<>(); private ArrayList mSmartReplies = new ArrayList<>(); private boolean mCanBubble = false; + private boolean mIsVisuallyInterruptive = false; public RankingBuilder() { } @@ -98,7 +99,8 @@ public class RankingBuilder { mNoisy, mSmartActions, mSmartReplies, - mCanBubble); + mCanBubble, + mIsVisuallyInterruptive); return ranking; } diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/NotificationEntryManagerTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/NotificationEntryManagerTest.java index 53d6bffdc896e..30e02e6b46d20 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/NotificationEntryManagerTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/NotificationEntryManagerTest.java @@ -183,7 +183,7 @@ public class NotificationEntryManagerTest extends SysuiTestCase { 0, NotificationManager.IMPORTANCE_DEFAULT, null, null, - null, null, null, true, sentiment, false, -1, false, null, null, false); + null, null, null, true, sentiment, false, -1, false, null, null, false, false); return true; }).when(mRankingMap).getRanking(eq(key), any(NotificationListenerService.Ranking.class)); } @@ -202,7 +202,7 @@ public class NotificationEntryManagerTest extends SysuiTestCase { null, null, null, null, null, true, NotificationListenerService.Ranking.USER_SENTIMENT_NEUTRAL, false, -1, - false, smartActions, null, false); + false, smartActions, null, false, false); return true; }).when(mRankingMap).getRanking(eq(key), any(NotificationListenerService.Ranking.class)); } diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/NotificationDataTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/NotificationDataTest.java index ab7f960d6a159..657ec61dfd112 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/NotificationDataTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/NotificationDataTest.java @@ -653,6 +653,7 @@ public class NotificationDataTest extends SysuiTestCase { public static final String OVERRIDE_SMART_ACTIONS = "sa"; public static final String OVERRIDE_SMART_REPLIES = "sr"; public static final String OVERRIDE_BUBBLE = "cb"; + public static final String OVERRIDE_VISUALLY_INTERRUPTIVE = "vi"; public Map rankingOverrides = new HashMap<>(); @@ -713,7 +714,9 @@ public class NotificationDataTest extends SysuiTestCase { overrides.containsKey(OVERRIDE_SMART_REPLIES) ? overrides.getCharSequenceArrayList(OVERRIDE_SMART_REPLIES) : currentReplies, - overrides.getBoolean(OVERRIDE_BUBBLE, outRanking.canBubble())); + overrides.getBoolean(OVERRIDE_BUBBLE, outRanking.canBubble()), + overrides.getBoolean(OVERRIDE_VISUALLY_INTERRUPTIVE, + outRanking.visuallyInterruptive())); } else { outRanking.populate( new RankingBuilder() diff --git a/services/core/java/com/android/server/notification/NotificationComparator.java b/services/core/java/com/android/server/notification/NotificationComparator.java index 9b9f4de7a18f1..bc051547a53f5 100644 --- a/services/core/java/com/android/server/notification/NotificationComparator.java +++ b/services/core/java/com/android/server/notification/NotificationComparator.java @@ -129,6 +129,12 @@ public class NotificationComparator return -1 * Integer.compare(leftPriority, rightPriority); } + final boolean leftInterruptive = left.isInterruptive(); + final boolean rightInterruptive = right.isInterruptive(); + if (leftInterruptive != rightInterruptive) { + return -1 * Boolean.compare(leftInterruptive, rightInterruptive); + } + // then break ties by time, most recent first return -1 * Long.compare(left.getRankingTimeMs(), right.getRankingTimeMs()); } diff --git a/services/core/java/com/android/server/notification/NotificationManagerService.java b/services/core/java/com/android/server/notification/NotificationManagerService.java index 2d4c6cf70847a..d480cb6e98007 100644 --- a/services/core/java/com/android/server/notification/NotificationManagerService.java +++ b/services/core/java/com/android/server/notification/NotificationManagerService.java @@ -5759,7 +5759,9 @@ public class NotificationManagerService extends SystemService { notification.flags |= old.getNotification().flags & FLAG_FOREGROUND_SERVICE; r.isUpdate = true; - r.setTextChanged(isVisuallyInterruptive(old, r)); + final boolean isInterruptive = isVisuallyInterruptive(old, r); + r.setTextChanged(isInterruptive); + r.setInterruptive(isInterruptive); } mNotificationsByKey.put(n.getKey(), r); @@ -5858,7 +5860,6 @@ public class NotificationManagerService extends SystemService { Notification oldN = old.sbn.getNotification(); Notification newN = r.sbn.getNotification(); - if (oldN.extras == null || newN.extras == null) { if (DEBUG_INTERRUPTIVENESS) { Slog.v(TAG, "INTERRUPTIVENESS: " @@ -5890,6 +5891,7 @@ public class NotificationManagerService extends SystemService { } return true; } + // Do not compare Spannables (will always return false); compare unstyled Strings final String oldText = String.valueOf(oldN.extras.get(Notification.EXTRA_TEXT)); final String newText = String.valueOf(newN.extras.get(Notification.EXTRA_TEXT)); @@ -5904,6 +5906,7 @@ public class NotificationManagerService extends SystemService { } return true; } + if (oldN.hasCompletedProgress() != newN.hasCompletedProgress()) { if (DEBUG_INTERRUPTIVENESS) { Slog.v(TAG, "INTERRUPTIVENESS: " @@ -5911,6 +5914,16 @@ public class NotificationManagerService extends SystemService { } return true; } + + // Fields below are invisible to bubbles. + if (r.canBubble()) { + if (DEBUG_INTERRUPTIVENESS) { + Slog.v(TAG, "INTERRUPTIVENESS: " + + r.getKey() + " is not interruptive: bubble"); + } + return false; + } + // Actions if (Notification.areActionsVisiblyDifferent(oldN, newN)) { if (DEBUG_INTERRUPTIVENESS) { @@ -5944,7 +5957,6 @@ public class NotificationManagerService extends SystemService { } catch (Exception e) { Slog.w(TAG, "error recovering builder", e); } - return false; } @@ -6139,12 +6151,17 @@ public class NotificationManagerService extends SystemService { Slog.v(TAG, "INTERRUPTIVENESS: " + record.getKey() + " is not interruptive: summary"); } + } else if (record.canBubble()) { + if (DEBUG_INTERRUPTIVENESS) { + Slog.v(TAG, "INTERRUPTIVENESS: " + + record.getKey() + " is not interruptive: bubble"); + } } else { + record.setInterruptive(true); if (DEBUG_INTERRUPTIVENESS) { Slog.v(TAG, "INTERRUPTIVENESS: " + record.getKey() + " is interruptive: alerted"); } - record.setInterruptive(true); } MetricsLogger.action(record.getLogMaker() .setCategory(MetricsEvent.NOTIFICATION_ALERT) @@ -6503,15 +6520,21 @@ public class NotificationManagerService extends SystemService { int indexBefore = findNotificationRecordIndexLocked(record); boolean interceptBefore = record.isIntercepted(); int visibilityBefore = record.getPackageVisibilityOverride(); + boolean interruptiveBefore = record.isInterruptive(); + recon.applyChangesLocked(record); applyZenModeLocked(record); mRankingHelper.sort(mNotificationList); - int indexAfter = findNotificationRecordIndexLocked(record); - boolean interceptAfter = record.isIntercepted(); - int visibilityAfter = record.getPackageVisibilityOverride(); - changed = indexBefore != indexAfter || interceptBefore != interceptAfter - || visibilityBefore != visibilityAfter; - if (interceptBefore && !interceptAfter + boolean indexChanged = indexBefore != findNotificationRecordIndexLocked(record); + boolean interceptChanged = interceptBefore != record.isIntercepted(); + boolean visibilityChanged = visibilityBefore != record.getPackageVisibilityOverride(); + + // Broadcast isInterruptive changes for bubbles. + boolean interruptiveChanged = + record.canBubble() && (interruptiveBefore != record.isInterruptive()); + + changed = indexChanged || interceptChanged || visibilityChanged || interruptiveChanged; + if (interceptBefore && !record.isIntercepted() && record.isNewEnoughForAlerting(System.currentTimeMillis())) { buzzBeepBlinkLocked(record); } @@ -7661,7 +7684,8 @@ public class NotificationManagerService extends SystemService { record.getSound() != null || record.getVibration() != null, record.getSystemGeneratedSmartActions(), record.getSmartReplies(), - record.canBubble() + record.canBubble(), + record.isInterruptive() ); rankings.add(ranking); } diff --git a/services/tests/uiservicestests/src/com/android/server/notification/NotificationComparatorTest.java b/services/tests/uiservicestests/src/com/android/server/notification/NotificationComparatorTest.java index e15af3dbecc46..0b4760d89686f 100644 --- a/services/tests/uiservicestests/src/com/android/server/notification/NotificationComparatorTest.java +++ b/services/tests/uiservicestests/src/com/android/server/notification/NotificationComparatorTest.java @@ -68,6 +68,7 @@ public class NotificationComparatorTest extends UiServiceTestCase { private final int uid2 = 1111111; private static final String TEST_CHANNEL_ID = "test_channel_id"; + private NotificationRecord mRecordMinCallNonInterruptive; private NotificationRecord mRecordMinCall; private NotificationRecord mRecordHighCall; private NotificationRecord mRecordDefaultMedia; @@ -105,6 +106,18 @@ public class NotificationComparatorTest extends UiServiceTestCase { smsPkg = Settings.Secure.getString(mContext.getContentResolver(), Settings.Secure.SMS_DEFAULT_APPLICATION); + Notification nonInterruptiveNotif = new Notification.Builder(mContext, TEST_CHANNEL_ID) + .setCategory(Notification.CATEGORY_CALL) + .setFlag(Notification.FLAG_FOREGROUND_SERVICE, true) + .build(); + mRecordMinCallNonInterruptive = new NotificationRecord(mContext, + new StatusBarNotification(callPkg, + callPkg, 1, "mRecordMinCallNonInterruptive", callUid, callUid, + nonInterruptiveNotif, + new UserHandle(userId), "", 2000), getDefaultChannel()); + mRecordMinCallNonInterruptive.setSystemImportance(NotificationManager.IMPORTANCE_MIN); + mRecordMinCallNonInterruptive.setInterruptive(false); + Notification n1 = new Notification.Builder(mContext, TEST_CHANNEL_ID) .setCategory(Notification.CATEGORY_CALL) .setFlag(Notification.FLAG_FOREGROUND_SERVICE, true) @@ -113,6 +126,7 @@ public class NotificationComparatorTest extends UiServiceTestCase { callPkg, 1, "minCall", callUid, callUid, n1, new UserHandle(userId), "", 2000), getDefaultChannel()); mRecordMinCall.setSystemImportance(NotificationManager.IMPORTANCE_MIN); + mRecordMinCall.setInterruptive(true); Notification n2 = new Notification.Builder(mContext, TEST_CHANNEL_ID) .setCategory(Notification.CATEGORY_CALL) @@ -245,6 +259,7 @@ public class NotificationComparatorTest extends UiServiceTestCase { expected.add(mRecordCheater); expected.add(mRecordCheaterColorized); expected.add(mRecordMinCall); + expected.add(mRecordMinCallNonInterruptive); List actual = new ArrayList<>(); actual.addAll(expected); diff --git a/services/tests/uiservicestests/src/com/android/server/notification/NotificationListenerServiceTest.java b/services/tests/uiservicestests/src/com/android/server/notification/NotificationListenerServiceTest.java index 397d2155beeb3..a9fe1a62b5587 100644 --- a/services/tests/uiservicestests/src/com/android/server/notification/NotificationListenerServiceTest.java +++ b/services/tests/uiservicestests/src/com/android/server/notification/NotificationListenerServiceTest.java @@ -51,6 +51,8 @@ import android.service.notification.NotificationRankingUpdate; import android.service.notification.SnoozeCriterion; import android.test.suitebuilder.annotation.SmallTest; +import androidx.test.runner.AndroidJUnit4; + import com.android.server.UiServiceTestCase; import org.junit.After; @@ -61,8 +63,6 @@ import org.junit.runner.RunWith; import java.util.ArrayList; import java.util.List; -import androidx.test.runner.AndroidJUnit4; - @SmallTest @RunWith(AndroidJUnit4.class) public class NotificationListenerServiceTest extends UiServiceTestCase { @@ -116,6 +116,7 @@ public class NotificationListenerServiceTest extends UiServiceTestCase { assertActionsEqual(getSmartActions(key, i), ranking.getSmartActions()); assertEquals(getSmartReplies(key, i), ranking.getSmartReplies()); assertEquals(canBubble(i), ranking.canBubble()); + assertEquals(visuallyInterruptive(i), ranking.visuallyInterruptive()); } } @@ -182,7 +183,8 @@ public class NotificationListenerServiceTest extends UiServiceTestCase { tweak.isNoisy(), (ArrayList) tweak.getSmartActions(), (ArrayList) tweak.getSmartReplies(), - tweak.canBubble() + tweak.canBubble(), + tweak.visuallyInterruptive() ); assertNotEquals(nru, nru2); } @@ -258,7 +260,8 @@ public class NotificationListenerServiceTest extends UiServiceTestCase { getNoisy(i), getSmartActions(key, i), getSmartReplies(key, i), - canBubble(i) + canBubble(i), + visuallyInterruptive(i) ); rankings[i] = ranking; } @@ -363,6 +366,10 @@ public class NotificationListenerServiceTest extends UiServiceTestCase { return index % 4 == 0; } + private boolean visuallyInterruptive(int index) { + return index % 4 == 0; + } + private void assertActionsEqual( List expecteds, List actuals) { assertEquals(expecteds.size(), actuals.size());