Replace `flutter_local_notifications` with pigeon bindings #856

rajveermalviya · 2024-08-01T18:27:52Z

Fixes: #351

gnprice · 2024-08-02T00:55:36Z

Exciting!

LMK if there are questions you have for me at this stage, or particular spots in the code you'd like an early high-level review of.

rajveermalviya · 2024-08-02T04:15:16Z

@gnprice, I have a question about whether this implementation is desirable. Currently it creates a pending intent using the ACTION_VIEW action with a URL payload for each notification. When the user opens the notification, the intent functions as a deeplink and is handled by the existing URL handling in Flutter. Then this url is parsed and handled here for incoming intents while the app is open and while the app is closed.

Also, when the intent is created we set explicity set the reciever component so that ensures that this instance of zulip://notificiation_open url intent is only handled by our app.

This way this implementation avoids creating a custom intent handler and reuses Flutter existing URL route handler.

rajveermalviya · 2024-08-02T04:24:27Z

Moved the discussion to chat thread here — https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/flutter.3A.20Drop.20.60flutter_local_notifications.60.20.23F856/near/1910352

Khader-1

Thanks @rajveermalviya! LGTM.

kenclary

lgtm

chrisbobbe

Thanks! Comments below, all small. Also, I haven't done any manual testing of this; did you?

chrisbobbe · 2024-09-09T21:55:00Z

android/app/src/main/kotlin/com/zulip/flutter/ZulipPlugin.kt

@@ -87,7 +87,7 @@ private class AndroidNotificationHost(val context: Context)
                        // FlutterLocalNotificationsPlugin, which handles receiving the Intent.
                        // TODO take care of receiving the notification-opened Intent ourselves


notif [nfc]: Replace intentPayload with AndroidIntent.extras

At this commit, the reference to "This […] extra name" seems like it's dangling; it's not clear from the code next to the comment that it means the string "payload". Can you make that fact clearer? It may also help to comment on the spot in the code where the string "payload" appears.

Also, I see that this special name "payload" remains after removing flutter_local_notifications. Is that because we're happy/neutral to keep it around, or is it a sign that there's still helpful cleanup of flutter_local_notifications to do?

Updated with main commit (notif: Handle opening of notification via URI intents) removing dangling references for payload.

chrisbobbe · 2024-09-09T22:49:56Z

lib/widgets/app.dart

+                HomePage.buildRoute(accountId: accountId),
+                InboxPage.buildRoute(accountId: accountId),
+                notificationRoute,
+              ] else if (initialAccountId != null) ...[
                HomePage.buildRoute(accountId: initialAccountId),
                InboxPage.buildRoute(accountId: initialAccountId),
              ],


notif: Introduce URL based notif open implementation

This looks like maybe a behavior change, right? Before, when you opened the app from a notification, the home page and inbox page would be for the first account in the list (the "initial account" in initialAccountId), even if that was different from the notification's account. Now, those pages are chosen to match the notification.

I agree the new behavior seems better. Let's mention the change, though, and any other user-visible behavior changes, in the commit message.

Removed this behaviour (for now) to keep the diff smaller, this would probably be better done in a separate PR.

chrisbobbe · 2024-09-13T23:16:57Z

It looks like there's been some further discussion on these changes: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/narrow.20links.20with.20userid.3F/near/1941174

Please post here when that's resolved and this is ready for another review. 🙂

gnprice

Thanks! Finished reviewing the whole branch, and generally this all looks great — just small comments below.

Would you do some manual testing of these changes, if you haven't already? In particular, see my PRs that implemented notifications and made previous changes to them; I describe there some test protocols we should repeat for these changes.

In general, notifications are one of the small number of areas where manual testing is needed to confirm things still work correctly after any significant change. That's because a key part of its behavior depends on interacting correctly with external APIs that (a) are supplied from the platform, so aren't present in a flutter test environment, and (b) have a lot of subtle details of their semantics, not all of which are necessarily even documented, and which can be easy to miss even if they are.

gnprice · 2024-10-27T01:08:15Z

test/notifications/display_test.dart

+          // throws 'TypeError'.
+          // Missing 'all_recipient_ids', when narrow_type == 'dm'
+          // throws 'TypeError'.
+          .throws<dynamic>();


