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

[android] Change request: Don't recreate TSLocationManager channel if exists #808

Closed
bengtan opened this issue Aug 21, 2019 · 18 comments
Closed

Comments

@bengtan
Copy link

bengtan commented Aug 21, 2019

Your Environment

  • Plugin version: 3.0.9
  • Platform: android
  • OS version: 8.0 and 9.0
  • Device manufacturer / model: Various
  • React Native version (react-native -v): 0.59.2
  • Plugin config: N/A

Context

On Android 8.0+, our app has an Android notification channel named TSLocationManager. This channel is created by the plugin.

We'd like to be able to rename/configure this channel.

I added code to Android's mainActivity.onCreate() to create a notification channel with the same id (and our desired configuration) and this successfully configured the channel.

However, a few app starts later, it had changed back to the name/configuration that the plugin sets.

I presume the plugin is repeatedly notificationManager.createNotificationChannel()-ing it?.

I'd like to request the following change please:

Instead of recreating the channel unconditionally, please change the plugin to check whether the channel exists. If it exists, don't recreate it.

@christocracy
Copy link
Member

I can do that

@bengtan
Copy link
Author

bengtan commented Aug 21, 2019

That would be great. Thank you very much.

@christocracy
Copy link
Member

Show me the Config you’re using with your channel.

@bengtan
Copy link
Author

bengtan commented Aug 21, 2019

Oh, I don't have that code handy anymore. I deleted it since it wasn't working due to the plugin overwriting it.

But I wanted to give the channel a different name. Maybe also add a description?

@bengtan
Copy link
Author

bengtan commented Aug 21, 2019

As in ... all I was doing was create a channel object with our desired configuration, and then calling createNotificationChannel(). The code was called from the main activity's onCreate() function so it was being called at every app startup.

@christocracy
Copy link
Member

See the docs for Config.notification, as well as the interface Notification. You can configure the name.

@bengtan
Copy link
Author

bengtan commented Aug 21, 2019

How about this ... I'll reconstruct my code and then post it to you. Then you can reproduce what I saw. But gimme a day or two to get around to it.

@christocracy
Copy link
Member

I’m not surprised the plugin would override your channel. I’m just interested in the specific channel options You were interested in, there are many.

If you’re only concerned with the name, you can easily configure that with the plugin’s Notification config.

@bengtan
Copy link
Author

bengtan commented Aug 22, 2019

Oh, I didn't even see the Config.notification option.

Since that exists, it makes sense to use/extend it rather than have a separate thing.

I'd like to configure the channel name and importance. (Maybe channel description in the future, but it's in the future so I don't want to make a fuss about it now.)

The name can be configured via Config.notification.channelName. That's covered.

Re: The channel importance ... I think you're setting it to IMPORTANCE_LOW? I'd like to configure it to IMPORTANCE_MIN.

Ideas:

  1. Add a new Config.notification.importance field and set channel importance to Config.notification.importance, or
  2. Re-use/map Config.notification.priority and set the channel importance to Config.notification.priority + 3.

@christocracy
Copy link
Member

christocracy commented Aug 22, 2019

Show me the docs links that prove Config.notification.priority + 3 = IMPORTANCE_*

@bengtan
Copy link
Author

bengtan commented Aug 22, 2019

It's not a proof. It's just a shortcut instead of creating a map/table lookup.

https://developer.android.com/reference/androidx/core/app/NotificationCompat.html#PRIORITY_DEFAULT

PRIORITY_MIN = -2
PRIORITY_LOW = -1
PRIORITY_DEFAULT = 0
PRIORITY_HIGH = 1
PRIORITY_MAX = 2

https://developer.android.com/reference/android/app/NotificationManager.html#IMPORTANCE_DEFAULT

IMPORTANCE_MIN = 1
IMPORTANCE_LOW = 2
IMPORTANCE_DEFAULT = 3
IMPORTANCE_HIGH = 4
IMPORTANCE_MAX = 5

If you don't want to use '+3', I'm fine with that.

@christocracy
Copy link
Member

christocracy commented Aug 22, 2019

NotificationChannel is highly configurable but implementing just a small handful of crucial ones should suffice for a long time.

My existing Notification class in the library is built to easily scale up. It'll take more time to add the properties to the Typescript interfaces, document them, then sync the Cordova and Flutter. But it's Android-only, so reduced complexity with no iOS involved.

So it'll be Notification.channelImportance. Why don't you develop the Typescript API, including necessary documentation in the same style using Notification.channelName as a template?

I'll re-use the existing const NOTIFICATION_PRIORITY rather than adding confusion of an extra set of BackgroundGeolocation.IMPORTANCE_* and do the +3 math in the native code.

@bengtan
Copy link
Author

bengtan commented Aug 22, 2019

It'll take more time to add the properties to the Typescript interfaces, document them, then sync the Cordova and Flutter. But it's Android-only, so reduced complexity with no iOS involved.

This is a wishlist request, so no rush. It will happen when it happens.

Why don't you develop the Typescript API, including necessary documentation in the same style using Notification.channelName as a template?

Huh? I don't really understand this. You want me to submit a documentation PR?

@christocracy
Copy link
Member

Yes. Here's Notification.channelName. Add your desired channel* attributes (including docs with @example) and submit a PR.

@christocracy
Copy link
Member

And also use Notification.priority as a documentation template, wth the table map fo the constants Value and Description.

@bengtan
Copy link
Author

bengtan commented Sep 3, 2019

See #818

@stale
Copy link

stale bot commented Nov 2, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. You may also mark this issue as a "discussion" and I will leave this open.

@stale stale bot added the stale label Nov 2, 2019
@stale
Copy link

stale bot commented Nov 9, 2019

Closing this issue after a prolonged period of inactivity. Fell free to reopen this issue, if this still affecting you.

@stale stale bot closed this as completed Nov 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants