Skip to content

Commit d412a04

Browse files
gnpricegnprice
committed
notif: Switch to our own Pigeon-based plugin for showing a notification
This delivers the first major tranche of #351, switching from package:flutter_local_notifications to our own thin API binding based on Pigeon. With this commit, we've converted over one of the handful of different methods we use from that package, and it's the most complex of them by a substantial margin. Fixes-partly: #351
1 parent 2a37606 commit d412a04

File tree

2 files changed

+70
-43
lines changed

2 files changed

+70
-43
lines changed

‎lib/notifications/display.dart

+37-34
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import 'package:flutter/widgets.dart';
77
import 'package:flutter_local_notifications/flutter_local_notifications.dart';
88

99
import '../api/notifications.dart';
10+
import '../host/android_notifications.dart';
1011
import '../log.dart';
1112
import '../model/binding.dart';
1213
import '../model/narrow.dart';
@@ -99,40 +100,42 @@ class NotificationDisplayManager {
99100
data.senderFullName,
100101
};
101102
final conversationKey = _conversationKey(data);
102-
ZulipBinding.instance.notifications.show(
103-
// When creating the PendingIntent for the user to open the notification,
104-
// the plugin makes the underlying Intent objects look the same.
105-
// They differ in their extras, but that doesn't count:
106-
// https://developer.android.com/reference/android/app/PendingIntent
107-
//
108-
// This leaves only PendingIntent.requestCode to distinguish one
109-
// PendingIntent from another; the plugin sets that to the notification ID.
110-
// We need a distinct PendingIntent for each conversation, so that the
111-
// notifications can lead to the right conversations when opened.
112-
// So, use a hash of the conversation key.
113-
notificationIdAsHashOf(conversationKey),
114-
title,
115-
data.content,
116-
payload: jsonEncode(dataJson),
117-
NotificationDetails(android: AndroidNotificationDetails(
118-
NotificationChannelManager.kChannelId,
119-
// This [FlutterLocalNotificationsPlugin.show] call can potentially create
120-
// a new channel, if our channel doesn't already exist. That *shouldn't*
121-
// happen; if it does, it won't get the right settings. Set the channel
122-
// name in that case to something that has a chance of warning the user,
123-
// and that can serve as a signature to diagnose the situation in support.
124-
// But really we should fix flutter_local_notifications to not do that
125-
// (see issue linked below), or replace that package entirely (#351).
126-
'(Zulip internal error)', // TODO never implicitly create channel: https://github.com/MaikuB/flutter_local_notifications/issues/2135
127-
tag: conversationKey,
128-
color: kZulipBrandColor,
129-
// TODO vary notification icon for debug
130-
icon: 'zulip_notification', // This name must appear in keep.xml too: https://github.com/zulip/zulip-flutter/issues/528
131-
// TODO(#128) inbox-style
132-
133-
// TODO plugin sets PendingIntent.FLAG_UPDATE_CURRENT; is that OK?
134-
// TODO plugin doesn't set our Intent flags; is that OK?
135-
)));
103+
ZulipBinding.instance.androidNotificationHost.notify(
104+
// TODO the notification ID can be constant, instead of matching requestCode
105+
// (This is a legacy of `flutter_local_notifications`.)
106+
id: notificationIdAsHashOf(conversationKey),
107+
tag: conversationKey,
108+
channelId: NotificationChannelManager.kChannelId,
109+
110+
contentTitle: title,
111+
contentText: data.content,
112+
color: kZulipBrandColor.value,
113+
// TODO vary notification icon for debug
114+
smallIconResourceName: 'zulip_notification', // This name must appear in keep.xml too: https://github.com/zulip/zulip-flutter/issues/528
115+
// TODO(#128) inbox-style
116+
117+
contentIntent: PendingIntent(
118+
// TODO make intent URLs distinct, instead of requestCode
119+
// (This way is a legacy of flutter_local_notifications.)
120+
// The Intent objects we make for different conversations look the same.
121+
// They differ in their extras, but that doesn't count:
122+
// https://developer.android.com/reference/android/app/PendingIntent
123+
//
124+
// This leaves only PendingIntent.requestCode to distinguish one
125+
// PendingIntent from another; the plugin sets that to the notification ID.
126+
// We need a distinct PendingIntent for each conversation, so that the
127+
// notifications can lead to the right conversations when opened.
128+
// So, use a hash of the conversation key.
129+
requestCode: notificationIdAsHashOf(conversationKey),
130+
131+
// TODO is setting PendingIntentFlag.updateCurrent OK?
132+
// (That's a legacy of `flutter_local_notifications`.)
133+
flags: PendingIntentFlag.immutable | PendingIntentFlag.updateCurrent,
134+
intentPayload: jsonEncode(dataJson),
135+
// TODO this doesn't set the Intent flags we set in zulip-mobile; is that OK?
136+
// (This is a legacy of `flutter_local_notifications`.)
137+
),
138+
);
136139
}
137140

