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

feat: use Firebase SDK for creating foreground notifications #1657

Conversation

tafelnl
Copy link
Contributor

@tafelnl tafelnl commented Jun 15, 2023

Upon further inspection of the Firebase SDK, I came up with a way better solution than the combination of my previous PRs to solve #1651

Basically it does what the Firebase SDK itself does internally to show background notifications. I only utilized methods marked as public!

This will:

Open issues:
Fix #1651
Fix #1368
Fix #1388

My 'old' PRs:
Close #1650
Close #1649
Close #1648
Close #1647

PRs by others:
Close #1629
Close #1478
Close #1324
Close #1423

@tafelnl tafelnl force-pushed the feat/use-firebase-sdk-for-creating-foreground-notifications branch from 50db0c8 to 30b2c19 Compare July 20, 2023 07:45
@jahead

This comment was marked as abuse.

@jahead
Copy link

jahead commented Jul 27, 2023

Hey not sure of the value, but we went ahead and applied this patch to our beta channel to see if closes out our internal bug.
It fixed

  • foreground notification not clickable
  • thumbnails not showing in foreground

So congrats! :)
However it did change the current behavior in that it now respects the importance level of the fallback channel. So the notification went from popping over (but not clickable) to not popping over (but clickable). Personally I think that it's now the correct behavior but it was an unexpected side effect - but illustrated that we forgot to send the channelId ;D.

This comment was marked as abuse.

Also, I'm not sure why my comment was flagged for abuse, spam maybe, off-topic probably, but my australian accent isn't that abusive ;) - we are just trying to plan around capacitor's release cycle.

@dallastjames dallastjames added the type: feature request A new feature, enhancement, or improvement label Jul 28, 2023
@jahead
Copy link

jahead commented Aug 4, 2023

Hey not to spam here, but it seems our testers got it wrong; we think they were triggering a background notification display without realising it.

This pr doesn't completely mimic the display notification class that the official firebase native Android SDK uses to display background notification.

One biggest change is it doesn't handle the property imageUrl
Which is a step that happens before they create the notificationInfo.

To add that feature though. It would be a bit more of a code change. So we have decided to fork the package so we customise the fire notification method.

It's unfortunate since we mostly only need to modify the one method.

If the capacitor team is interested in accepting a pr to bring the foreground in line with how the background is handled I'm sure our client would be happy to share back.

I will say though this did get us mostly to where we needed to be

@dallastjames dallastjames added the pr review requested Adds PR to internal issue tracker for team review label Aug 7, 2023
/**
* Create notification channel
*/
public void createForegroundNotificationChannel() {
Copy link
Contributor

Choose a reason for hiding this comment

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

@jcesarmobile Do we still need this, or is adding a default channel id to the manifest enough?

Copy link
Contributor Author

@tafelnl tafelnl Aug 24, 2023

Choose a reason for hiding this comment

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

As far as I am aware, it's not needed anymore. The CommonNotificationBuilder.getOrCreateChannel( will take care of upserting the channel.

The Firebase SDK will execute similar logic as what's explained in #1649:

[It] will derive the channelId to be used for the foreground in the following order:

  1. Firstly it will check if the incoming notification has a channelId set
  2. Then it will check for a possible given value in the AndroidManifest.xml1
  3. Lastly it will use the fallback channelId that the Firebase SDK provides for us. This channel will be created by the Firebase SDK upon receiving the first push message2

1 From the Firebase docs:

From Android 8.0 (API level 26) and higher, notification channels are supported and recommended. FCM provides a default notification channel with basic settings. If you prefer to create and use your own default channel, set default_notification_channel_id to the ID of your notification channel object as shown; FCM will use this value whenever incoming messages do not explicitly set a notification channel.

(see README also)

2 This is similar to how react-native-push-notification is handling this: https://github.com/zo0r/react-native-push-notification/blob/master/android/src/main/java/com/dieam/reactnativepushnotification/modules/RNPushNotificationConfig.java#LL69C19-L69C50

I think this makes way more sense than the current solution. It will also automatically inherit the channel settings set for the incoming notification. So that's more expected behaviour for the developer


It would make sense to explain all this to the user of course. I did add some stuff to the docs in my other PR (https://github.com/ionic-team/capacitor-plugins/pull/1649/files#diff-09c58e1cc87af661e7a9d0081a9360efcd3da00a53911c3f6b998a66168ea170). Honestly, I don't know why I didn't add that to this PR. Probably just forgot about it. I can add it if you want?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I would add that note about the default notification channel in case users needed to define and use their own. Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added! I tried my best to explain it as clear as possible.

@theproducer theproducer merged commit 530029c into ionic-team:main Sep 7, 2023
9 checks passed
7-plus-t pushed a commit to ergon/capacitor-plugins that referenced this pull request Oct 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr review requested Adds PR to internal issue tracker for team review type: feature request A new feature, enhancement, or improvement
Projects
None yet
7 participants