Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Heads-Up Displaying Previous Notification When Grouped on some devices #1578

Merged
merged 2 commits into from
May 10, 2022

Conversation

jkasten2
Copy link
Member

@jkasten2 jkasten2 commented May 2, 2022

Description

One Line Summary

Some devices would display the previous notification as a heads-up notification when they become grouped which this PR addresses.

Details

Motivation

The main motivation is to fix the bug noted above. Secondary is this changes the heads-up notification from showing the group summary to showing the most recent notification. This makes it much easier to see what is new instead of it being mixed with older notifications. Video below will show this behavior difference.

Scope

Only effects the display of grouped heads-up notifications for Android 7.0 and newer.

Background details - Android features

Google's documentation on specific Android Notification features referenced in this PR:

Details of the change

Using GROUP_ALERT_CHILDREN with setGroupAlertBehavior means the just receive notification will be used as the heads-up notification. Before GROUP_ALERT_SUMMARY would result in seeing a summary of all notifications in the group.

This also fixes a bug where the previous notification would be shown as the heads-up notification on some devices. Such as Samsung's OneUI 4 with the Brief style of notifications, as well as some other manufacturers.
Lastly the bug would sometimes happen on Android 7 and 8 AOSP since the summary and child notification are posted at the same time and these versions of Android would pick one at random. In this case it seemed to only be an issue when setting android_group and only on the 2nd notification display.

Fixes #1574

Testing

Unit testing

None, this is a visual change

Manual testing

Tested on Android Studio Emulators:

  • Android 5.0
  • Android 6.0
  • Android 7.0
  • Android 7.1
  • Android 8.1
  • Android 11
  • Android 12

Test on device:

  • Samsung S21 with Android 12 with One UI 4.1
    • With both Brief and Detailed pop-up styles.

Videos

Android 12 Emulator

Before

Android_headsup_as_summary.mp4

After

Android_headsup_set_as_child.mp4

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries
  • Outcomes
  • Sessions
  • In-App Messaging
  • REST API requests
  • Public API changes

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing
  • Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • I have included test coverage for these changes, or explained why they are not needed
  • All automated tests pass, or I explained why that is not possible
  • I have personally tested this on my device, or explained why that is not possible

Final pass

  • Code is as readable as possible.
  • I have reviewed this PR myself, ensuring it meets each checklist item.

This change is Reviewable

Using GROUP_ALERT_CHILDREN with setGroupAlertBehavior means the just
receive notification will be used as the heads-up, if the notification
channel is configured for highest display priority. Before with
GROUP_ALERT_SUMMARY, you would see a summary of all notifications in the
group which was much harder to see the most recent.

This also fixes a bug where the previous notification would be shown as
the heads-up notification on some devices. Such as Samsung's OneUI 4
with the Brief style of notifications, as well as some other
manufacturers.
Lastly the bug would sometimes happen on Android 7 and 8 AOSP since the
summary and child notification are posted at the same time and these
versions of Android would pick one at random. In this case it seemed to
only be an issue when setting android_group and only on the 2nd
notification display.
@jkasten2 jkasten2 changed the title Fix Heads-Up Displaying Previous Notification When Grouped on some devices WIP - Fix Heads-Up Displaying Previous Notification When Grouped on some devices May 3, 2022
After testing Android 6 in the Android Studio emulator it was found
that using setGroupAlertBehavior with GROUP_ALERT_CHILDREN breaks
the heads-up and sound for notifications that are grouped. Since the
GROUP_ALERT_CHILDREN flag is required for newer versions of Android
to fix a different bug with heads up notifications from showing old
notifications in the group we needed to add an Android version runtime
check to select the correct flag.

We could report this bug to Google and they might be able to fix it
in a future AndroidX release however due to the age of Android 6 and
it's limitations it is unlikely it will be fixed. So this workaround
will mostly likely need to stay in place until we drop support for
Android 6.

See the last commit for more details on why GROUP_ALERT_CHILDREN is
needed for Android 8+. This also addresses the failing test form the
last commit.
@jkasten2 jkasten2 force-pushed the fix/grouped_heads_up_showing_last branch from 72d207f to 2a11fcf Compare May 3, 2022 03:53
@jkasten2 jkasten2 changed the title WIP - Fix Heads-Up Displaying Previous Notification When Grouped on some devices Fix Heads-Up Displaying Previous Notification When Grouped on some devices May 3, 2022
@jkasten2 jkasten2 requested a review from a team May 3, 2022 03:54
@nan-li nan-li self-requested a review May 3, 2022 22:39
Copy link
Contributor

@nan-li nan-li left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@jkasten2 jkasten2 merged commit 0443c85 into main May 10, 2022
@jkasten2 jkasten2 deleted the fix/grouped_heads_up_showing_last branch May 10, 2022 21:19
This was referenced May 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Heads up notifications show the contains of a previous notification when grouped
2 participants