TobiasBuchholz / Plugin.Firebase

Wrapper around the native Android and iOS Firebase Xamarin SDKs
MIT License
211 stars 49 forks source link

[Question] CloudMessaging Notification get replaced when app is in foreground? #163

Closed Ghevi closed 10 months ago

Ghevi commented 1 year ago

Hi it's me again. In the method HandleShowLocalNotification the notification Id is hardcoded as 1337.

notificationManager.Notify(1337, builder.SetContentIntent(pendingIntent).Build());

When the app is in foreground the notification get replaced by following notifications, when in the background or closed each notification received is different. Is this behavioud desired? Is the same method called in both cases?

A related issue is that I'm not sure how to create a notification summary other than this, because unless I throw an exception, i need to return the builder and then the same line with the hardcoded Id will be invoked. And when the app is not in the foreground it seems the default builder is used instead. If there is some work required I could try to fix it.

FirebaseCloudMessagingImplementation.NotificationBuilderProvider = (notification) =>
{
       notification.Data.TryGetValue("groupId", out var groupId);
       notification.Data.TryGetValue("notificationId", out var notificationId);

       var summary = new NotificationCompat.Builder(ApplicationContext, channelId)
              .SetStyle(new NotificationCompat.BigTextStyle().SetSummaryText("This is the summary text"))
              .SetSmallIcon(Resource.Drawable.small_icon)
              .SetGroup(groupId)
              .SetGroupSummary(true)
              .SetPriority((Int32)NotificationPriority.High)
              .SetDefaults((Int32)NotificationDefaults.Sound | (Int32)NotificationDefaults.Vibrate)
              .Build();
        var notification = new NotificationCompat.Builder(ApplicationContext, channelId)
              .SetSmallIcon(Resource.Drawable.small_icon)
              .SetContentTitle(notification.Title)
              .SetContentText(notification.Body)
              .SetGroup(groupId)
              .SetPriority((Int32)NotificationPriority.High)
              .SetDefaults((Int32)NotificationDefaults.Sound | (Int32)NotificationDefaults.Vibrate)
              .SetAutoCancel(autoCancel: true)
              .Build();

        notificationManager.Notify(Int32.Parse(notificationId), notification);
        notificationManager.Notify(Int32.Parse(groupId), summary);

        throw new InvalidOperationException("Preventing default notify");
};
Ghevi commented 1 year ago

I tried to extend the default firebase service and when debugging this code is invoked. But as soon I put the app in the background or closed i dont see a new entry on the device log. I'm using the firebase console to send the notifications by the way. (I also tried with Android.Util.Log)

Screenshot 2023-05-08 111228

Ghevi commented 1 year ago

Ok apparently on the server side specifying the Notification property for the FirebaseAdmin.Messaging.Message is a mistake in this scenario, now that I only have the Data dictionary specified, it works except when the app is closed.

tranb3r commented 1 year ago
Just for information, here is how android handles messages, depending on app state and the presence of notification and/or data in the message: App state Notification only Data only Notification & Data
Foreground onMessageReceived onMessageReceived onMessageReceived
Background or Killed system tray onMessageReceived notification: system tray
data : in extras of the intent
warning: onMessageReceived not called !!!

So basically, do not send Notification and Data together if you implement only onMessageReceived.

Alos, just like @ghevi, I'm using the throw exception trick to disable the default behavior of NotificationBuilderProvider, and I would be interested to know if something better could be implemented in the plugin.

Ghevi commented 1 year ago

@tranb3r thank you for the explanation. So you are saying that even in killed state the onReceiveMethod should be called. I have a Xiaomi as you can see from the screenshot, I made sure to turn on autostart and turn off Battery saving restriction. But when i remove the app from the background screen, notifications just do not arrive, i tried running the app without debugging. Maybe i should try release mode? :/ Is there any specific code you had to implement?

tranb3r commented 1 year ago

No, I did not have any specific code to implement from what I remember. Although I'm not sure if you're talking about notification or data.

