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

Show notifications! on Android, in foreground #344

Merged
merged 3 commits into from
Oct 31, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
6 changes: 6 additions & 0 deletions ios/Podfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Filed that thought as #351.

- Flutter
- GoogleDataTransport (9.2.5):
- GoogleUtilities/Environment (~> 7.7)
- nanopb (< 2.30910.0, >= 2.30908.0)
Expand Down Expand Up @@ -135,6 +137,7 @@ DEPENDENCIES:
- firebase_core (from `.symlinks/plugins/firebase_core/ios`)
- firebase_messaging (from `.symlinks/plugins/firebase_messaging/ios`)
- Flutter (from `Flutter`)
- flutter_local_notifications (from `.symlinks/plugins/flutter_local_notifications/ios`)
- image_picker_ios (from `.symlinks/plugins/image_picker_ios/ios`)
- package_info_plus (from `.symlinks/plugins/package_info_plus/ios`)
- path_provider_foundation (from `.symlinks/plugins/path_provider_foundation/darwin`)
Expand Down Expand Up @@ -172,6 +175,8 @@ EXTERNAL SOURCES:
:path: ".symlinks/plugins/firebase_messaging/ios"
Flutter:
:path: Flutter
flutter_local_notifications:
:path: ".symlinks/plugins/flutter_local_notifications/ios"
image_picker_ios:
:path: ".symlinks/plugins/image_picker_ios/ios"
package_info_plus:
Expand Down Expand Up @@ -199,6 +204,7 @@ SPEC CHECKSUMS:
FirebaseInstallations: b822f91a61f7d1ba763e5ccc9d4f2e6f2ed3b3ee
FirebaseMessaging: 80b4a086d20ed4fd385a702f4bfa920e14f5064d
Flutter: f04841e97a9d0b0a8025694d0796dd46242b2854
flutter_local_notifications: 0c0b1ae97e741e1521e4c1629a459d04b9aec743
GoogleDataTransport: 54dee9d48d14580407f8f5fbf2f496e92437a2f2
GoogleUtilities: 13e2c67ede716b8741c7989e26893d151b2b2084
image_picker_ios: 4a8aadfbb6dc30ad5141a2ce3832af9214a705b5
Expand Down
7 changes: 7 additions & 0 deletions lib/model/binding.dart
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import 'package:device_info_plus/device_info_plus.dart' as device_info_plus;
import 'package:firebase_core/firebase_core.dart' as firebase_core;
import 'package:firebase_messaging/firebase_messaging.dart' as firebase_messaging;
import 'package:flutter/foundation.dart';
import 'package:flutter_local_notifications/flutter_local_notifications.dart';
import 'package:url_launcher/url_launcher.dart' as url_launcher;

import '../firebase_options.dart';
Expand Down Expand Up @@ -98,6 +99,9 @@ abstract class ZulipBinding {

/// Wraps [firebase_messaging.FirebaseMessaging.onMessage].
Stream<firebase_messaging.RemoteMessage> get firebaseMessagingOnMessage;

/// Wraps the [FlutterLocalNotificationsPlugin] singleton constructor.
FlutterLocalNotificationsPlugin get notifications;
}

/// Like [device_info_plus.BaseDeviceInfo], but without things we don't use.
Expand Down Expand Up @@ -194,4 +198,7 @@ class LiveZulipBinding extends ZulipBinding {
Stream<firebase_messaging.RemoteMessage> get firebaseMessagingOnMessage {
return firebase_messaging.FirebaseMessaging.onMessage;
}

@override
FlutterLocalNotificationsPlugin get notifications => FlutterLocalNotificationsPlugin();
}
121 changes: 118 additions & 3 deletions lib/notifications.dart
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import 'package:flutter/foundation.dart';
import 'package:flutter_local_notifications/flutter_local_notifications.dart';

import 'api/notifications.dart';
import 'log.dart';
import 'model/binding.dart';
import 'widgets/app.dart';

