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

Replace flutter_local_notifications with a thin API binding using Pigeon #351

Open
gnprice opened this issue Oct 31, 2023 · 5 comments
Open

Comments

@gnprice
Copy link
Member

gnprice commented Oct 31, 2023

For our initial implementation of notification UI #122 (introduced in #344), we use the package flutter_local_notifications. That's because it's what's currently recommended by Flutter upstream for showing a notification, and because I looked around a bit a few months ago and concluded that it was the best existing option for that.

As discussed at #344 (comment) and in chat, though, that package has quite a bit of baggage for functionality we'll never need, like timers. It also goes for an unfortunate pattern — reminiscent of React Native and its ecosystem, in fact — of providing a bunch of logic of its own, so that the API it presents is different from that of the underlying platform and adds its own quirks, bugs, and lack of documentation. For example, the issue I filed which is discussed at #344 (comment) .

So I don't find this package very satisfying, and I think it's likely that we'll ultimately want to rip it out in favor of something we write ourselves. The plan would be:

  • Expose the platform APIs as thinly and faithfully as possible. Let the logic of deciding how to use those APIs stay in Dart.
  • Use Pigeon to make the writing of those API bindings as easy and automated as possible.
@gnprice
Copy link
Member Author

gnprice commented Mar 22, 2024

As described at #341 (comment) , I started trying Pigeon today and wrote a draft branch that does this for the method to show a notification, as well as a couple of other methods we don't yet use in main. So I'll plan to clean that up and send it.

I might go ahead and cover the rest of the library (since what I already have is the most complex of the methods we use), or might leave that for later.

@rajveermalviya
Copy link
Collaborator

(Just another data point of why it would be better to have our custom platform integration for some APIs)

Yesterday I experimented with #128 and my draft branch uses flutter_local_notification because it provides all the functionality that is needed to implement #128.

But while implementing it, I thought it would also be great to have the notifications be in the "Conversations" space of the notification panel (available since Android 11), including with the new notification UI that "Conversations" space provides. To implement that we would first need to create dynamic "Conversation Shortcuts" (could be per Realm or per Stream) using ShortcutManagerCompat and then pass the shortcut-id when creating notification using setShortcutId, but flutter_local_notification doesn't yet provide this ability of setShortcutId.

You can read more about Conversation Shortcuts & Conversation Notifications here.

There are these two packages I found that provide a wrapper around ShortcutManagerCompat to manage conversation shortcuts: android_conversation_shortcut & flutter_shortcuts. But they aren't maintained anymore, so including the NotificationCompat we would also need custom platform integration for ShortcutManagerCompat.

@gnprice
Copy link
Member Author

gnprice commented Mar 22, 2024

Yesterday I experimented with #128 and my draft branch uses flutter_local_notification because it provides all the functionality that is needed to implement #128.

Interesting, thanks! I'd be very glad to see that as a PR 😄

(On a quick read-through, the one significant thing it looks like it needs is tests.)

But while implementing it, I thought it would also be great to have the notifications be in the "Conversations" space of the notification panel (available since Android 11), including with the new notification UI that "Conversations" space provides. […] You can read more about Conversation Shortcuts & Conversation Notifications here.

Hmm, yeah, that would be neat. And I agree, the way to do that will probably be via whipping up the platform integration ourselves.

@rajveermalviya
Copy link
Collaborator

I'd be very glad to see that as a PR

Sure, I plan to send it after your PR for replacing flutter_local_notification lands, to avoid re-doing that work on main.

Conversation Notification

Created a discussion for it on here

gnprice added a commit to gnprice/zulip-flutter that referenced this issue Mar 26, 2024
This delivers the first major tranche of zulip#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: zulip#351
@gnprice
Copy link
Member Author

gnprice commented Mar 26, 2024

OK, sent #592 for that conversion to Pigeon.

That PR doesn't completely remove package:flutter_local_notifications — but it replaces our use of its show method, which is by far our most complicated call site. It also sets up a pattern within which I think it should be fairly straightforward to add more methods in our new Pigeon-based plugin (at least where the underlying API is simple, anyway).

So I think that's ripe for you to experiment with rebasing your draft branch atop #592 and using the new mechanism: use androidNotificationHost.notify instead of notifications.show, add to that method any missing functionality you need, and also add to the new plugin any other methods you need. (Like getActiveNotifications.)

If you find rough edges in that flow or any questions come up, please comment on the PR or in a thread in #mobile-team. (For any back-and-forth discussion, probably a chat thread is best.)

gnprice added a commit to gnprice/zulip-flutter that referenced this issue Mar 26, 2024
This delivers the first major tranche of zulip#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: zulip#351
gnprice added a commit to gnprice/zulip-flutter that referenced this issue Mar 28, 2024
This delivers the first major tranche of zulip#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: zulip#351
@gnprice gnprice modified the milestones: Launch, B2: Summer 2024 May 9, 2024
@gnprice gnprice removed their assignment May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

No branches or pull requests

2 participants