138141
/// A notification ID, derived as a hash of the given string key.

‎test/notifications/display_test.dart

+33-9
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import 'package:flutter_local_notifications/flutter_local_notifications.dart' hi
99
import 'package:flutter_test/flutter_test.dart';
1010
import 'package:zulip/api/model/model.dart';
1111
import 'package:zulip/api/notifications.dart';
12+
import 'package:zulip/host/android_notifications.dart';
1213
import 'package:zulip/model/narrow.dart';
1314
import 'package:zulip/model/store.dart';
1415
import 'package:zulip/notifications/display.dart';
@@ -111,16 +112,21 @@ void main() {
111112
final expectedTag = '${data.realmUri}|${data.userId}|$expectedTagComponent';
112113
final expectedId =
113114
NotificationDisplayManager.notificationIdAsHashOf(expectedTag);
114-
check(testBinding.notifications.takeShowCalls()).single
115+
const expectedIntentFlags =
116+
PendingIntentFlag.immutable | PendingIntentFlag.updateCurrent;
117+
check(testBinding.androidNotificationHost.takeNotifyCalls()).single
115118
..id.equals(expectedId)
116-
..title.equals(expectedTitle)
117-
..body.equals(data.content)
118-
..payload.equals(jsonEncode(data.toJson()))
119-
..notificationDetails.isNotNull().android.isNotNull().which((it) => it
120-
..channelId.equals(NotificationChannelManager.kChannelId)
121-
..tag.equals(expectedTag)
122-
..color.equals(kZulipBrandColor)
123-
..icon.equals('zulip_notification')
119+
..tag.equals(expectedTag)
120+
..channelId.equals(NotificationChannelManager.kChannelId)
121+
..contentTitle.equals(expectedTitle)
122+
..contentText.equals(data.content)
123+
..color.equals(kZulipBrandColor.value)
124+
..smallIconResourceName.equals('zulip_notification')
125+
..extras.isNull()
126+
..contentIntent.which((it) => it.isNotNull()
127+
..requestCode.equals(expectedId)
128+
..flags.equals(expectedIntentFlags)
129+
..intentPayload.equals(jsonEncode(data.toJson()))
124130
);
125131
}
126132

@@ -358,6 +364,24 @@ extension ShowCallChecks on Subject<FlutterLocalNotificationsPluginShowCall> {
358364
Subject<String?> get payload => has((x) => x.payload, 'payload');
359365
}
360366

367+
extension on Subject<AndroidNotificationHostApiNotifyCall> {
368+
Subject<String?> get tag => has((x) => x.tag, 'tag');
369+
Subject<int> get id => has((x) => x.id, 'id');
370+
Subject<String> get channelId => has((x) => x.channelId, 'channelId');
371+
Subject<int?> get color => has((x) => x.color, 'color');
372+
Subject<PendingIntent?> get contentIntent => has((x) => x.contentIntent, 'contentIntent');
373+
Subject<String?> get contentText => has((x) => x.contentText, 'contentText');
374+
Subject<String?> get contentTitle => has((x) => x.contentTitle, 'contentTitle');
375+
Subject<Map<String?, String?>?> get extras => has((x) => x.extras, 'extras');
376+
Subject<String?> get smallIconResourceName => has((x) => x.smallIconResourceName, 'smallIconResourceName');
377+
}
378+
379+
extension on Subject<PendingIntent> {
380+
Subject<int> get requestCode => has((x) => x.requestCode, 'requestCode');
381+
Subject<String> get intentPayload => has((x) => x.intentPayload, 'intentPayload');
382+
Subject<int> get flags => has((x) => x.flags, 'flags');
383+
}
384+
361385
extension NotificationDetailsChecks on Subject<NotificationDetails> {
362386
Subject<AndroidNotificationDetails?> get android => has((x) => x.android, 'android');
363387
Subject<DarwinNotificationDetails?> get iOS => has((x) => x.iOS, 'iOS');

0 commit comments

Comments
 (0)