Show notifications! on Android, in foreground #344
Conversation
| @@ -70,6 +70,8 @@ PODS: | |||
| - GoogleUtilities/UserDefaults (~> 7.8) | |||
| - nanopb (< 2.30910.0, >= 2.30908.0) | |||
| - Flutter (1.0.0) | |||
| - flutter_local_notifications (0.0.1): | |||
There was a problem hiding this comment.
flutter_local_notifications
Hmm. We don't have any local notifications to show, only remote ones. In my understanding (mostly informed by iOS), local notifications are for things like timers that the app sets independently of any messages received from a server. Evidently this has APIs we can use for our remote notifications, but it might also have lots of baggage for functionality we'll never need.
There was a problem hiding this comment.
Yeah, good question. Replied in chat here: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/notification.20libraries/near/1670983
In short: indeed it does have such baggage, and we'll likely want to move to writing our own thin layer with Pigeon in the future, but this library is what's recommended by Flutter upstream and seems to be the best existing thing to use.
There was a problem hiding this comment.
Filed that thought as #351.
| data.content, | ||
| NotificationDetails(android: AndroidNotificationDetails( | ||
| NotificationChannelManager.kChannelId, | ||
| '(Zulip internal error)', // TODO never implicitly create channel: https://github.com/MaikuB/flutter_local_notifications/issues/2135 |
There was a problem hiding this comment.
Hmm, it looks like "(Zulip internal error)" is passed as a channelName argument. I don't follow yet; why that name?
…Oh I guess the point is we don't want to pass any channel name at all, because we don't want this call to create a channel at all (I see the issue you filed). But we have to choose a name, so we choose this one. And maybe if a channel is created, the string will be presented to the user somewhere, like in the system notification settings for the app?
There was a problem hiding this comment.
Added a comment explaining this more.
We'll use the latter for writing our binding class.
This implements most of the remaining work of zulip#320. It doesn't yet deliver most of the user-facing value: for that, the remaining piece will be to make notifications appear when the app is in the background as well as when it's in the foreground. I have a draft for that and it works, but the tests require some more fiddling which I may not get to today, so I wanted to get the bulk of this posted for review. Many other improvements still to be made are tracked as their own issues: most notably iOS support zulip#321, and opening a conversation by tapping the notification zulip#123.
|
Thanks for the review! Revised to add a comment, and merged. |
This implements most of the remaining work of #320.
It doesn't yet deliver most of the user-facing value: for that, the
remaining piece will be to make notifications appear when the app is
in the background as well as when it's in the foreground. I have a
draft for that and it works, but the tests require some more fiddling
which I may not get to today, so I wanted to get the bulk of this
posted for review.
Many other improvements still to be made are tracked as their own
issues: most notably iOS support #321, and opening a conversation
by tapping the notification #123.