[firebase_messaging] Add support for handling messages in background #1900

kroikie · 2019-07-24T20:55:12Z

Description

When Flutter app is in the background or terminated allow it to handle incoming FCM messages. This update uses a similar strategy to the one used by the android_alarm_manager plugin. When the application starts an additional background channel is started to handle incoming messages when the app is not in the foreground.

Related Issues

flutter/flutter#32372

Related PR

#1898

Breaking Change

This is not a breaking change, but a version bump and change log entry will be needed.

collinjackson · 2019-07-24T21:55:36Z

Thanks. @ivk1800 I'd love to get your thoughts on how this PR compares to #1898 since it seems like they're both tackling the same problem.

The-Redhat · 2019-07-25T08:15:23Z

Wouldn't it be better to have per default the same behaviour like on iOS ? The data message get's stored an onMessage is called at app start. We can then offer the possibility to override this behaviour and handle the message in background. Same thoughts for #1898

kroikie · 2019-07-25T13:35:19Z

@The-Redhat Background message handling here is optional here. Storing the messages till the app is launched would be a departure from how the native SDKs generally operate. The desired functionality is for messages (non user visible) to be handled when they are received, otherwise the app could just perform required operations at next launch with no need for messaging.

Here a background isolate is started (if enabled by the developer) to handle background messages as they arrive. I'd be happy to do this on iOS as well once I figure out background isolates for iOS.

kroikie · 2019-07-30T16:24:22Z

@ivk1800 If you can please take a look to ensure that this PR covers the use case you solved in #1898

...ging/android/src/main/java/io/flutter/plugins/firebasemessaging/FirebaseMessagingPlugin.java

collinjackson · 2019-07-31T22:29:46Z

packages/firebase_messaging/lib/firebase_messaging.dart

