search

MaikuB / flutter_local_notifications

A Flutter plugin for displaying local notifications on Android, iOS, macOS and Linux
2.43k stars 1.38k forks source link

Add way to use existing android notification channel. #1527

Open Turburlar opened 2 years ago

Turburlar commented 2 years ago

My app has already created an Android notification channel in kotlin for use by firebase messaging.

When specifying a channel in AndroidNotificationDetails, you must specify an channelId and a channelName. This is redundant since I have already defined the channel in kotlin. Forcing the channel to be redundantly defined is a violation of the single source of truth principle.

It would be nice if you could specify a parameter 'existingChannelId' which means, don't create a new channel, or redefine any of its properties, but rather just send the notification to that channelId.

MaikuB commented 2 years ago

Thanks for the feedback. This is an area I thought about before after the plugin added methods that can be used to manage notification channels. Whilst the ability to specify an existingChannelId of sorts could be a solution and remove the duplication for your scenario, IMO it makes it awkward that the API has two properties for specifying the notification channel. Having one and making it mandatory would be better compared to have both and checking that one of them is specified. Given that most details of a notification channel cannot be updated after it's created, what's provided already

Putting your scenario aside, some apps may want to create the channel ahead of time and then specify the channel that the notification should be associated with. In this case, they'll have no choice but to duplicate some values as certain aspects like sound, vibration etc are tied with notification channels on Android 8.0+ but on older versions would directly associated with a notification. Then there are those that want the channel to be created just as the notification is about to be shown. With both of these in mind, the approach taken is likely the least worst approach for most users of the plugin. If you have other suggestions/feedback on this then do share but potentially this is where you need to look at fork for your own scenario.

Turburlar commented 2 years ago

The proper solution would be to have channel creation completely separated from notifcation sends.....not sent on every notification...as this really makes no sense. Individual notification sends should just take a channel ID, and expect it to already exist. Then, through a function provided by this library, or through the users own Kotlin code, the channel can be created. This cleanly solves all the use cases and corrects the awkward implementation that currently exists.

The current implementation IMO, is the worst possible option of all of them. It just does not play well at all with libraries like FCM.

MaikuB commented 2 years ago

The proper solution would be to have channel creation completely separated from notifcation sends.....not sent on every notification...as this really makes no sense. Individual notification sends should just take a channel ID, and expect it to already exist. Then, through a function provided by this library, or through the users own Kotlin code, the channel can be created. This cleanly solves all the use cases and corrects the awkward implementation that currently exists.

Can you submit a draft PR to show how it would so then? I agree that expecting the channel to exists make sense but that's only when you consider it for Android 8.0+. From what your reply I don't see how such an approach

  1. allow channels to be created just before a notification is shown including if it's one that is scheduled
  2. deals with properties being that are duplicated across a notification and notification channel given how the Android notifications API has evolved and this plugin needs to cater for older Android versions

Seeing a draft PR would help make this clearer in case I'm missing something and there's a 10.0.0 branch that could be used as a target. Note that point 1 is the main reason why the plugin API is the way it is at the moment where channel information is included with every notification. I couldn't think of another approach to deal with this as the current approach would allow delay the channel creation before the first notification. This in turn, prevents having the channel appear in the app's settings until that point in time. Or are you are suggesting to stick with decoupling and not support this scenario anymore and require consumers of the plugin to, as an example, specify the custom sound at a notification and notification channel level to support all versions of Android? Certainly possible but IMO should get feedback from the community first. The plugin's API has been like this for quite a while and I believe you're the first to provide such feedback on the API, which I acknowledge but have shared reasons why it's the way it is.

It just does not play well at all with libraries like FCM.

Besides the issue you mentioned, do you mean there are other problems? If so, what are they? What you said would imply the plugin doesn't work with FCM but from what I've seen and heard it does work. Furthermore, I've the docs for the Flutter plugin also have a section on how to create the notification channel with this plugin so that it could be specified to be used when sending a push notification via FCM

MaikuB commented 2 years ago

FYI I've opened a thread to get feedback on the community https://github.com/MaikuB/flutter_local_notifications/issues/1528