When the app is killed, a notification should be handled by the system, so you have nothing to do. Of course you should customize icon and color in the manifest, but I think this is optional. Data will be processed via onMessageReceived, so if you use the plugin you simply have to implement your logic in the NotificationReceived event handler.

Of course, if the app is killed, it will be re-created before processing Data. I remember having issues with the startup code of my app failing silently in this scenario. Just make sure you have enough logs to check that everything is happening normally.

Also, you should probably test your implementation on the emulator first, it should work even in debug mode.

Ghevi commented 1 year ago

@tranb3r as you can see in the code at the top, I would like to group client side notifications using the summary. So i want to know if it is possible to have that code called even with the app closed. It should be possible, I mean Telegram have a custom inbox layout when notifications arrives even with the app closed, right? I can't understand it from their code base but that's on me.

So if i send from server side a notification with "Notification" property, they arrive but with a default layout because onMessageReceived is not invoked. If i send only "Data", onMessageReceived is invoked only in foreground and background. No notification arrive when app is closed.

tranb3r commented 1 year ago

So if i send from server side a notification with "Notification" property, they arrive but with a default layout because onMessageReceived is not invoked.

Yes, this is the expected behavior, if the app is not in foreground, the notification is managed by the system, not the app code.

If i send only "Data", onMessageReceived is invoked only in foreground and background. No notification arrive when app is closed.

When sending Data, onMessageReceived should be invoked even when app is killed. I'm using it and it works, both on the emulator and on devices (not sure about xiaomi though). As I said before, maybe something is failing before onMessageReceived is called, when re-creating the app. So be sure you have enough logs to check that on the one hand the app is being created successfully and on the other hand onMessageReceived is called (or not). Or maybe this is just some weird thing that xiaomi is doing. So you should definitely start with the emulator.

Ghevi commented 1 year ago

@tranb3r Then in my case i only want to send Data if i want the onMessageReceived method of my app to be called in all three cases. I want to use the group summary in foreground, background and app killed.

I also tried with emulator, i've put the logs but the method is not called if the app is killed.

Screenshot 2023-05-09 150818

Ghevi commented 1 year ago

Sorry for the spam, in the FirebaseCloudMessagingImplementation there is this code, I think the Notification property must always be specified then...

private static void HandleShowLocalNotificationIfNeeded(FCMNotification fcmNotification)
        {
            if (!string.IsNullOrEmpty(fcmNotification.Title) || !string.IsNullOrEmpty(fcmNotification.Body))
            {
                HandleShowLocalNotification(fcmNotification);
            }
        }
tranb3r commented 1 year ago

Ok, now I remember all the tricky details in my implementation.

First, I'm only sending Data, since this is the only way to do everything in the onMessageReceived method.

Second, I'm actually bypassing the TryHandleShowLocalNotificationIfNeeded from this plugin, by throwing an exception in the NotificationBuilderProvider. This is a bit ugly, but I don't think there another way to disable it. Although looking at the code again, I'm not sure this is required, since it's already bypassed when sending only data. Maybe this is a recent change?

And finally, I'm showing the local notification in the NotificationReceived event handler. I'm actually using Plugin.LocalNotification for the plumbing, but this is not important, you can also do it manually.

Ghevi commented 1 year ago

@tranb3r i tried downloading the code of this package and eventually even if i changed it to display the notification with only the Data i got a warning that led me to discover that the channelId is lost when the app is killed, so im start to thinking this is simply impossible. Screenshot 2023-05-10 162645

tranb3r commented 1 year ago

I don't understand. You can do whatever you want in onMessageReceived. I think you should show your code for us to understand the issue.

Ghevi commented 1 year ago

This is the method i'm using now to send the notifications Screenshot 2023-05-10 170000

Here i harcoded the channeld so it always set (could also set from onMessageReceived? should it be persisted?) Screenshot 2023-05-10 170045

Here I changed the code to take the title and body from the Data payload Screenshot 2023-05-10 170125

All the other settings were exacly as here https://github.com/TobiasBuchholz/Plugin.Firebase/blob/development/docs/cloud_messaging.md

With the code like this it works if you only send Data without Notification from server. If u want me i can upload a repo.