class NotificationService {
static NotificationService get instance => (_instance ??= NotificationService._());
Expand Down Expand Up @@ -38,6 +40,7 @@ class NotificationService {
// TODO(#324) defer notif setup if user not logged into any accounts
// (in order to avoid calling for permissions)

await NotificationDisplayManager._init();
ZulipBinding.instance.firebaseMessagingOnMessage.listen(_onRemoteMessage);

// Get the FCM registration token, now and upon changes. See FCM API docs:
Expand Down Expand Up @@ -71,9 +74,121 @@ class NotificationService {
static void _onRemoteMessage(FirebaseRemoteMessage message) {
assert(debugLog("notif message: ${message.data}"));
final data = FcmMessage.fromJson(message.data);
if (data is MessageFcmMessage) {
assert(debugLog('notif message content: ${data.content}'));
// TODO(#122): show notification UI
switch (data) {
case MessageFcmMessage(): NotificationDisplayManager._onMessageFcmMessage(data, message.data);
case RemoveFcmMessage(): break; // TODO(#341) handle
case UnexpectedFcmMessage(): break; // TODO(log)
}
}
}

/// Service for configuring our Android "notification channel".
class NotificationChannelManager {
@visibleForTesting
static const kChannelId = 'messages-1';

/// The vibration pattern we set for notifications.
// We try to set a vibration pattern that, with the phone in one's pocket,
// is both distinctly present and distinctly different from the default.
// Discussion: https://chat.zulip.org/#narrow/stream/48-mobile/topic/notification.20vibration.20pattern/near/1284530
@visibleForTesting
static final kVibrationPattern = Int64List.fromList([0, 125, 100, 450]);

/// Create our notification channel, if it doesn't already exist.
//
// NOTE when changing anything here: the changes will not take effect
// for existing installs of the app! That's because we'll have already
// created the channel with the old settings, and they're in the user's
// hands from there. Our choices are:
//
// * Leave the old settings in place for existing installs, so the
// changes only apply to new installs.
//
// * Change `kChannelId`, so that we abandon the old channel and use
// a new one. Existing installs will get the new settings.
//
// This also means that if the user has changed any of the notification
// settings for the channel -- like "override Do Not Disturb", or "use
// a different sound", or "don't pop on screen" -- their changes get
// reset. So this has to be done sparingly.
//
// If we do this, we should also look for any channel with the old
// channel ID and delete it. See zulip-mobile's `createNotificationChannel`
// in android/app/src/main/java/com/zulipmobile/notifications/NotificationChannelManager.kt .
static Future<void> _ensureChannel() async {
final plugin = ZulipBinding.instance.notifications;
await plugin.resolvePlatformSpecificImplementation<AndroidFlutterLocalNotificationsPlugin>()
?.createNotificationChannel(AndroidNotificationChannel(
kChannelId,
'Messages', // TODO(i18n)
importance: Importance.high,
enableLights: true,
vibrationPattern: kVibrationPattern,
// TODO(#340) sound
));
}
}

/// Service for managing the notifications shown to the user.
class NotificationDisplayManager {
// We rely on the tag instead.
@visibleForTesting
static const kNotificationId = 0;

static Future<void> _init() async {
await ZulipBinding.instance.notifications.initialize(
const InitializationSettings(
android: AndroidInitializationSettings('zulip_notification'),
),
);
await NotificationChannelManager._ensureChannel();
}

static void _onMessageFcmMessage(MessageFcmMessage data, Map<String, dynamic> dataJson) {
assert(debugLog('notif message content: ${data.content}'));
final title = switch (data.recipient) {
FcmMessageStreamRecipient(:var streamName?, :var topic) =>
'$streamName > $topic',
FcmMessageStreamRecipient(:var topic) =>
'(unknown stream) > $topic', // TODO get stream name from data
FcmMessageDmRecipient(:var allRecipientIds) when allRecipientIds.length > 2 =>
'${data.senderFullName} to you and ${allRecipientIds.length - 2} others', // TODO(i18n), also plural; TODO use others' names, from data
FcmMessageDmRecipient() =>
data.senderFullName,
};
ZulipBinding.instance.notifications.show(
kNotificationId,
title,
data.content,
NotificationDetails(android: AndroidNotificationDetails(
NotificationChannelManager.kChannelId,
// This [FlutterLocalNotificationsPlugin.show] call can potentially create
// a new channel, if our channel doesn't already exist. That *shouldn't*
// happen; if it does, it won't get the right settings. Set the channel
// name in that case to something that has a chance of warning the user,
// and that can serve as a signature to diagnose the situation in support.
// But really we should fix flutter_local_notifications to not do that
// (see issue linked below), or replace that package entirely (#351).
'(Zulip internal error)', // TODO never implicitly create channel: https://github.com/MaikuB/flutter_local_notifications/issues/2135
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, exactly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a comment explaining this more.

tag: _conversationKey(data),
color: kZulipBrandColor,
icon: 'zulip_notification', // TODO vary for debug
// TODO(#128) inbox-style
)));
}

static String _conversationKey(MessageFcmMessage data) {
final groupKey = _groupKey(data);
final conversation = switch (data.recipient) {
FcmMessageStreamRecipient(:var streamId, :var topic) => 'stream:$streamId:$topic',
FcmMessageDmRecipient(:var allRecipientIds) => 'dm:${allRecipientIds.join(',')}',
};
return '$groupKey|$conversation';
}

static String _groupKey(FcmMessageWithIdentity data) {
// The realm URL can't contain a `|`, because `|` is not a URL code point:
// https://url.spec.whatwg.org/#url-code-points
return "${data.realmUri}|${data.userId}";
}
}
2 changes: 2 additions & 0 deletions macos/Flutter/GeneratedPluginRegistrant.swift
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import device_info_plus
import file_selector_macos
import firebase_core
import firebase_messaging
import flutter_local_notifications
import package_info_plus
import path_provider_foundation
import share_plus
Expand All @@ -20,6 +21,7 @@ func RegisterGeneratedPlugins(registry: FlutterPluginRegistry) {
FileSelectorPlugin.register(with: registry.registrar(forPlugin: "FileSelectorPlugin"))
FLTFirebaseCorePlugin.register(with: registry.registrar(forPlugin: "FLTFirebaseCorePlugin"))
FLTFirebaseMessagingPlugin.register(with: registry.registrar(forPlugin: "FLTFirebaseMessagingPlugin"))
FlutterLocalNotificationsPlugin.register(with: registry.registrar(forPlugin: "FlutterLocalNotificationsPlugin"))
FPPPackageInfoPlusPlugin.register(with: registry.registrar(forPlugin: "FPPPackageInfoPlusPlugin"))
PathProviderPlugin.register(with: registry.registrar(forPlugin: "PathProviderPlugin"))
SharePlusMacosPlugin.register(with: registry.registrar(forPlugin: "SharePlusMacosPlugin"))
Expand Down
6 changes: 6 additions & 0 deletions macos/Podfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ PODS:
- GoogleUtilities/Reachability (~> 7.8)
- GoogleUtilities/UserDefaults (~> 7.8)
- nanopb (< 2.30910.0, >= 2.30908.0)
- flutter_local_notifications (0.0.1):
- FlutterMacOS
- FlutterMacOS (1.0.0)
- GoogleDataTransport (9.2.5):
- GoogleUtilities/Environment (~> 7.7)
Expand Down Expand Up @@ -94,6 +96,7 @@ DEPENDENCIES:
- file_selector_macos (from `Flutter/ephemeral/.symlinks/plugins/file_selector_macos/macos`)
- firebase_core (from `Flutter/ephemeral/.symlinks/plugins/firebase_core/macos`)
- firebase_messaging (from `Flutter/ephemeral/.symlinks/plugins/firebase_messaging/macos`)
- flutter_local_notifications (from `Flutter/ephemeral/.symlinks/plugins/flutter_local_notifications/macos`)
- FlutterMacOS (from `Flutter/ephemeral`)
- package_info_plus (from `Flutter/ephemeral/.symlinks/plugins/package_info_plus/macos`)
- path_provider_foundation (from `Flutter/ephemeral/.symlinks/plugins/path_provider_foundation/darwin`)
Expand Down Expand Up @@ -123,6 +126,8 @@ EXTERNAL SOURCES:
:path: Flutter/ephemeral/.symlinks/plugins/firebase_core/macos
firebase_messaging:
:path: Flutter/ephemeral/.symlinks/plugins/firebase_messaging/macos
flutter_local_notifications:
:path: Flutter/ephemeral/.symlinks/plugins/flutter_local_notifications/macos
FlutterMacOS:
:path: Flutter/ephemeral
package_info_plus:
Expand All @@ -146,6 +151,7 @@ SPEC CHECKSUMS:
FirebaseCoreInternal: 26233f705cc4531236818a07ac84d20c333e505a
FirebaseInstallations: b822f91a61f7d1ba763e5ccc9d4f2e6f2ed3b3ee
FirebaseMessaging: 80b4a086d20ed4fd385a702f4bfa920e14f5064d
flutter_local_notifications: 3805ca215b2fb7f397d78b66db91f6a747af52e4
FlutterMacOS: 8f6f14fa908a6fb3fba0cd85dbd81ec4b251fb24
GoogleDataTransport: 54dee9d48d14580407f8f5fbf2f496e92437a2f2
GoogleUtilities: 13e2c67ede716b8741c7989e26893d151b2b2084
Expand Down
56 changes: 56 additions & 0 deletions pubspec.lock
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,14 @@ packages:
url: "https://pub.dev"
source: hosted
version: "2.3.3"
dbus:
dependency: transitive
description:
name: dbus
sha256: "6f07cba3f7b3448d42d015bfd3d53fe12e5b36da2423f23838efc1d5fb31a263"
url: "https://pub.dev"
source: hosted
version: "0.7.8"
device_info_plus:
dependency: "direct main"
description:
Expand Down Expand Up @@ -414,6 +422,30 @@ packages:
url: "https://pub.dev"
source: hosted
version: "3.0.0"
flutter_local_notifications:
dependency: "direct main"
description:
name: flutter_local_notifications
sha256: "6d11ea777496061e583623aaf31923f93a9409ef8fcaeeefdd6cd78bf4fe5bb3"
url: "https://pub.dev"
source: hosted
version: "16.1.0"
flutter_local_notifications_linux:
dependency: transitive
description:
name: flutter_local_notifications_linux
sha256: "33f741ef47b5f63cc7f78fe75eeeac7e19f171ff3c3df054d84c1e38bedb6a03"
url: "https://pub.dev"
source: hosted
version: "4.0.0+1"
flutter_local_notifications_platform_interface:
dependency: "direct main"
description:
name: flutter_local_notifications_platform_interface
sha256: "7cf643d6d5022f3baed0be777b0662cce5919c0a7b86e700299f22dc4ae660ef"
url: "https://pub.dev"
source: hosted
version: "7.0.0+1"
flutter_localizations:
dependency: "direct main"
description: flutter
Expand Down Expand Up @@ -757,6 +789,14 @@ packages:
url: "https://pub.dev"
source: hosted
version: "2.2.1"
petitparser:
dependency: transitive
description:
name: petitparser
sha256: cb3798bef7fc021ac45b308f4b51208a152792445cce0448c9a4ba5879dd8750
url: "https://pub.dev"
source: hosted
version: "5.4.0"
platform:
dependency: transitive
description:
Expand Down Expand Up @@ -994,6 +1034,14 @@ packages:
url: "https://pub.dev"
source: hosted
version: "0.5.9"
timezone:
dependency: transitive
description:
name: timezone
sha256: "1cfd8ddc2d1cfd836bc93e67b9be88c3adaeca6f40a00ca999104c30693cdca0"
url: "https://pub.dev"
source: hosted
version: "0.9.2"
timing:
dependency: transitive
description:
Expand Down Expand Up @@ -1154,6 +1202,14 @@ packages:
url: "https://pub.dev"
source: hosted
version: "1.0.3"
xml:
dependency: transitive
description:
name: xml
sha256: "5bc72e1e45e941d825fd7468b9b4cc3b9327942649aeb6fc5cdbf135f0a86e84"
url: "https://pub.dev"
source: hosted
version: "6.3.0"
yaml:
dependency: transitive
description:
Expand Down
2 changes: 2 additions & 0 deletions pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ dependencies:
sdk: flutter
firebase_messaging: ^14.6.3
firebase_core: ^2.14.0
flutter_local_notifications_platform_interface: ^7.0.0+1
flutter_local_notifications: ^16.1.0

dev_dependencies:
flutter_test:
Expand Down
Loading