notif: Handle when app in background, too (on Android) #352
Conversation
gnprice
added
a-Android
There was a problem hiding this comment.
Thanks! Small comments below, then please merge at will.
Sadly this does not work very well if the app isn't running at all:
e.g., if you terminate the app by swiping it away in the app switcher.
In that case the notification can be quite a bit delayed.
FWIW, in some brief testing on the office Android device (Samsung Galaxy S9 running Android 9), I don't notice long delays in the case where the app isn't running at all. After I swipe it away in the app switcher, I can send myself DMs from a test account and still get a notification for each one within a few seconds.
test/model/binding.dart
Outdated
| /// Controls [TestZulipBinding.firebaseMessagingOnBackgroundMessage]. | ||
| /// | ||
| /// Calling [StreamController.add] on this will cause a call | ||
| /// to any handler registered through the latter. | ||
| StreamController<RemoteMessage> onBackgroundMessage = StreamController.broadcast(); |
There was a problem hiding this comment.
I'm getting lost about what "the latter" refers to. I'm not sure what the corresponding "former" is. Would it be accurate to replace "the latter" with [TestZulipBinding.firebaseMessagingOnBackgroundMessage]? But "the latter" doesn't seem like the right label for that, because nothing is mentioned before it.
There was a problem hiding this comment.
Hmm yeah. That is indeed what I meant it to refer to — basically I didn't want to repeat that very long name. :-)
Maybe I'll say "registered through that method"; that's at least accurate, it's potentially ambiguous with StreamController.add, but hopefully it's clear enough which of the two mentioned methods is meant.
| assert(() { | ||
| debugLogEnabled = true; | ||
| return true; | ||
| }()); |
There was a problem hiding this comment.
What's the reason for setting debugLogEnabled to true here?
There was a problem hiding this comment.
Same as in main (the one in lib/main.dart) — it causes the debugLog calls to be active when running a debug build of the app, so that you get them in flutter run output.
Or perhaps the question is: why does this code need to do that, when the whole rest of the app is covered by main doing so? That's because it's in a separate isolate, as mentioned in _onBackgroundMessage; same reason it needs the LiveZulipBinding.ensureInitialized and NotificationDisplayManager._init calls.
I'll add a comment pointing out the comparison to main.
Great! Well, maybe the symptoms of #342 vary between devices, then; perhaps it even only affects relatively few devices. One thing I wonder is if that's a difference in the app switcher UI — if maybe the one in Samsung's Android 9 leaves the app still in the background, in a way that the one in Google/Pixel's Android 13 doesn't. |
This has the same effect at present, but it seems a bit cleaner in that we'll end up rerunning the normal initializer for each field, rather than having to duplicate them here.
This completes the core functionality of showing notifications on Android, zulip#320. Sadly this does not work very well if the app isn't running at all: e.g., if you terminate the app by swiping it away in the app switcher. In that case the notification can be quite a bit delayed. But fixing that seems likely to require some deeper debugging, and getting our hands into Java or Kotlin code for I think the first time in zulip-flutter. So we'll deal with that as a followup issue, zulip#342. Details there. Fixes: zulip#320
|
Thanks for the review! Merged with those tweaks. |
This completes the core functionality of showing notifications on
Android, #320.
Sadly this does not work very well if the app isn't running at all:
e.g., if you terminate the app by swiping it away in the app switcher.
In that case the notification can be quite a bit delayed.
But fixing that seems likely to require some deeper debugging, and
getting our hands into Java or Kotlin code for I think the first time
in zulip-flutter. So we'll deal with that as a followup issue, #342.
Details there.
Fixes: #320