notif: Replace flutter_local_notification `createNotificationChannel` with pigeon #795

rajveermalviya · 2024-07-08T15:51:36Z

Work towards #351

sm-sayedi · 2024-07-10T18:39:55Z

I will leave my review in the next few hours.

sm-sayedi

Thanks @rajveermalviya for this! Just a small comment below, otherwise LGTM!

Let's move forward to the mentor review with @sumanthvrao!

sm-sayedi · 2024-07-11T22:34:49Z

pigeon/notifications.dart

+  /// Corresponds to `androidx.core.app.NotificationManagerCompat.createNotificationChannel`.
+  ///
+  /// See: https://developer.android.com/reference/androidx/core/app/NotificationManagerCompat#createNotificationChannel(androidx.core.app.NotificationChannelCompat)
+  void createNotificationChannel(NotificationChannel channel);


I am not sure if this could return Future<void> instead of void in here too, as the autogenerated concrete class has a return type of Future<void>. Looking at the docs from the link provided, it seems that this method has a return type of void, but that's in Java, and it doesn't have a type Future; at least that's how I know so far about Java. 😀

This file is a Pigeon file, not a standard Dart file. It contains types and methods that define the native plugin API interface. See Pigeon's example.

Adding a Future<void> here will result in an error:

Error: pigeon/notifications.dart:120: Unknown type: Future

Ok thanks, got it!

kenclary

this looks fine to me, but I have basically no familiarity with pigeon.

chrisbobbe

Thanks! Small comments below, and now over to Greg for his review.

chrisbobbe · 2024-07-17T21:42:02Z

pigeon/notifications.dart

@@ -94,8 +94,39 @@ class MessagingStyle {
  final bool isGroupConversation;
 }

+/// Corresponds to `androidx.core.app.NotificationChannelCompat`
+///
+/// See: https://developer.androicd.com/reference/androidx/core/app/NotificationChannelCompat


nit: developer.android.com

chrisbobbe · 2024-07-17T22:39:23Z

lib/host/android_notifications.dart

+///
+/// See: https://developer.android.com/reference/android/app/NotificationChannel#setImportance(int)
+abstract class NotificationImportance {
+  /// Corresponds to [`IMPORTANCE_UNSPECIFIED`](https://developer.android.com/reference/androidx/core/app/NotificationManagerCompat#IMPORTANCE_UNSPECIFIED())


The [text](example.com) syntax makes it a bit harder to work out what the intended URL is and select it, especially here where the intended URL ends with ")".

I did notice Android Studio helpfully trying to linkify this in a hover interaction:

but then it similarly gets confused about what the intended URL is; it takes me here: https://developer.android.com/reference/androidx/core/app/NotificationManagerCompat#IMPORTANCE_UNSPECIFIED(

when it should have taken me here: https://developer.android.com/reference/androidx/core/app/NotificationManagerCompat#IMPORTANCE_UNSPECIFIED()

so the browser just leaves me at the top of the page, instead of scrolling me down to the #IMPORTANCE_UNSPECIFIED() element.

So I think probably most helpful to give up that syntax and just write some thing like:

/// Corresponds to `IMPORTANCE_UNSPECIFIED`: /// https://developer.android.com/reference/androidx/core/app/NotificationManagerCompat#IMPORTANCE_UNSPECIFIED()

(here and elsewhere in the PR)

rajveermalviya · 2024-07-18T14:04:03Z

Thanks for the review @chrisbobbe, pushed a new revision PTAL :)

gnprice

Thanks @rajveermalviya, and thanks @sm-sayedi and @chrisbobbe for the previous reviews!

Generally this looks great — a few nits below.

gnprice · 2024-07-19T20:31:53Z

pigeon/notifications.dart

