From 878e9322034d9de4f4b87a61b24e4ba4af908d5f Mon Sep 17 00:00:00 2001 From: Jeff DeCew Date: Thu, 13 Jan 2022 21:01:11 +0000 Subject: [PATCH 1/3] Fix the animation glitch when NSSL adds and removes a HUN in the same frame. Bug: 185680162 Fixes: 213876313 Test: visual inspection when legacy pipeline initiates a group alert transfer Merged-In: I755fff6944da0699d9cb7e6b2456e4811f7888d5 Change-Id: I755fff6944da0699d9cb7e6b2456e4811f7888d5 (cherry picked from commit 357207971089bf3c91e27b6304c18e175f215cfb) (cherry picked from commit f87c4e7bdcb5ad811df96fd999955b3041d49e39) Merged-In:I755fff6944da0699d9cb7e6b2456e4811f7888d5 --- .../stack/NotificationStackScrollLayout.java | 25 ++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) 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 73a48c3b5cb06..c14e2f96320cd 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 @@ -130,6 +130,7 @@ public class NotificationStackScrollLayout extends ViewGroup implements Dumpable public static final float BACKGROUND_ALPHA_DIMMED = 0.7f; private static final String TAG = "StackScroller"; + private static final boolean SPEW = Log.isLoggable(TAG, Log.VERBOSE); // Usage: // adb shell setprop persist.debug.nssl true && adb reboot @@ -3156,6 +3157,13 @@ public class NotificationStackScrollLayout extends ViewGroup implements Dumpable AnimationEvent event = new AnimationEvent(row, type); event.headsUpFromBottom = onBottom; mAnimationEvents.add(event); + if (SPEW) { + Log.v(TAG, "Generating HUN animation event: " + + " isHeadsUp=" + isHeadsUp + + " type=" + type + + " onBottom=" + onBottom + + " row=" + row.getEntry().getKey()); + } } mHeadsUpChangeAnimations.clear(); mAddedHeadsUpChildren.clear(); @@ -4679,7 +4687,22 @@ public class NotificationStackScrollLayout extends ViewGroup implements Dumpable @ShadeViewRefactor(RefactorComponent.STATE_RESOLVER) public void generateHeadsUpAnimation(ExpandableNotificationRow row, boolean isHeadsUp) { - if (mAnimationsEnabled && (isHeadsUp || mHeadsUpGoingAwayAnimationsAllowed)) { + final boolean add = mAnimationsEnabled && (isHeadsUp || mHeadsUpGoingAwayAnimationsAllowed); + if (SPEW) { + Log.v(TAG, "generateHeadsUpAnimation:" + + " willAdd=" + add + + " isHeadsUp=" + isHeadsUp + + " row=" + row.getEntry().getKey()); + } + if (add) { + // If we're hiding a HUN we just started showing THIS FRAME, then remove that event, + // and do not add the disappear event either. + if (!isHeadsUp && mHeadsUpChangeAnimations.remove(new Pair<>(row, true))) { + if (SPEW) { + Log.v(TAG, "generateHeadsUpAnimation: previous hun appear animation cancelled"); + } + return; + } mHeadsUpChangeAnimations.add(new Pair<>(row, isHeadsUp)); mNeedsAnimation = true; if (!mIsExpanded && !mWillExpand && !isHeadsUp) { From b479d6487827d5ee39f58d76b1b5727b57ea02d1 Mon Sep 17 00:00:00 2001 From: Jeff DeCew Date: Fri, 14 Jan 2022 01:52:13 +0000 Subject: [PATCH 2/3] Allow alertOverride if the summary GROUP_ALERT_ALL Because we still require all children to have GROUP_ALERT_PARENT, the meaning of GROUP_ALERT_ALL is equivalent to GROUP_ALERT_PARENT in any case where we would have an alertOverride, so we now allow either of those values. Also of note, because the alertOverride absolutely needs to have GROUP_ALERT_PARENT, and the semantics of heterogeneous child alert behaviors is unclear, we're keeping the requirement that all children have GROUP_APERT_PARENT to avoid over-applying this logic. Bug: 185680162 Test: atest NotificationGroupManagerLegacyTest Merged-In: I63a51cc1d94fc8a8617ca76a0fa479e3829816c6 Change-Id: I63a51cc1d94fc8a8617ca76a0fa479e3829816c6 (cherry picked from commit 6ee64f6f263f183b27f7b24d30d74bce1ceef660) (cherry picked from commit 99b1011efdeb73dc15ded2e6b1cd569b4ed07373) Merged-In:I63a51cc1d94fc8a8617ca76a0fa479e3829816c6 --- .../NotificationGroupManagerLegacy.java | 4 +- .../NotificationGroupManagerLegacyTest.java | 66 +++++++++++++++++-- 2 files changed, 63 insertions(+), 7 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/legacy/NotificationGroupManagerLegacy.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/legacy/NotificationGroupManagerLegacy.java index 5993f1dee3a7d..14b9795fe67f6 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/legacy/NotificationGroupManagerLegacy.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/legacy/NotificationGroupManagerLegacy.java @@ -384,9 +384,9 @@ public class NotificationGroupManagerLegacy implements // * Only necessary when all notifications in the group use GROUP_ALERT_SUMMARY // * Only necessary when at least one notification in the group is on a priority channel if (group.summary.getSbn().getNotification().getGroupAlertBehavior() - != Notification.GROUP_ALERT_SUMMARY) { + == Notification.GROUP_ALERT_CHILDREN) { if (SPEW) { - Log.d(TAG, "getPriorityConversationAlertOverride: summary != GROUP_ALERT_SUMMARY"); + Log.d(TAG, "getPriorityConversationAlertOverride: summary == GROUP_ALERT_CHILDREN"); } return null; } diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/NotificationGroupManagerLegacyTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/NotificationGroupManagerLegacyTest.java index 1be27da27d258..6d170b673cc3d 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/NotificationGroupManagerLegacyTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/NotificationGroupManagerLegacyTest.java @@ -177,21 +177,69 @@ public class NotificationGroupManagerLegacyTest extends SysuiTestCase { helpTestAlertOverrideWithSiblings(2); } + /** + * Helper for testing various sibling counts + */ + private void helpTestAlertOverrideWithSiblings(int numSiblings) { + helpTestAlertOverride( + /* numSiblings */ numSiblings, + /* summaryAlert */ Notification.GROUP_ALERT_SUMMARY, + /* childAlert */ Notification.GROUP_ALERT_SUMMARY, + /* siblingAlert */ Notification.GROUP_ALERT_SUMMARY, + /* expectAlertOverride */ true); + } + + @Test + public void testAlertOverrideWithParentAlertAll() { + // tests that summary can have GROUP_ALERT_ALL and this still works + helpTestAlertOverride( + /* numSiblings */ 1, + /* summaryAlert */ Notification.GROUP_ALERT_ALL, + /* childAlert */ Notification.GROUP_ALERT_SUMMARY, + /* siblingAlert */ Notification.GROUP_ALERT_SUMMARY, + /* expectAlertOverride */ true); + } + + @Test + public void testAlertOverrideWithParentAlertChild() { + // Tests that if the summary alerts CHILDREN, there's no alertOverride + helpTestAlertOverride( + /* numSiblings */ 1, + /* summaryAlert */ Notification.GROUP_ALERT_CHILDREN, + /* childAlert */ Notification.GROUP_ALERT_SUMMARY, + /* siblingAlert */ Notification.GROUP_ALERT_SUMMARY, + /* expectAlertOverride */ false); + } + + @Test + public void testAlertOverrideWithChildrenAlertAll() { + // Tests that if the children alert ALL, there's no alertOverride + helpTestAlertOverride( + /* numSiblings */ 1, + /* summaryAlert */ Notification.GROUP_ALERT_SUMMARY, + /* childAlert */ Notification.GROUP_ALERT_ALL, + /* siblingAlert */ Notification.GROUP_ALERT_ALL, + /* expectAlertOverride */ false); + } + /** * This tests, for a group with a priority entry and the given number of siblings, that: * 1) the priority entry is identified as the alertOverride for the group * 2) the onAlertOverrideChanged method is called at that time * 3) when the priority entry is removed, these are reversed */ - private void helpTestAlertOverrideWithSiblings(int numSiblings) { - int groupAlert = Notification.GROUP_ALERT_SUMMARY; + private void helpTestAlertOverride(int numSiblings, + @Notification.GroupAlertBehavior int summaryAlert, + @Notification.GroupAlertBehavior int childAlert, + @Notification.GroupAlertBehavior int siblingAlert, + boolean expectAlertOverride) { // Create entries in an order so that the priority entry can be deemed the newest child. NotificationEntry[] siblings = new NotificationEntry[numSiblings]; for (int i = 0; i < numSiblings; i++) { - siblings[i] = mGroupTestHelper.createChildNotification(groupAlert); + siblings[i] = mGroupTestHelper.createChildNotification(siblingAlert); } - NotificationEntry priorityEntry = mGroupTestHelper.createChildNotification(groupAlert); - NotificationEntry summaryEntry = mGroupTestHelper.createSummaryNotification(groupAlert); + NotificationEntry priorityEntry = mGroupTestHelper.createChildNotification(childAlert); + NotificationEntry summaryEntry = mGroupTestHelper.createSummaryNotification(summaryAlert); // The priority entry is an important conversation. when(mPeopleNotificationIdentifier.getPeopleNotificationType(eq(priorityEntry))) @@ -208,6 +256,14 @@ public class NotificationGroupManagerLegacyTest extends SysuiTestCase { } mGroupManager.onEntryAdded(priorityEntry); + if (!expectAlertOverride) { + // Test expectation is that there will NOT be an alert, so verify that! + NotificationGroup summaryGroup = + mGroupManager.getGroupForSummary(summaryEntry.getSbn()); + assertNull(summaryGroup.alertOverride); + return; + } + // Verify that the summary group has the priority child as its alertOverride NotificationGroup summaryGroup = mGroupManager.getGroupForSummary(summaryEntry.getSbn()); assertEquals(priorityEntry, summaryGroup.alertOverride); From 38d57ffe4aae7393c830cccf1b01c55880cf3700 Mon Sep 17 00:00:00 2001 From: Jeff DeCew Date: Fri, 14 Jan 2022 15:08:22 +0000 Subject: [PATCH 3/3] Fix missing group alertOverride recalculation Bug: 185680162 Test: manual testing w/ telegram and whatsapp Test: atest NotificationGroupAlertTransferHelperTest Test: atest NotificationGroupManagerLegacyTest Merged-In: I4447810af5e2a17c3b6841dfa5cd31703e5f334d Change-Id: I4447810af5e2a17c3b6841dfa5cd31703e5f334d (cherry picked from commit 457bab63923c84d13ba1abe6a5857e4a1dd0bd92) (cherry picked from commit aa6a7754ea82ffe72b9f601997d2e9297d03f786) Merged-In:I4447810af5e2a17c3b6841dfa5cd31703e5f334d --- .../collection/legacy/NotificationGroupManagerLegacy.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/legacy/NotificationGroupManagerLegacy.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/legacy/NotificationGroupManagerLegacy.java index 14b9795fe67f6..c29905bc70089 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/legacy/NotificationGroupManagerLegacy.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/legacy/NotificationGroupManagerLegacy.java @@ -529,8 +529,10 @@ public class NotificationGroupManagerLegacy implements mIsolatedEntries.put(entry.getKey(), entry.getSbn()); if (groupKeysChanged) { updateSuppression(mGroupMap.get(oldGroupKey)); - updateSuppression(mGroupMap.get(newGroupKey)); } + // Always update the suppression of the group from which you're isolated, in case + // this entry was or now is the alertOverride for that group. + updateSuppression(mGroupMap.get(newGroupKey)); } else if (!wasGroupChild && isGroupChild) { onEntryBecomingChild(entry); }