tranb3r commented 1 year ago

So, basically, you're saying that it works, the only issue being the channelId lost when app is killed? I think it could be normal. When the app is killed, it's re-created when triggered by the notification, but I'm not all sure that the android activity is created, which means channelId is not set (assuming you're setting channelId in mainactivity, like it's done in the sample app). You should add a log to check this point. Then, the fix would simply consist in setting the channelId again, in whatever part of the code that is run on app creation (either MauiProgram, or onMessageReceived).

Ghevi commented 1 year ago

@tranb3r yes i've tried logging that now in the emulator, if im not making any mistake, OnCreate and OnNewIntent are not called when the app is killed, unless you tap the notification ofc. If all the static variables loses states i need to set the ChannelId, SmallIconRef and the custom notification builder in onMessageReceived again yes. But also I had to add the retrieval of the title and body from the Data dictionary because of course there is a check for this two properties to not be null or empty right before calling the method to notify the notification. FirebaseCloudMessagingImplementation and MyFirebaseMessagingService where I've changed the code, are two classes from the package so I need to build it for my self for now, I don't think it can be overriden otherwise

You don't plan to support this feature? Is it something one can contribute to?

Thank you for the patience btw.

tranb3r commented 1 year ago

Maybe you should show the modifications you've done in FirebaseCloudMessagingImplementation and MyFirebaseMessagingService. But again, I don't think you have to modify the plugin.

Indeed, you can skip the handling of the notification, and do it manually onMessageReceived. Then you don't care about the title and body properties check. It's not ideal (requires throwing an exception), but it works.

About supporting this feature, I have no idea, I'm not responsible for this plugin. And also I'm not sure I totally understand your requirement and why you'd need a change in the plugin. But feel free to propose a PR.

Ghevi commented 1 year ago

@tranb3r

This is the method i'm using now to send the notifications Screenshot 2023-05-10 170000

Here i harcoded the channeld so it always set (could also set from onMessageReceived? should it be persisted?) Screenshot 2023-05-10 170045

Here I changed the code to take the title and body from the Data payload Screenshot 2023-05-10 170125

All the other settings were exacly as here https://github.com/TobiasBuchholz/Plugin.Firebase/blob/development/docs/cloud_messaging.md

With the code like this it works if you only send Data without Notification from server. If u want me i can upload a repo.

The screenshots i posted here show the edited code (underline)

Indeed, you can skip the handling of the notification, and do it manually onMessageReceived.

The onMessageReceived lives in the class MyFirebaseMessagingService which is part of this package. Are you saying I have to register another service to override MyFirebaseMessagingService? I don't understand.

tranb3r commented 1 year ago

You don't need to override onMessageReceived. Simply add your code to the NotificationReceived event handler.

About channelId, it's public static, so just set it from your code.

The message data is already stored in FCMNotification, so you don't need to modify the ToFCMNotification, just get the properties you need when you implement your logic in NotificationReceived.

Ghevi commented 1 year ago

@tranb3r now i see what you mean, I need to attach to the NotificationReceived event that is invoked here Screenshot 2023-05-10 175738

tranb3r commented 1 year ago

Yes. public event EventHandler<FCMNotificationReceivedEventArgs> NotificationReceived;

Ghevi commented 1 year ago

@tranb3r And where would you do that if the state get lost when the app is closed and onCreate is not called again?

tranb3r commented 1 year ago

OnCreate of the MAUI app should be called again. For your android activity, I'm not 100% sure.

Ghevi commented 1 year ago

@tranb3r yes MauiProgram.CreateMauiApp() is called again! I will continue tomorrow, but It's all clear now. Thank you for the help!! 😄

TobiasBuchholz commented 10 months ago

Hey guys, I've just released the new version CloudMessaging 2.0.4 which contains the method FirebaseCloudMessagingImplementation.ShowLocalNotificationAction that you can use for showing local push notifications, so you don't need the exception throwing workaround anymore. See the readme for more information ✌️

tranb3r commented 10 months ago

@TobiasBuchholz Thank you. It's much better than the exception hack ;)