+///
+/// See: https://developer.android.com/reference/androidx/core/app/NotificationChannelCompat
+class NotificationChannel {
+  /// Corresponds to [`NotificationChannelCompat.Builder`](https://developer.android.com/reference/androidx/core/app/NotificationChannelCompat.Builder#Builder(java.lang.String,int))


nit: one remaining instance of the pattern @chrisbobbe pointed out above

Separately, the reference to the builder class is good but I think it's most accurate to link to the class as a whole rather than that constructor. Three of the five parameters here don't go to the builder's constructor, but rather to method calls on the builder.

gnprice · 2024-07-19T20:36:27Z

lib/host/android_notifications.dart

+
+/// For use in [NotificationChannel.importance].
+///
+/// See: https://developer.android.com/reference/android/app/NotificationChannel#setImportance(int)


Suggested change

/// See: https://developer.android.com/reference/android/app/NotificationChannel#setImportance(int)

/// See:

/// https://developer.android.com/reference/android/app/NotificationChannel#setImportance(int)

/// https://developer.android.com/reference/android/app/NotificationChannel#getImportance()

The getter's doc has some additional information that isn't on the setter, and they don't link to each other, so best to provide both.

gnprice · 2024-07-19T20:43:24Z

pigeon/notifications.dart

+/// Corresponds to `androidx.core.app.NotificationChannelCompat`
+///
+/// See: https://developer.android.com/reference/androidx/core/app/NotificationChannelCompat
+class NotificationChannel {


nit: put this at the top of the file, before PendingIntent — that makes a nice logical lifecycle-oriented order

The methods on AndroidNotificationHostApi are already good this way.

The enum class NotificationImportance should get the same treatment, coming before PendingIntentFlag.

gnprice · 2024-07-19T20:48:41Z

test/model/binding.dart

 class FakeAndroidFlutterLocalNotificationsPlugin extends Fake implements AndroidFlutterLocalNotificationsPlugin {
+}
+
+class FakeIOSFlutterLocalNotificationsPlugin extends Fake implements IOSFlutterLocalNotificationsPlugin {
+}


Neat!

Does this mean we can also cut out the test binding's resolvePlatformSpecificImplementation? That'd be a nice further cleanup commit to come after this one.

gnprice · 2024-07-19T20:50:48Z

test/model/binding.dart

@@ -513,6 +509,11 @@ class FakeAndroidNotificationHostApi implements AndroidNotificationHostApi {
    _activeNotificationsMessagingStyle.clear();
  }

+  @override
+  Future<void> createNotificationChannel(NotificationChannel channel) async {


nit: keep this next to the data it acts on (so _createdChannels)

Relatedly, in the first commit this should go above takeNotifyCalls, so as not to separate that from notify.

rajveermalviya · 2024-07-25T23:10:42Z

Thanks for the review @gnprice, pushed a new revision.

gnprice

Thanks for the revision! Two nits, and otherwise this looks great.

gnprice · 2024-07-26T21:49:51Z

pigeon/notifications.dart

+/// See:
+///   https://developer.android.com/reference/androidx/core/app/NotificationChannelCompat
+///   https://developer.android.com/reference/androidx/core/app/NotificationChannelCompat.Builder#Builder(java.lang.String,int)
+class NotificationChannel {
+  NotificationChannel({


Ah, just to clarify what I meant at #795 (comment) because it might have been confusing:

I think having a doc comment on this Pigeon/Dart constructor is fine and potentially helpful. What I meant was that the doc on this constructor should link to the doc of the upstream builder class as a whole, and not to the specific point on that page for the builder's constructor.

So like this:

Suggested change

/// See:

/// https://developer.android.com/reference/androidx/core/app/NotificationChannelCompat

/// https://developer.android.com/reference/androidx/core/app/NotificationChannelCompat.Builder#Builder(java.lang.String,int)

class NotificationChannel {

NotificationChannel({

/// See:

/// https://developer.android.com/reference/androidx/core/app/NotificationChannelCompat

/// https://developer.android.com/reference/androidx/core/app/NotificationChannelCompat.Builder

class NotificationChannel {

NotificationChannel({

but also that second link would be fine to move back to being a doc comment on this NotificationChannel constructor rather than on the NotificationChannel class.

gnprice · 2024-07-26T21:51:41Z

test/model/binding.dart

-  FlutterLocalNotificationsPlatform? _platform;
-
-  @override
-  T? resolvePlatformSpecificImplementation<T extends FlutterLocalNotificationsPlatform>() {


commit-structure nit: this should come as a separate commit after the main commit, as per #795 (comment)

rajveermalviya · 2024-07-28T08:57:22Z

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

See: https://developer.android.com/reference/androidx/core/app/NotificationManagerCompat#createNotificationChannel(androidx.core.app.NotificationChannelCompat)

gnprice

Thanks for the revision! Looks good — merging, after fixing two quick nits mentioned below.

This is great progress on #351; looking forward to seeing the remaining pieces.

gnprice · 2024-07-28T21:20:05Z

test/model/binding.dart

 class FakeAndroidFlutterLocalNotificationsPlugin extends Fake implements AndroidFlutterLocalNotificationsPlugin {
+}
+
+class FakeIOSFlutterLocalNotificationsPlugin extends Fake implements IOSFlutterLocalNotificationsPlugin {


nit: these can be cut too, since they're effectively part of the implementation of resolvePlatformSpecificImplementation

(the previous revision did cut them, so I'm guessing their coming back was a rebase error)

gnprice · 2024-07-28T21:21:45Z

test/model/binding.dart

-  FlutterLocalNotificationsPlatform? _platform;
-
-  @override
-  T? resolvePlatformSpecificImplementation<T extends FlutterLocalNotificationsPlatform>() {


nit in commit message:

notif [nfc]: Remove unused test harness for flutter_local_notification

This sounds like it might mean our whole test harness for that package. Also the package's name has an "s" at the end. So:

notif [nfc]: Remove unused test harness for parts of flutter_local_notifications

gnprice · 2024-07-28T21:29:52Z

Oh and a different commit-message nit — the commit series looks like this:
1f641b7 notif: Add createNotificationChannel method to pigeon bindings
2af238b notif: Add createNotificationChannel method to pigeon bindings
b35b73c28 notif [nfc]: Remove unused test harness for parts of flutter_local_notifications

So the first commit message is duplicated.

I think you intended the second commit to have the message it had in the last revision:

notif: Replace flutter_local_notification `createNotificationChannel` with pigeon

Updates #351

So I'll put that message back.

This is probably also a good moment for a reminder that I highly recommend setting up and using a graphical Git client, like gitk. Especially when doing a rebase, or operations like splitting up commits, a graphical client can be extremely helpful for getting a clear picture of what the old and new branch look like. I think if you'd used gitk or similar when self-reviewing your revision, you'd probably have immediately spotted that the commit message on the second commit had gotten clobbered.

… with pigeon Updates zulip#351

…tifications

rajveermalviya requested a review from sm-sayedi July 9, 2024 06:23

rajveermalviya added the buddy review GSoC buddy review needed. label Jul 9, 2024

sm-sayedi reviewed Jul 11, 2024

View reviewed changes

sm-sayedi added mentor review GSoC mentor review needed. and removed buddy review GSoC buddy review needed. labels Jul 11, 2024

sm-sayedi requested a review from sumanthvrao July 11, 2024 22:37

rajveermalviya requested a review from kenclary July 12, 2024 08:40

kenclary approved these changes Jul 13, 2024

View reviewed changes

rajveermalviya requested a review from chrisbobbe July 13, 2024 05:07

rajveermalviya added maintainer review PR ready for review by Zulip maintainers and removed mentor review GSoC mentor review needed. labels Jul 13, 2024

rajveermalviya force-pushed the pr-pigeon-create-channel branch from 2ef9e12 to 60d84a0 Compare July 15, 2024 22:31

chrisbobbe reviewed Jul 17, 2024

View reviewed changes

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 Jul 17, 2024

rajveermalviya force-pushed the pr-pigeon-create-channel branch from 60d84a0 to e37e269 Compare July 18, 2024 14:03

rajveermalviya requested a review from chrisbobbe July 18, 2024 14:04

rajveermalviya force-pushed the pr-pigeon-create-channel branch 2 times, most recently from ab32f24 to 01adf07 Compare July 19, 2024 06:09

gnprice reviewed Jul 19, 2024

View reviewed changes

rajveermalviya force-pushed the pr-pigeon-create-channel branch 2 times, most recently from bfe1433 to 59d248d Compare July 25, 2024 23:10

rajveermalviya requested a review from gnprice July 25, 2024 23:11

gnprice reviewed Jul 26, 2024

View reviewed changes

rajveermalviya force-pushed the pr-pigeon-create-channel branch from 59d248d to 107810f Compare July 28, 2024 08:55

rajveermalviya requested a review from gnprice July 28, 2024 08:57

notif: Add createNotificationChannel method to pigeon bindings

1f641b7

See: https://developer.android.com/reference/androidx/core/app/NotificationManagerCompat#createNotificationChannel(androidx.core.app.NotificationChannelCompat)

rajveermalviya force-pushed the pr-pigeon-create-channel branch 2 times, most recently from de6271f to b36b8da Compare July 28, 2024 19:28

gnprice reviewed Jul 28, 2024

View reviewed changes

rajveermalviya added 2 commits July 28, 2024 14:30

notif: Replace flutter_local_notification createNotificationChannel…

6d69b16

… with pigeon Updates zulip#351

notif [nfc]: Remove unused test harness for parts of flutter_local_no…

016f22a

…tifications

gnprice force-pushed the pr-pigeon-create-channel branch from b36b8da to 016f22a Compare July 28, 2024 21:35

gnprice merged commit 016f22a into zulip:main Jul 28, 2024
1 check passed

rajveermalviya deleted the pr-pigeon-create-channel branch July 28, 2024 22:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

notif: Replace flutter_local_notification `createNotificationChannel` with pigeon #795

notif: Replace flutter_local_notification `createNotificationChannel` with pigeon #795

rajveermalviya commented Jul 8, 2024

sm-sayedi commented Jul 10, 2024 •

edited

Loading

sm-sayedi left a comment

sm-sayedi Jul 11, 2024

rajveermalviya Jul 12, 2024

sm-sayedi Jul 12, 2024

kenclary left a comment

chrisbobbe left a comment

chrisbobbe Jul 17, 2024

chrisbobbe Jul 17, 2024

rajveermalviya commented Jul 18, 2024

gnprice left a comment

gnprice Jul 19, 2024

gnprice Jul 19, 2024

gnprice Jul 19, 2024

gnprice Jul 19, 2024

gnprice Jul 19, 2024

gnprice Jul 19, 2024

gnprice Jul 19, 2024

rajveermalviya commented Jul 25, 2024

gnprice left a comment

gnprice Jul 26, 2024

gnprice Jul 26, 2024

rajveermalviya commented Jul 28, 2024

gnprice left a comment

gnprice Jul 28, 2024

gnprice Jul 28, 2024

gnprice commented Jul 28, 2024

notif: Replace flutter_local_notification createNotificationChannel with pigeon #795

notif: Replace flutter_local_notification createNotificationChannel with pigeon #795

Conversation

rajveermalviya commented Jul 8, 2024

sm-sayedi commented Jul 10, 2024 • edited Loading

sm-sayedi 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

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

rajveermalviya commented Jul 18, 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

rajveermalviya commented Jul 25, 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 Jul 28, 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

gnprice commented Jul 28, 2024

notif: Replace flutter_local_notification `createNotificationChannel` with pigeon #795

notif: Replace flutter_local_notification `createNotificationChannel` with pigeon #795

sm-sayedi commented Jul 10, 2024 •

edited

Loading