+      _channel.invokeMethod<bool>(
+        'FcmDartService.start',
+        <dynamic>[
+          backgroundSetupHandle.toRawHandle(),


nit: I think it's more readable/consistent to use named rather than positional arguments for MethodChannel, unless there's only one argument.

Used a <String, dynamic> map so parameters can be named.

packages/firebase_messaging/README.md

collinjackson · 2019-08-04T20:10:40Z

...ging/android/src/main/java/io/flutter/plugins/firebasemessaging/FirebaseMessagingPlugin.java

@@ -99,7 +110,28 @@ public void onReceive(Context context, Intent intent) {

  @Override
  public void onMethodCall(final MethodCall call, final Result result) {
-    if ("configure".equals(call.method)) {
+    if ("FcmDartService.start".equals(call.method)) {


For consistency with other plugins that use method names to represent both a class and a method (e.g. storage), consider using # here.

Also if the class you're calling is FlutterFirebaseMessagingService would it make sense to put that here as well?

Used # for consistency.

FlutterFIrebaseMessagingService starts without the call from the Dart side. I'm happy to change the name but it should be something that indicates the setting up of the background channel.

...roid/src/main/java/io/flutter/plugins/firebasemessaging/FlutterFirebaseMessagingService.java

collinjackson · 2019-08-04T20:39:19Z

...ging/android/src/main/java/io/flutter/plugins/firebasemessaging/FirebaseMessagingPlugin.java

+        new MethodChannel(
+            registrar.messenger(),
+            "plugins.flutter.io/android_fcm_background",
+            JSONMethodCodec.INSTANCE);


Just curious, why do we use JSONMethodCodec here instead of the default?

I think this is just a artifact from the alarm manager plugin I don't think it is necessary, the default codec should be sufficient.

collinjackson · 2019-08-04T20:43:31Z

packages/firebase_messaging/lib/firebase_messaging.dart

 import 'package:meta/meta.dart';
 import 'package:platform/platform.dart';

 typedef Future<dynamic> MessageHandler(Map<String, dynamic> message);

+const String _backgroundName = 'plugins.flutter.io/android_fcm_background';


Not sure it makes sense to put this here as it's only used in one place, but should probably have channel in the method name

collinjackson · 2019-08-04T20:45:16Z

packages/firebase_messaging/lib/firebase_messaging.dart

 import 'package:meta/meta.dart';
 import 'package:platform/platform.dart';

 typedef Future<dynamic> MessageHandler(Map<String, dynamic> message);

+const String _backgroundName = 'plugins.flutter.io/android_fcm_background';
+
+void _fcmSetupCallback() {


You might want to put this code in the one place it's used, or in a static method of the class, it's unusual to have it in a function at the global scope.

Added as a static member of FirebaseMessaging

collinjackson · 2019-08-04T20:46:53Z

packages/firebase_messaging/lib/firebase_messaging.dart

+const String _backgroundName = 'plugins.flutter.io/android_fcm_background';
+
+void _fcmSetupCallback() {
+  const MethodChannel _channel =


An underscore for hiding is confusing here since the variable is already scoped to the current function. I would either put this on the main class (so you can make it @visibleForTesting) or just call it channel here.

This variable is no longer defined in this scope.

collinjackson · 2019-08-04T20:48:07Z

...roid/src/main/java/io/flutter/plugins/firebasemessaging/FlutterFirebaseMessagingService.java

+    sBackgroundChannel.invokeMethod("", args, result);
+  }
+
+  // TODO(kroikie): Find a better way to determine application state.


@matthew-carroll might have ideas for other ways to do this

@kroikie can you clarify what you mean technically by "foreground" in this case?

@matthew-carroll the goal here is to determine when the Flutter process is still alive, in which case the original isolate can be used to process incoming messages. However when the Flutter process is no longer running the "background" isolate is used to process incoming messages.

Here we are checking if the app is in the foreground and visible, which is not exactly what the goal is but I could not find a clean way to determine if the Flutter process is still alive, this approximation was the closest I could get.

There should only be 1 process, and that's the app's process. If this code is executing then that process exists, by definition...

@matthew-carroll I'm not sure if this is actionable feedback for Arthur, are you ok with landing this as is?

matthew-carroll · 2019-08-05T20:11:18Z

packages/firebase_messaging/README.md

+```
+<application android:name=".Application" ...>
+```
+1. Define a top level method to handle background messages


Can you explicitly mention that this is a Dart method so that readers do not get confused?

matthew-carroll · 2019-08-05T20:12:07Z

packages/firebase_messaging/README.md

+1. Define a top level method to handle background messages
+```
+Future<dynamic> myBackgroundMessageHandler(Map<String, dynamic> message) {
+  if (message.containsKey('data')) {


Can you mention somewhere where a developer can find the protocol that is being used here? The reference to 'data' and 'notification' looks magical.

Done, it is in line with RemoteMessage fields.

matthew-carroll · 2019-08-05T20:12:53Z

packages/firebase_messaging/README.md

+    dynamic data = message['data'];
+  }
+
+  if (message.containsKey('notification')) {


Can a message be both a "data" message and a "notification" message? If not, please consider using an if-else-if statement to make that explicit.

RemoteMessages can contain both a data and notification part.

How the Android SDK passes the message over to the developer (and Plugin) depends on the state of the app.

matthew-carroll · 2019-08-05T20:15:33Z

packages/firebase_messaging/README.md

+  }
+
+  // Or do work with other plugins, eg: write to RTDB.
+  FirebaseDatabase.instance.reference().child('foo').set('bar');


This reference to another plugin might be confusing for the reader - it seems to come out of nowhere. Also, are you sure this works as expected? You're executing in a background isolate, which will have a different version of the FirebaseDatabase plugin than the main isolate. This line might suggest to readers that there is only 1 FirebaseDatabase plugin and that they're controlling it from this background isolate.

This example is to suggest that the developer is able to perform work in the background. In this case writing some data to RTDB. I assume the dev could use the shared preferences plugin to store some data locally.

I have tried this and it does work as expected from my tests, is there some reason why getting an instance of RTDB (which is a singleton) in a different isolate would be an issue?

If the implementation of a plugin ensures that it shares resources across all plugin instances then things are fine. My concern here is that we're suggesting that you can interact with any plugin from here, but any plugin that does not setup its own singleton behavior would not work as expected...

packages/firebase_messaging/README.md

...ging/android/src/main/java/io/flutter/plugins/firebasemessaging/FirebaseMessagingPlugin.java

...roid/src/main/java/io/flutter/plugins/firebasemessaging/FlutterFirebaseMessagingService.java

matthew-carroll · 2019-08-05T20:26:18Z

...roid/src/main/java/io/flutter/plugins/firebasemessaging/FlutterFirebaseMessagingService.java

+
+  public static void startBackgroundIsolate(Context context, long callbackHandle) {
+    FlutterMain.ensureInitializationComplete(context, null);
+    String mAppBundlePath = FlutterMain.findAppBundlePath(context);


In the embedding I've migrated us away from hungarian notation. @collinjackson do you think we could migrate plugins away from it, too?

...roid/src/main/java/io/flutter/plugins/firebasemessaging/FlutterFirebaseMessagingService.java

matthew-carroll · 2019-08-05T20:27:40Z

...roid/src/main/java/io/flutter/plugins/firebasemessaging/FlutterFirebaseMessagingService.java

+    sBackgroundChannel.invokeMethod("", args, result);
+  }
+
+  // TODO(kroikie): Find a better way to determine application state.


@kroikie can you clarify what you mean technically by "foreground" in this case?

matthew-carroll · 2019-08-05T20:29:41Z

CC @xster - This PR replicates a lot of the functionality from android_alarm_manager. I think you and I knew this was eventually coming after our conversation with @kroikie, but wanted to make sure its on your radar. We've got a very subpar solution for background execution in android_alarm_manager and we're seeing that expand here. We may want to prioritize a first-class background execution solution in the add-to-app timeline.

XExistence · 2019-08-09T09:33:24Z

@kroikie I have implemented the branch and maybe this might help developers who don't notice at first hand which I didn't. When creating the Application.java class. The package name needs to be specific to the application path of the file. If not the app breaks on launch.

package "your specific package name of your app"

I hope that helps.

...roid/src/main/java/io/flutter/plugins/firebasemessaging/FlutterFirebaseMessagingService.java

…lugins/firebasemessaging/FirebaseMessagingPlugin.java Co-Authored-By: Collin Jackson <jackson@google.com>

xster · 2019-08-13T21:57:21Z

Responding late. We did talk about this and FWIW, doing a thing for now rather than building out some sort of dependency injection mechanism to keep the same engine instance is ok and pragmatic for now. Though it's "unsupported" and we can't guarantee what's going to happen when this creates 2 engine instances.

Data messages when terminated will be supported soon. Check flutter/plugins#1900

…background

...roid/src/main/java/io/flutter/plugins/firebasemessaging/FlutterFirebaseMessagingService.java

...ging/android/src/main/java/io/flutter/plugins/firebasemessaging/FirebaseMessagingPlugin.java

collinjackson

I'm happy with landing this assuming @matthew-carroll is on board with the approach (for now). Just a few minor nits.

collinjackson · 2019-08-23T20:46:43Z

...roid/src/main/java/io/flutter/plugins/firebasemessaging/FlutterFirebaseMessagingService.java

+    sBackgroundChannel.invokeMethod("", args, result);
+  }
+
+  // TODO(kroikie): Find a better way to determine application state.


@matthew-carroll I'm not sure if this is actionable feedback for Arthur, are you ok with landing this as is?

collinjackson · 2019-08-23T20:48:45Z

packages/firebase_messaging/lib/firebase_messaging.dart

+/// Your app should never call this method directly, this is only for use
+/// by the firebase_messaging plugin to setup background message handling.
+@visibleForTesting
+Future<void> fcmSetupBackgroundChannel(


nit: I would lean towards making this a static void on FirebaseMessaging and removing fcm for stylistic consistency with other plugins. We don't usually namespace global functions with prefixes like this.

collinjackson · 2019-08-23T20:53:22Z

...roid/src/main/java/io/flutter/plugins/firebasemessaging/FlutterFirebaseMessagingService.java

+      "background_message_callback";
+
+  // TODO(kroikie): make sIsIsolateRunning per-instance, not static.
+  private static AtomicBoolean sIsIsolateRunning = new AtomicBoolean(false);


Per @matthew-carroll's suggestion, let's migrate away from Hungarian notation in first-party plugins. I think his feedback from earlier in this PR is being hidden due to changes but here's a direct link.

collinjackson · 2019-08-26T16:40:12Z

I've moved this over to firebase/flutterfire#38

@kroikie, I've given you push access to my fork so you can push your changes there until it's ready to merge

[firebase_messaging] Add Android support for handling messages in background

* commit 'bfc666daa89698c9fdc342e7ae521ec5e4e8ec5b': (59 commits) Fix analyze errors Make fcmSetupBackgroundChannel private Make fcmSetupBackgroundChannel top-level fix formatting fix analyze issues address comments Wrap chanel result logic in LatchResult Migration of flutter/plugins#1900 Remove emulators references from .cirrus.yaml format initial Update issue templates Update version/changelog per PR guidelines Remove duplicate example. reformat Update docs to encourage contributors to self-label PRs with the plugin they affect Manual merge of rejected diff Migrate PR Fix analyzer failures + reformat reformat ...

JillyTaboga · 2019-09-16T10:50:33Z

packages/firebase_messaging/README.md


+By default background messaging is not enabled. To handle messages in the background:
+
+1. Add an Application.java class to your app


Where in the App we should put this Java class, with the MainActivity or inside io.flutter.plugins?

@JillyTaboga You should add it alongside your MainActivity.java file.

brunoripa · 2019-09-17T17:58:25Z

So, basically, this PR makes possible to handle background messages by following the info in the README ? Adding java class etc ? Would it be possible to get pointed to a complete example, so that maybe we can contribute to a documentation ?

kroikie requested a review from collinjackson as a code owner July 24, 2019 20:55

googlebot added the cla: yes label Jul 24, 2019

kroikie added the flutterfire label Jul 24, 2019

kroikie added 2 commits July 29, 2019 14:32

Add support for handling messages in background

fb18e66

fix formatting

46a36a2

kroikie force-pushed the fcm_background branch from dec9fa2 to 46a36a2 Compare July 29, 2019 21:33

Prepare for review

dd6cbae

kroikie changed the title ~~[WIP][firebase_messaging]Add support for handling messages in background~~ [firebase_messaging]Add support for handling messages in background Jul 30, 2019

kroikie mentioned this pull request Jul 30, 2019

[firebase_messaging] Android: Receiving Data messages if app terminated #1898

Closed

12 tasks

kroikie changed the title ~~[firebase_messaging]Add support for handling messages in background~~ [firebase_messaging] Add support for handling messages in background Jul 31, 2019

collinjackson reviewed Aug 4, 2019

View reviewed changes

kroikie requested a review from matthew-carroll August 5, 2019 18:34

matthew-carroll reviewed Aug 5, 2019

View reviewed changes

matthew-carroll requested a review from mklim August 5, 2019 20:28

XExistence reviewed Aug 9, 2019

View reviewed changes

...roid/src/main/java/io/flutter/plugins/firebasemessaging/FlutterFirebaseMessagingService.java Show resolved Hide resolved

josh-burton mentioned this pull request Aug 12, 2019

[firebase_messaging] iOS notifications only working while in foreground. flutter/flutter#38336

Closed

Update packages/firebase_messaging/android/src/main/java/io/flutter/p…

80abf02

…lugins/firebasemessaging/FirebaseMessagingPlugin.java Co-Authored-By: Collin Jackson <jackson@google.com>

Levi-Lesches added a commit to Student-Lyf/RamLife that referenced this pull request Aug 14, 2019

Registered app for notifications

143d186

Data messages when terminated will be supported soon. Check flutter/plugins#1900

Changes addressing comments

3c85f60

kroikie added 4 commits August 14, 2019 16:47

fix formatting

52f0f05

Merge branch 'fcm_background' of github.com:kroikie/plugins into fcm_…

db06ee3

…background

fix analyze errors

8c6b11e

fix formatting

52a19ba

XExistence reviewed Aug 16, 2019

View reviewed changes

...roid/src/main/java/io/flutter/plugins/firebasemessaging/FlutterFirebaseMessagingService.java Outdated Show resolved Hide resolved

kroikie added 3 commits August 16, 2019 15:45

return missing method from MessagingService

300db8a

fix formatting

699bbe4

revert to using available on stable

14ceed7

josh-burton reviewed Aug 17, 2019

View reviewed changes

...ging/android/src/main/java/io/flutter/plugins/firebasemessaging/FirebaseMessagingPlugin.java Outdated Show resolved Hide resolved

kroikie added 4 commits August 21, 2019 08:36

Update background channel name

806ce88

fix formatting

121ba01

remove RTDB example from README.md

d300a84

fix typo

e7b521f

collinjackson reviewed Aug 23, 2019

View reviewed changes

collinjackson added a commit to collinjackson/flutterfire that referenced this pull request Aug 26, 2019

Migration of flutter/plugins#1900

f23b7e3

collinjackson mentioned this pull request Aug 26, 2019

[firebase_messaging] Add support for handling messages in background firebase/flutterfire#38

Merged

4 tasks

collinjackson closed this Aug 26, 2019

kroikie pushed a commit to collinjackson/flutterfire that referenced this pull request Sep 4, 2019

Migration of flutter/plugins#1900

0834c14

kroikie added a commit to firebase/flutterfire that referenced this pull request Sep 5, 2019

Merge pull request #38 from collinjackson/flutter/plugins#1900

bfc666d

[firebase_messaging] Add Android support for handling messages in background

JillyTaboga reviewed Sep 16, 2019

View reviewed changes

kroikie pushed a commit to collinjackson/flutterfire that referenced this pull request Nov 15, 2019

Migration of flutter/plugins#1900

5395f0f

nksteven pushed a commit to nksteven/ntf-firebase_messaging that referenced this pull request Apr 8, 2020

Migration of flutter/plugins#1900

e29683f


		By default background messaging is not enabled. To handle messages in the background:

		1. Add an Application.java class to your app

[firebase_messaging] Add support for handling messages in background #1900

[firebase_messaging] Add support for handling messages in background #1900

Conversation

kroikie commented Jul 24, 2019 • edited Loading

Description

Related Issues

Related PR

Breaking Change

collinjackson commented Jul 24, 2019

The-Redhat commented Jul 25, 2019 • edited Loading

kroikie commented Jul 25, 2019

kroikie commented Jul 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthew-carroll commented Aug 5, 2019

XExistence commented Aug 9, 2019

xster commented Aug 13, 2019

collinjackson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

collinjackson commented Aug 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brunoripa commented Sep 17, 2019

kroikie commented Jul 24, 2019 •

edited

Loading

The-Redhat commented Jul 25, 2019 •

edited

Loading