nit: avoid saying dynamic except where we really mean dynamic behavior (which is rare):

Suggested change

.throws<dynamic>();

.throws<Object>();

The exact equivalent to saying dynamic here would be to say Object?, which is the supertype of all types. But then we may as well be more precise: null can't be thrown, so the object thrown will always be an instance of Object.

gnprice · 2024-10-27T01:10:11Z

lib/api/notifications.dart

@@ -147,6 +153,7 @@ class MessageFcmMessage extends FcmMessageWithIdentity {
        if (recipient.streamName != null) result['stream'] = recipient.streamName;
        result['topic'] = recipient.topic;
    }
+    result['realm_uri'] = realmUrl.toString(); // TODO(server-9); deprecated in FL 257


nit:

Suggested change

result['realm_uri'] = realmUrl.toString(); // TODO(server-9); deprecated in FL 257

result['realm_uri'] = realmUrl.toString(); // TODO(server-9): deprecated in FL 257

(also one below; the one above is already a colon :)

gnprice · 2024-10-27T01:14:28Z

test/api/notifications_test.dart

+      test("${n++}", () => checkParseFails({ ...dmJson }
+                                            ..remove('realm_url')
+                                            ..remove('realm_uri')));


Suggested change

test("${n++}", () => checkParseFails({ ...dmJson }

..remove('realm_url')

..remove('realm_uri')));

test("${n++}", () => checkParseFails({ ...dmJson }

..remove('realm_url')

..remove('realm_uri'))); // TODO(server-9)

That way we'll spot this line to remove it when we're removing all the pre-server-9 compatibility code.

gnprice · 2024-10-27T01:14:53Z

test/api/notifications_test.dart

    "realm_uri": "https://zulip.example.com/",
+    "realm_url": "https://zulip.example.com/",


similarly:

Suggested change

"realm_uri": "https://zulip.example.com/",

"realm_url": "https://zulip.example.com/",

"realm_uri": "https://zulip.example.com/", // TODO(server-9)

"realm_url": "https://zulip.example.com/",

gnprice · 2024-10-27T01:16:46Z

test/api/notifications_test.dart

+      // FL 257 deprecated 'realm_uri' in favor of 'realm_url'.
+      final jsonSansRealm =
+        { ...dmJson }..remove('realm_url')..remove('realm_uri');
+      check(parse({ ...jsonSansRealm, 'realm_uri': 'https://zulip.example.com/' })).jsonEquals(baseline);


(OTOH no need to add a TODO comment here — this test will naturally fail when we remove the fallback logic itself, which has its own such comment, and so we'll naturally find this test and remove it.)

gnprice · 2024-10-27T01:17:48Z

test/api/notifications_test.dart

+    test('uses deprecated fields when newer fields are missing', () {
+      final baseline = parse(baseJson);
+
+      // FL 257 deprecated 'realm_uri' in favor of 'realm_url'.
+      final jsonSansRealm =
+        { ...baseJson }..remove('realm_url')..remove('realm_uri');
+      check(parse({ ...jsonSansRealm, 'realm_url': 'https://zulip.example.com/' })).jsonEquals(baseline);


Suggested change

test('uses deprecated fields when newer fields are missing', () {

final baseline = parse(baseJson);

// FL 257 deprecated 'realm_uri' in favor of 'realm_url'.

final jsonSansRealm =

{ ...baseJson }..remove('realm_url')..remove('realm_uri');

check(parse({ ...jsonSansRealm, 'realm_url': 'https://zulip.example.com/' })).jsonEquals(baseline);

test('uses deprecated fields when newer fields are missing', () {

final baseline = parse(baseJson);

// FL 257 deprecated 'realm_uri' in favor of 'realm_url'.

final jsonSansRealm =

{ ...baseJson }..remove('realm_url')..remove('realm_uri');

check(parse({ ...jsonSansRealm, 'realm_uri': 'https://zulip.example.com/' })).jsonEquals(baseline);

right?

gnprice · 2024-10-27T02:05:16Z

pubspec.yaml

@@ -45,8 +45,6 @@ dependencies:
  firebase_core: ^3.1.0
  firebase_messaging: ^15.0.1
  flutter_color_models: ^1.3.3+2
-  flutter_local_notifications: ^17.2.1


Searching for remaining references to this package, I find just one:

$ git grep -i 'flutter.?local.?notif' android/app/src/main/res/raw/keep.xml: The issue is that the package:flutter_local_notifications API has us

So that should be updated too.

gnprice · 2024-10-27T02:16:07Z

lib/host/android_notifications.dart

+/// See: https://developer.android.com/reference/android/content/Intent#constants_1
+abstract class IntentFlag {


nit:

Suggested change

/// See: https://developer.android.com/reference/android/content/Intent#constants_1

abstract class IntentFlag {

/// See: https://developer.android.com/reference/android/content/Intent#FLAG_ACTIVITY_BROUGHT_TO_FRONT

abstract class IntentFlag {

There's a ton of constants on that page, so going straight to one of the flags (the one that's currently first in the list) saves some scrolling.

gnprice · 2024-10-27T02:22:53Z

pigeon/notifications.dart

+///   https://developer.android.com/reference/android/content/Intent
+///   https://developer.android.com/reference/android/content/Intent#Intent(java.lang.String,%20android.net.Uri,%20android.content.Context,%20java.lang.Class%3C?%3E)
+class AndroidIntent {
+  AndroidIntent({required this.action, required this.dataUrl, required this.flags});


nit:

Suggested change

AndroidIntent({required this.action, required this.dataUrl, required this.flags});

AndroidIntent({required this.action, required this.dataUrl, this.flags = 0});

This isn't a parameter of the underlying constructor, so effectively it's optional in the underlying API. (One has to set it later after construction.) So making it optional here seems the most faithful to that API.

rajveermalviya · 2024-10-27T19:03:53Z

Thanks for the review @gnprice! Pushed a new revision, PTAL.

Although I was unable to find the mentioned test protocol, I did some fresh manual testing of following scenarios:

Recieved muliple messages from two different users in two different streams and thus different topics, notifications are correctly displayed and routed to correct conversation when opened.
Recieved muliple messages from two different users in same stream but two different topics, notifications are correctly displayed and routed to correct conversation when opened.
Recieved a message in one-one DM, notification is correctly displayed and routed to the DM page when opened.
Recieved messages in group DM with two users (total 3 users), notification is correctly displayed and routed to the group DM page when opened.

(Tested the above cases with both foreground and background notifications)

Recieved messages in a stream+topic conversation, notification is correctly displayed, marked as read the conversation from web client, notifications for that specific conversation were removed.
Recieved messages in a one-one DM, notification is correctly displayed, marked as read the conversation from web client, notifications for that specific conversation were removed.

rajveermalviya · 2024-10-27T19:07:47Z

In addition to that, I also had the mobile notifications enabled for all channels in multiple (busy) servers to test the realm group summary notifications.

gnprice

Cool, thanks for that testing. That set of scenarios sounds solid.

The revision also looks good. Just one comment below, and then I think this is ready to merge.

gnprice · 2024-10-27T22:10:01Z

test/api/notifications_test.dart

+      // FL 257 deprecated 'realm_uri' in favor of 'realm_url'.
+      final jsonSansRealm =
+        { ...baseJson }..remove('realm_url')..remove('realm_uri');
+      check(parse({ ...jsonSansRealm, 'realm_url': 'https://zulip.example.com/' })).jsonEquals(baseline);


bump; and I think in fact this revision took this point in the opposite direction 🙂

I believe the point of this test is to check that when the old name is present, and not the new name, we get the data from the old name. So the input this test constructs to pass to parse should use the old name.

Right, sorry about that! Updated to use the older field.

rajveermalviya · 2024-10-28T03:48:33Z

Thanks for the review @gnprice. Pushed a new revision, PTAL.

Zulip server 9.0 in feature level 257, deprecated `realm_uri` in favor of `realm_url` in push notification payloads: https://zulip.com/api/changelog#changes-in-zulip-90 To be compatible with older servers, added a fallback to use both fields while parsing, otherwise use the newer field in code.

…load

This should never be possible.

Replaces the notification-open handling provided by `package:flutter_local_notifications` with a custom Intent data URL-based approach. Notifications will now be created with a `PendingIntent` containing a `zulip://notification…` URL. When a notification is opened, the Android intent's data URL is passed through Flutter's platform-navigation system, triggering a route. If the app is already in the foreground, the `ZulipApp` widget's overridden `didPushRouteInformation` function handles the route. If the app is not running, the intent data URL becomes the initial route, read from `WidgetsBinding.instance.platformDispatcher.defaultRouteName` in the `ZulipApp` widget. In both cases, the intent data URL is then passed to `NotificationDisplayManager.navigateForNotification`, where it is parsed and used to generate the route to the `MessageListPage` for the relevant conversation.

….xml`

Fixes: zulip#351

As we no longer depend on `package:flutter_local_notifications`, which required it.

The intent data URL for notification embed the conversation information, stream-id and topic name for topic conversation, and recipient-ids for dm conversations. So a distinct `requestCode` isn't necessary anymore, set it to zero (matching zulip-mobile).

Docs: https://developer.android.com/reference/android/app/PendingIntent#FLAG_UPDATE_CURRENT According to the documentation, this flag is only necessary when we want to update the extras data without causing a new PendingIntent entry to be created. Since we no longer pass any extras data, this flag is no longer required.

…obile

Matching the `requestCode` is no longer necessary due to the removal of `pakcage:flutter_local_notifications`, which required it.

gnprice · 2024-10-28T06:35:45Z

Looks good — merging. Thanks for all your work getting this transition finished!

rajveermalviya force-pushed the pr-drop-fln branch from 63714e3 to 0747551 Compare August 1, 2024 20:02

rajveermalviya force-pushed the pr-drop-fln branch 6 times, most recently from 984da67 to 1825510 Compare August 21, 2024 01:45

rajveermalviya marked this pull request as ready for review August 21, 2024 02:06

rajveermalviya added the buddy review GSoC buddy review needed. label Aug 21, 2024

rajveermalviya assigned Khader-1 Aug 21, 2024

rajveermalviya force-pushed the pr-drop-fln branch from 1825510 to a860492 Compare August 22, 2024 09:44

rajveermalviya unassigned Khader-1 Aug 22, 2024

rajveermalviya requested a review from Khader-1 August 22, 2024 09:44

Khader-1 approved these changes Aug 23, 2024

View reviewed changes

Khader-1 added mentor review GSoC mentor review needed. and removed buddy review GSoC buddy review needed. labels Aug 23, 2024

Khader-1 requested review from kenclary and sumanthvrao August 23, 2024 08:15

kenclary approved these changes Aug 30, 2024

View reviewed changes

gnprice added the maintainer review PR ready for review by Zulip maintainers label Sep 5, 2024

chrisbobbe reviewed Sep 9, 2024

View reviewed changes

rajveermalviya force-pushed the pr-drop-fln branch from a860492 to 7b44e8b Compare September 11, 2024 16:40

gnprice assigned chrisbobbe Sep 12, 2024

rajveermalviya force-pushed the pr-drop-fln branch from 7b44e8b to 90e62f4 Compare September 12, 2024 21:12

chrisbobbe removed the mentor review GSoC mentor review needed. label Oct 9, 2024

rajveermalviya force-pushed the pr-drop-fln branch from 90e62f4 to 0cf3d46 Compare October 15, 2024 23:08

gnprice mentioned this pull request Oct 27, 2024

deps android: Upgrade Android Gradle Plugin #887

Merged

gnprice reviewed Oct 27, 2024

View reviewed changes

rajveermalviya force-pushed the pr-drop-fln branch 2 times, most recently from 2e15594 to b5a7933 Compare October 27, 2024 07:56

rajveermalviya requested a review from gnprice October 27, 2024 19:03

gnprice mentioned this pull request Oct 27, 2024

Support setting typing status #897

Merged

gnprice reviewed Oct 27, 2024

View reviewed changes

rajveermalviya force-pushed the pr-drop-fln branch from b5a7933 to 73837e4 Compare October 28, 2024 03:43

rajveermalviya requested a review from gnprice October 28, 2024 03:48

rajveermalviya added 13 commits October 27, 2024 23:32

notif [nfc]: Introduce AndroidIntent; replace PendingIntent.intentPay…

3fa51c5

…load

notif test: Fix incorrect test accounts with same account-ids

311271e

This should never be possible.

notif [nfc]: Remove unused FlutterLocalNotificationsPlugin bindings

3a45399

notif android [nfc]: Pigeon bindings too need resource names in `keep…

8dbab86

….xml`

deps: Remove flutter_local_notifications

05a07f8

Fixes: zulip#351

android build: Remove use of core-library desugaring

3973460

As we no longer depend on `package:flutter_local_notifications`, which required it.

notif: Introduce AndroidIntent.flags; use existing flags from zulip-m…

0129165

…obile

notif [nfc]: Remove unused AndroidIntent.extras

91e4004

notif: Use the same notification ID for every notification

f86498e

Matching the `requestCode` is no longer necessary due to the removal of `pakcage:flutter_local_notifications`, which required it.

gnprice force-pushed the pr-drop-fln branch from 73837e4 to f86498e Compare October 28, 2024 06:35

gnprice merged commit f86498e into zulip:main Oct 28, 2024

rajveermalviya deleted the pr-drop-fln branch October 28, 2024 07:01

PIG208 mentioned this pull request Oct 22, 2024

notif: Use Zulip's custom notification sound on Android #982

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace `flutter_local_notifications` with pigeon bindings #856

Replace `flutter_local_notifications` with pigeon bindings #856

rajveermalviya commented Aug 1, 2024 •

edited

Loading

gnprice commented Aug 2, 2024

rajveermalviya commented Aug 2, 2024

rajveermalviya commented Aug 2, 2024

Khader-1 left a comment •

edited

Loading

kenclary left a comment

chrisbobbe left a comment

chrisbobbe Sep 9, 2024

chrisbobbe Sep 9, 2024

rajveermalviya Oct 15, 2024

chrisbobbe Sep 9, 2024

rajveermalviya Oct 15, 2024

chrisbobbe commented Sep 13, 2024

gnprice left a comment

gnprice Oct 27, 2024

gnprice Oct 27, 2024

gnprice Oct 27, 2024

gnprice Oct 27, 2024

gnprice Oct 27, 2024

gnprice Oct 27, 2024

gnprice Oct 27, 2024

gnprice Oct 27, 2024

gnprice Oct 27, 2024

rajveermalviya commented Oct 27, 2024

rajveermalviya commented Oct 27, 2024

gnprice left a comment

gnprice Oct 27, 2024

rajveermalviya Oct 28, 2024

rajveermalviya commented Oct 28, 2024

gnprice commented Oct 28, 2024

		@@ -87,7 +87,7 @@ private class AndroidNotificationHost(val context: Context)
		// FlutterLocalNotificationsPlugin, which handles receiving the Intent.
		// TODO take care of receiving the notification-opened Intent ourselves

	result['realm_uri'] = realmUrl.toString(); // TODO(server-9); deprecated in FL 257
	result['realm_uri'] = realmUrl.toString(); // TODO(server-9): deprecated in FL 257

		"realm_uri": "https://zulip.example.com/",
		"realm_url": "https://zulip.example.com/",

		/// See: https://developer.android.com/reference/android/content/Intent#constants_1
		abstract class IntentFlag {

	AndroidIntent({required this.action, required this.dataUrl, required this.flags});
	AndroidIntent({required this.action, required this.dataUrl, this.flags = 0});

Replace flutter_local_notifications with pigeon bindings #856

Replace flutter_local_notifications with pigeon bindings #856

Conversation

rajveermalviya commented Aug 1, 2024 • edited Loading

gnprice commented Aug 2, 2024

rajveermalviya commented Aug 2, 2024

rajveermalviya commented Aug 2, 2024

Khader-1 left a comment • edited Loading

Choose a reason for hiding this comment

kenclary left a comment

Choose a reason for hiding this comment

chrisbobbe 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Sep 13, 2024

gnprice 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

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

rajveermalviya commented Oct 27, 2024

rajveermalviya commented Oct 27, 2024

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajveermalviya commented Oct 28, 2024

gnprice commented Oct 28, 2024

Replace `flutter_local_notifications` with pigeon bindings #856

Replace `flutter_local_notifications` with pigeon bindings #856

rajveermalviya commented Aug 1, 2024 •

edited

Loading

Khader-1 left a comment •

edited

Loading