From 8c1b763dcf5df6e7fc7177dad5e7390adb229fa4 Mon Sep 17 00:00:00 2001 From: Ned Burns Date: Fri, 19 Jul 2019 14:26:15 -0400 Subject: [PATCH] Fix issue with media notifs being misbucketed Previous code assumed that "isHighPriority" == "is in top section", which is not always true. Media notifs and some other notifs can appear in the top section even if they're not high priority. Because we detect section boundaries by iterating through the list until we find the first notif where isHighPriority == false, we were sometimes drawing the section boundary way too high. This change creates a new propery, isInTopSection() that accurately tracks this state. Setting this value in the proper location would require some seriously destabilizing refactors, so instead we set it in the list comparator, which is awful but here are. Test: manual Bug: 136709564 Change-Id: I19223720bac534ab92219a2962169097819e8efb --- .../NotificationLockscreenUserManager.java | 3 +-- .../NotificationLockscreenUserManagerImpl.java | 7 +++---- .../NotificationViewHierarchyManager.java | 6 ++---- .../collection/NotificationData.java | 18 +++++++++++++----- .../collection/NotificationEntry.java | 15 +++++++++++++++ .../stack/NotificationSectionsManager.java | 4 ++-- .../stack/NotificationStackScrollLayout.java | 6 +++--- .../statusbar/phone/NotificationPanelView.java | 3 +-- .../NotificationLockscreenUserManagerTest.java | 6 +++--- .../stack/NotificationSectionsManagerTest.java | 2 ++ 10 files changed, 45 insertions(+), 25 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/NotificationLockscreenUserManager.java b/packages/SystemUI/src/com/android/systemui/statusbar/NotificationLockscreenUserManager.java index 0fe5f8a6af5ab..4cc5b2144adc4 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/NotificationLockscreenUserManager.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/NotificationLockscreenUserManager.java @@ -15,7 +15,6 @@ package com.android.systemui.statusbar; import android.content.pm.UserInfo; -import android.service.notification.StatusBarNotification; import android.util.SparseArray; import com.android.systemui.statusbar.notification.collection.NotificationEntry; @@ -58,7 +57,7 @@ public interface NotificationLockscreenUserManager { boolean shouldHideNotifications(int userId); boolean shouldHideNotifications(String key); - boolean shouldShowOnKeyguard(StatusBarNotification sbn); + boolean shouldShowOnKeyguard(NotificationEntry entry); boolean isAnyProfilePublicMode(); diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/NotificationLockscreenUserManagerImpl.java b/packages/SystemUI/src/com/android/systemui/statusbar/NotificationLockscreenUserManagerImpl.java index 4ea1ed5b94515..e08a5ae07bd89 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/NotificationLockscreenUserManagerImpl.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/NotificationLockscreenUserManagerImpl.java @@ -33,7 +33,6 @@ import android.os.ServiceManager; import android.os.UserHandle; import android.os.UserManager; import android.provider.Settings; -import android.service.notification.StatusBarNotification; import android.util.Log; import android.util.SparseArray; import android.util.SparseBooleanArray; @@ -302,7 +301,7 @@ public class NotificationLockscreenUserManagerImpl implements Notification.VISIBILITY_SECRET; } - public boolean shouldShowOnKeyguard(StatusBarNotification sbn) { + public boolean shouldShowOnKeyguard(NotificationEntry entry) { if (getEntryManager() == null) { Log.wtf(TAG, "mEntryManager was null!", new Throwable()); return false; @@ -310,10 +309,10 @@ public class NotificationLockscreenUserManagerImpl implements boolean exceedsPriorityThreshold; if (NotificationUtils.useNewInterruptionModel(mContext) && hideSilentNotificationsOnLockscreen()) { - exceedsPriorityThreshold = getEntryManager().getNotificationData().isHighPriority(sbn); + exceedsPriorityThreshold = entry.isTopBucket(); } else { exceedsPriorityThreshold = - !getEntryManager().getNotificationData().isAmbient(sbn.getKey()); + !getEntryManager().getNotificationData().isAmbient(entry.key); } return mShowLockscreenNotifications && exceedsPriorityThreshold; } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/NotificationViewHierarchyManager.java b/packages/SystemUI/src/com/android/systemui/statusbar/NotificationViewHierarchyManager.java index 22c91647d7f79..6e75c0375afc0 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/NotificationViewHierarchyManager.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/NotificationViewHierarchyManager.java @@ -397,15 +397,13 @@ public class NotificationViewHierarchyManager implements DynamicPrivacyControlle int userId = entry.notification.getUserId(); boolean suppressedSummary = mGroupManager.isSummaryOfSuppressedGroup( entry.notification) && !entry.isRowRemoved(); - boolean showOnKeyguard = mLockscreenUserManager.shouldShowOnKeyguard(entry - .notification); + boolean showOnKeyguard = mLockscreenUserManager.shouldShowOnKeyguard(entry); if (!showOnKeyguard) { // min priority notifications should show if their summary is showing if (mGroupManager.isChildInGroupWithSummary(entry.notification)) { NotificationEntry summary = mGroupManager.getLogicalGroupSummary( entry.notification); - if (summary != null && mLockscreenUserManager.shouldShowOnKeyguard( - summary.notification)) { + if (summary != null && mLockscreenUserManager.shouldShowOnKeyguard(summary)) { showOnKeyguard = true; } } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotificationData.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotificationData.java index fca520fe05219..1ce493444e25b 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotificationData.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotificationData.java @@ -25,7 +25,6 @@ import android.service.notification.NotificationListenerService.RankingMap; import android.service.notification.SnoozeCriterion; import android.service.notification.StatusBarNotification; import android.util.ArrayMap; -import android.util.Slog; import com.android.internal.annotations.VisibleForTesting; import com.android.systemui.Dependency; @@ -108,10 +107,19 @@ public class NotificationData { boolean bSystemMax = bImportance >= NotificationManager.IMPORTANCE_HIGH && isSystemNotification(nb); - boolean isHeadsUp = a.getRow().isHeadsUp(); - if (isHeadsUp != b.getRow().isHeadsUp()) { - return isHeadsUp ? -1 : 1; - } else if (isHeadsUp) { + + boolean aHeadsUp = a.getRow().isHeadsUp(); + boolean bHeadsUp = b.getRow().isHeadsUp(); + + // HACK: This should really go elsewhere, but it's currently not straightforward to + // extract the comparison code and we're guaranteed to touch every element, so this is + // the best place to set the buckets for the moment. + a.setIsTopBucket(aHeadsUp || aMedia || aSystemMax || a.isHighPriority()); + b.setIsTopBucket(bHeadsUp || bMedia || bSystemMax || b.isHighPriority()); + + if (aHeadsUp != bHeadsUp) { + return aHeadsUp ? -1 : 1; + } else if (aHeadsUp) { // Provide consistent ranking with headsUpManager return mHeadsUpManager.compare(a, b); } else if (aMedia != bMedia) { 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 b19d2ca29c96a..d718570822291 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 @@ -173,6 +173,9 @@ public final class NotificationEntry { * the lock screen/status bar and in the top section in the shade. */ private boolean mHighPriority; + + private boolean mIsTopBucket; + private boolean mSensitive = true; private Runnable mOnSensitiveChangedListener; private boolean mAutoHeadsUp; @@ -224,6 +227,18 @@ public final class NotificationEntry { this.mHighPriority = highPriority; } + /** + * @return True if the notif should appear in the "top" or "important" section of notifications + * (as opposed to the "bottom" or "silent" section). This is usually the same as + * {@link #isHighPriority()}, but there are certain exceptions, such as media notifs. + */ + public boolean isTopBucket() { + return mIsTopBucket; + } + public void setIsTopBucket(boolean isTopBucket) { + mIsTopBucket = isTopBucket; + } + public boolean isBubble() { return (notification.getNotification().flags & FLAG_BUBBLE) != 0; } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/NotificationSectionsManager.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/NotificationSectionsManager.java index 170a4d5706887..d119fb79e4c60 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/NotificationSectionsManager.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/NotificationSectionsManager.java @@ -133,7 +133,7 @@ class NotificationSectionsManager implements StackScrollAlgorithm.SectionProvide if (child instanceof ExpandableNotificationRow && child.getVisibility() != View.GONE) { ExpandableNotificationRow row = (ExpandableNotificationRow) child; - if (!row.getEntry().isHighPriority()) { + if (!row.getEntry().isTopBucket()) { firstGentleNotifIndex = i; mFirstGentleNotif = row; break; @@ -248,7 +248,7 @@ class NotificationSectionsManager implements StackScrollAlgorithm.SectionProvide View child = mParent.getChildAt(i); if (child.getVisibility() != View.GONE && child instanceof ExpandableNotificationRow) { ExpandableNotificationRow row = (ExpandableNotificationRow) child; - if (!row.getEntry().isHighPriority()) { + if (!row.getEntry().isTopBucket()) { break; } else { lastChildBeforeGap = row; diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/NotificationStackScrollLayout.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/NotificationStackScrollLayout.java index 8ebc87fbb3553..aea780d9cf360 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/NotificationStackScrollLayout.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/NotificationStackScrollLayout.java @@ -5746,7 +5746,7 @@ public class NotificationStackScrollLayout extends ViewGroup implements ScrollAd currentIndex++; boolean beforeSpeedBump; if (mHighPriorityBeforeSpeedBump) { - beforeSpeedBump = row.getEntry().isHighPriority(); + beforeSpeedBump = row.getEntry().isTopBucket(); } else { beforeSpeedBump = !row.getEntry().ambient; } @@ -5804,9 +5804,9 @@ public class NotificationStackScrollLayout extends ViewGroup implements ScrollAd case ROWS_ALL: return true; case ROWS_HIGH_PRIORITY: - return row.getEntry().isHighPriority(); + return row.getEntry().isTopBucket(); case ROWS_GENTLE: - return !row.getEntry().isHighPriority(); + return !row.getEntry().isTopBucket(); default: throw new IllegalArgumentException("Unknown selection: " + selection); } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/phone/NotificationPanelView.java b/packages/SystemUI/src/com/android/systemui/statusbar/phone/NotificationPanelView.java index 37855ae592a50..f17496bbc7c3f 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/phone/NotificationPanelView.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/phone/NotificationPanelView.java @@ -803,8 +803,7 @@ public class NotificationPanelView extends PanelView implements if (suppressedSummary) { continue; } - if (!mLockscreenUserManager.shouldShowOnKeyguard( - row.getStatusBarNotification())) { + if (!mLockscreenUserManager.shouldShowOnKeyguard(row.getEntry())) { continue; } if (row.isRemoved()) { diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/NotificationLockscreenUserManagerTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/NotificationLockscreenUserManagerTest.java index 49a263a8d781f..57dd8c94c7900 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/NotificationLockscreenUserManagerTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/NotificationLockscreenUserManagerTest.java @@ -38,7 +38,6 @@ import android.os.Handler; import android.os.Looper; import android.os.UserManager; import android.provider.Settings; -import android.service.notification.StatusBarNotification; import android.testing.AndroidTestingRunner; import android.testing.TestableLooper; @@ -48,6 +47,7 @@ import com.android.systemui.Dependency; import com.android.systemui.SysuiTestCase; import com.android.systemui.statusbar.notification.NotificationEntryManager; import com.android.systemui.statusbar.notification.collection.NotificationData; +import com.android.systemui.statusbar.notification.collection.NotificationEntry; import com.android.systemui.statusbar.phone.StatusBarKeyguardViewManager; import com.android.systemui.statusbar.policy.DeviceProvisionedController; @@ -166,7 +166,7 @@ public class NotificationLockscreenUserManagerTest extends SysuiTestCase { Settings.Secure.LOCK_SCREEN_SHOW_SILENT_NOTIFICATIONS, 1); when(mNotificationData.isHighPriority(any())).thenReturn(false); - assertTrue(mLockscreenUserManager.shouldShowOnKeyguard(mock(StatusBarNotification.class))); + assertTrue(mLockscreenUserManager.shouldShowOnKeyguard(mock(NotificationEntry.class))); } @Test @@ -179,7 +179,7 @@ public class NotificationLockscreenUserManagerTest extends SysuiTestCase { Settings.Secure.LOCK_SCREEN_SHOW_SILENT_NOTIFICATIONS, 0); when(mNotificationData.isHighPriority(any())).thenReturn(false); - assertFalse(mLockscreenUserManager.shouldShowOnKeyguard(mock(StatusBarNotification.class))); + assertFalse(mLockscreenUserManager.shouldShowOnKeyguard(mock(NotificationEntry.class))); } private class TestNotificationLockscreenUserManager diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/stack/NotificationSectionsManagerTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/stack/NotificationSectionsManagerTest.java index 73abda9a5da7a..59d0f912d38ef 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/stack/NotificationSectionsManagerTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/stack/NotificationSectionsManagerTest.java @@ -263,6 +263,8 @@ public class NotificationSectionsManagerTest extends SysuiTestCase { when(notifRow.getVisibility()).thenReturn(View.VISIBLE); when(notifRow.getEntry().isHighPriority()) .thenReturn(children[i] == ChildType.HIPRI); + when(notifRow.getEntry().isTopBucket()) + .thenReturn(children[i] == ChildType.HIPRI); when(notifRow.getParent()).thenReturn(mNssl); child = notifRow; break;