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. 🙂

…M messages Prep commit for upcoming implementation where for foreground notifications it'd depend on GlobalStore inherited widget and thus would require the use of `testWidget` instead of `test` function.

…ntent Prep work for upcoming support of receiving notification-opened Intents without relying on `package:flutter_local_notifications`.

Fixes: zulip#351

rajveermalviya · 2024-10-15T23:27:31Z

Apologies for the delay in the revision. Thanks for the previous review @chrisbobbe, pushed a new revision, PTAL.

chrisbobbe

Thanks! Small comments below from a very light review; Greg knows this system better than I do, and I'll go ahead and mark for his review to save some time. 🙂

chrisbobbe · 2024-10-17T23:44:55Z

android/app/src/main/kotlin/com/zulip/flutter/Notifications.g.kt

@@ -87,6 +87,36 @@ data class NotificationChannel (
  }
 }

+/**
+ * Corresponds to `android.content.Intent`


notif [nfc]: Replace PendingIntent.intentPayload with PendingIntent.intent

Hmm, tools/check pigeon fails for me at this commit; do you reproduce?

Running pigeon... Error: there were changes to pigeons: M android/app/src/main/kotlin/com/zulip/flutter/Notifications.g.kt M lib/host/android_notifications.g.dart FAILED: pigeon

But the commit after it passes. So maybe some generated code was committed in that later commit but should have been committed here.

chrisbobbe · 2024-10-18T00:08:04Z

test/notifications/display_test.dart

-        expectedTitle: expectedTitle,
-        expectedTagComponent: expectedTagComponent);
-      testBinding.androidNotificationHost.clearActiveNotifications();
+    void testNotification(String description, Future<void> Function(Future<void> Function(FcmMessage data) receiveFcmMessage) body, {http.Client Function()? httpClientFactory}) {


notif test: Use testNotification to test foreground and background FCM messages

It looks like this commit is adding a new helper named testNotification. Could you reword the summary line to be clearer about that? At first this looked like it might be about adding callers of an existing testNotification function.

gnprice · 2024-10-18T04:27:49Z

lib/notifications/display.dart

+    final GlobalStore globalStore;
+    if (isBackground) {
+      _backgroundIsolateGlobalStore ??= await ZulipBinding.instance.loadGlobalStore();
+      globalStore = _backgroundIsolateGlobalStore!;
+    } else {
+      NavigatorState navigator = await ZulipApp.navigator;
+      final context = navigator.context;
+      assert(context.mounted);
+      if (!context.mounted) return; // TODO(linter): this is impossible as there's no actual async gap, but the use_build_context_synchronously lint doesn't see that
+      globalStore = GlobalStoreWidget.of(context);
+    }


Hmm, it feels like the wrong structure for this NotificationDisplayManager class to be maintaining its own GlobalStore instance (in _backgroundIsolateGlobalStore).

Having this method, and its caller chain, need to know about whether it's running in a background isolate or in the foreground seems messy too.

I've just sent #1005 to solve this a different way. Then this becomes:

Suggested change

final GlobalStore globalStore;

if (isBackground) {

_backgroundIsolateGlobalStore ??= await ZulipBinding.instance.loadGlobalStore();

globalStore = _backgroundIsolateGlobalStore!;

} else {

NavigatorState navigator = await ZulipApp.navigator;

final context = navigator.context;

assert(context.mounted);

if (!context.mounted) return; // TODO(linter): this is impossible as there's no actual async gap, but the use_build_context_synchronously lint doesn't see that

globalStore = GlobalStoreWidget.of(context);

}

final globalStore = await ZulipBinding.instance.getGlobalStore();

gnprice · 2024-10-18T04:29:23Z

lib/notifications/display.dart

+    required MessageFcmMessage data,
+  }) async {
+    final account = globalStore.accounts.firstWhereOrNull((account) =>
+      account.realmUrl.origin == data.realmUri.origin && account.userId == data.userId);


notif: Match realmUri using only the origin part of the uri

Can you elaborate on the motivation for this change? What's the situation where it makes a difference, and why is the new behavior better than the old behavior in that situation?

gnprice

Thanks @rajveermalviya for taking care of this, and thanks @chrisbobbe for the previous reviews!

For tonight I've just skimmed this; a couple of comments above.

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

notif: Match realmUri using only the origin part of the uri

27104ad

rajveermalviya added 2 commits October 16, 2024 02:39

notif test: Use testNotification to test foreground and background FC…

f6f3795

…M messages Prep commit for upcoming implementation where for foreground notifications it'd depend on GlobalStore inherited widget and thus would require the use of `testWidget` instead of `test` function.

notif [nfc]: Replace PendingIntent.intentPayload with PendingIntent.i…

37a6fcd

…ntent Prep work for upcoming support of receiving notification-opened Intents without relying on `package:flutter_local_notifications`.

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

rajveermalviya added 4 commits October 16, 2024 04:44

notif: Handle opening of notification via URI intents

0226918

notif: Remove unused code for package:flutter_local_notifications

10adc7e

deps: Remove flutter_local_notifications

a419837

Fixes: zulip#351

android build: Remove use of core-library desugaring

01463f3

rajveermalviya force-pushed the pr-drop-fln branch from 0cf3d46 to 01463f3 Compare October 15, 2024 23:14

rajveermalviya requested a review from chrisbobbe October 15, 2024 23:27

This comment was marked as resolved.

Sign in to view

chrisbobbe reviewed Oct 18, 2024

View reviewed changes

chrisbobbe assigned gnprice Oct 18, 2024

chrisbobbe requested a review from gnprice October 18, 2024 01:57

chrisbobbe added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Oct 18, 2024

gnprice mentioned this pull request Oct 18, 2024

store [nfc]: Offer singleton global store on binding #1005

Open

gnprice reviewed Oct 18, 2024

View reviewed changes

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

rajveermalviya commented Oct 15, 2024

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

chrisbobbe left a comment •

edited

Loading

chrisbobbe Oct 17, 2024

chrisbobbe Oct 18, 2024

gnprice Oct 18, 2024

gnprice Oct 18, 2024

gnprice left a comment

		@@ -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

Replace flutter_local_notifications with pigeon bindings #856

Are you sure you want to change the base?

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

rajveermalviya commented Oct 15, 2024

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

chrisbobbe left a comment • edited Loading

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

gnprice left a comment

Choose a reason for hiding this comment

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

chrisbobbe left a comment •

edited

Loading