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

[android] android 7.0 notification grouping is not working #259

Closed
ZahiC opened this issue Jan 5, 2017 · 12 comments
Closed

[android] android 7.0 notification grouping is not working #259

ZahiC opened this issue Jan 5, 2017 · 12 comments

Comments

@ZahiC
Copy link

ZahiC commented Jan 5, 2017

RN 0.39
Android 7.0

In the docs, it seems that grouping is supported. I tried to trigger notifications with the same group, but I get them separated. I looked at the code and it seems that it is missing a 'summaryNotification'. As I understand, setGroup is not enough for displaying grouped notifications.

I didn't find an issue on this. Anyone uses the group functionality successfully with this module?

Some links to support this:
Official docs - "To learn how to add notifications to a group, see Add Each Notification to a Group."

Stackoverflow post

"It is important to also create a summary notification. This summary notification, denoted by setGroupSummary(true), is the only notification that appears on Marshmallow and lower devices and should (you guessed it) summarize all of the individual notifications"

@evollu
Copy link
Owner

evollu commented Jan 5, 2017

hmm good catch. can you propose a solution?

@holyxiaoxin
Copy link

Checked that the only was to prevent multiple notifications from piling up right now, is to use the same "id". Using setGroup alone didn't group the messages.

@mjmasn
Copy link
Contributor

mjmasn commented May 16, 2018

@holyxiaoxin the workaround we've found is to not set the group option, then the notifications do auto-group when there are more than 4 notifications (at least on the last few versions of Android). Setting the group option currently somehow disables any auto-grouping (I guess because it's expecting a manually created parent notification to put the grouped notifications under) and you end up with a huge list of notifications.

@mjmasn
Copy link
Contributor

mjmasn commented May 16, 2018

@evollu @holyxiaoxin just had a look at this, seems the solution is as simple as adding 3 lines to SendNotificationTask.java, replacing:

if (android.os.Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP){
  notification.setGroup(bundle.getString("group"));
}

with:

if (android.os.Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP){
  notification.setGroup(bundle.getString("group"));
  if (bundle.containsKey("groupSummary") && bundle.getBoolean("groupSummary")) {
    notification.setGroupSummary(true);
  }
}

Then in your own code before calling presentLocalNotification for the actual notification, create/overwrite the group summary with e.g:

FCM.presentLocalNotification({
  id: 'my_group',
  title: 'You have 5 notifications',
  body: 'Tap to read them',
  sound: 'default',
  priority: 'normal',
  wake_screen: false,
  group: 'my_group',
  groupSummary: true, // <--- the important line, used in the change above
  // channel: 'my_channel', // Uncomment if using sdk-26 branch
  lights: false,
  show_in_foreground: true,
});

Set the id to the group name so that you can 'create' the notification every time and it will just overwrite with the new data rather than creating a totally new notification.

Edit: Note that the summary title/body will only show up on phones that don't support notification grouping, where it will be the only notification that shows. For newer Android versions, the content of the summary notification is not shown, but any other notifications with that group name will be grouped.

@noambonnie
Copy link

@mjmasn Could you share the code snippet where you send both summary and specific notifications? I got it to group properly but I get sound for both notifications. Oddly, when I disable the sound for one of them (sound: "") the grouping no longer works.

My code:

// The summary notification
FCM.presentLocalNotification({
    id: "comments", 
    channel: 'default',
    title: "New Notification",
    body: "Tap to read", 
    sound: "", // <-- Removing this line the grouping works but alert sounds twice
    priority: "high",  
    group: "comments",
    large_icon: "ic_launcher",
    icon: "icon_comment",
    groupSummary: true,
    color: StyleConstants.BRAND_COLOR_DARK,
    show_in_foreground: true, 
    vibrate: 0, 
    wake_screen: false, 
    lights: false,     
});

// The specific notification
FCM.presentLocalNotification({
    channel: 'default',
    title: "Someone commented on something",
    body: notificationData.commentText, 
    priority: "high",
    large_icon: "ic_launcher",
    icon: "icon_comment",
    group: "comments",
    color: StyleConstants.BRAND_COLOR_DARK,
    click_action: "ACTION", 
    myData: notif.myData,   
    vibrate: 300,    
    wake_screen: false,   
    lights: true,      
    show_in_foreground: true
});

@noambonnie
Copy link

Ok I found the issue with the notification alerting summary and actual - The API defaults to GROUP_ALERT_ALL but can also be set to GROUP_ALERT_SUMMARY or GROUP_ALERT_CHILDREN. See more here:

https://developer.android.com/reference/android/support/v4/app/NotificationCompat.Builder#setGroupAlertBehavior(int)

Once I add notification.setGroupAlertBehavior(NotificationCompat.GROUP_ALERT_CHILDREN); after @mjmasn's setGroupSummary(true) things look better. I will explore this a bit further and if all is well I'll create a PR.

@mjmasn
Copy link
Contributor

mjmasn commented Jun 18, 2018

@noambonnie yeah that makes sense, and is why I shouldn't test with my devices on silent/no vibrate mode 😳

@noambonnie
Copy link

noambonnie commented Jun 18, 2018

@evollu @mjmasn I'm trying to create a PR for the above issue I reported but can't push my branch. Is the project open for PRs? Am I missing something?

$ git push --set-upstream origin group-alert-behavior 
ERROR: Permission to evollu/react-native-fcm.git denied to noambonnie.
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

@evollu
Copy link
Owner

evollu commented Jun 18, 2018

@noambonnie you need to fork this repo to under your account. then create a cross repo PR from your repo to this one

@noambonnie
Copy link

noambonnie commented Jun 19, 2018

Ok I created a PR. @mjmasn, maybe you can also give it a spin to see it works for you as well?

@evollu, I hope I got everything you need to accept it. Let me know if there's anything else you need from me.

@mjmasn
Copy link
Contributor

mjmasn commented Jun 20, 2018

@noambonnie I'll try to have a look later this week 👍 We're using a forked version of this repo so I'll have to copy your changes across.

BTW I'm mostly testing with a Samsung A5 (2016) running Android 7.0 and a LG Nexus 5X running Android 8.1 and only the 5X has the double-sound issue. Wonder if it's a device / android version specific thing?

@noambonnie
Copy link

@mjmasn I got the double sound issue on Oreo Emulator and on Nougat Samsung Note 5. So while it may not be consistent it definitely happens across the board. Might be more/less noticeable based on the phone's performance. Maybe with faster CPUs the sound overrides quickly enough so users don't notice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants