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

Added Android 8 Support for Sound and Vibrate properties. #1696

Merged
merged 1 commit into from
Mar 19, 2023

Conversation

timkellypa
Copy link
Contributor

@timkellypa timkellypa commented Oct 2, 2018

Created ability to have multiple channels
generated dynamically for vibrate/sound configurations.

Created ability to have multiple channels
generated dynamically for vibrate/sound configurations.
* removed extra changes caused by creating a new branch *
@sanctus671
Copy link

sanctus671 commented Oct 13, 2018

@timkellypa Thanks for this! But if I set different sounds for each notification, only the first sound that was set plays. Is it something to do with the channel being created only once at the beginning? Any way to fix?

Edit:
I found the issue. It was in the getChannel() method in Options.java. Basically the way it was, it was only creating a new channel if no channel value was provided in the options. So I removed this:

        if (!options.optString("channel").isEmpty() || SDK_INT < O) {
            return options.optString("channel", DEFAULT_CHANNEL_ID);
        }

And added this:

        if (!options.optString("channel").isEmpty() || SDK_INT < O) {
            String channelId = options.optString("channel", DEFAULT_CHANNEL_ID);
            Manager.getInstance(context).createChannel(channelId, channelName != null ? channelName : "default-channel-name",
                4, shouldVibrate, soundUri);     
            
            return channelId;
}
            

Just to make sure the channel is created. See my repo that I cloned from you and made the changes on: https://github.com/sanctus671/cordova-plugin-local-notifications. Now you can create different channels to play different sounds by making sure the channel property is set when you create the notification.

@timkellypa
Copy link
Contributor Author

timkellypa commented Oct 14, 2018

@sanctus671 Yes, I was missing that case. I may end up just changing it to optionally pass in the channel ID (like it does the channel name) to createChannelWithOptions (that way everything filters through one function).

But yeah, basically making new channels is the only way to go for this to work. Once you create one, it can't be updated. Glad you got it working!

@TLMNicolas
Copy link

Trying the code in https://github.com/sanctus671/cordova-plugin-local-notifications (also tried the the code in the beggining) but the sound still doesnt play for me.
Im trying to play a .wav

Wondering if anything changed in the setup of the notification for Android 8 in this plugin ?

@sanctus671
Copy link

@TLMNicolas I think it needs to be .mp3 for Android. Try testing with an mp3 file and see if that works. If not, can you paste the code you are using the schedule the notification? I'll see if anything sticks out to me as being a problem.

@timkellypa
Copy link
Contributor Author

@TLMNicolas , @sanctus671 is correct. It needs to be an MP3 to work. If you have a wav file, you can just download LAME to convert it easily.

@TLMNicolas
Copy link

TLMNicolas commented Oct 30, 2018

Appreciate the help. I traditionally have used .wav for notifications because it worked on ios and android so it meant i only had to keep 1 file instead of multiple, and deal with naming and such.

Tried using an mp3. Placed it in two places,

  1. The res folder, app/src/main/res/raw/rooster.mp3
  2. The www/assets folder, assets/sounds/rooster.mp3

Then in the code i tried multiple variations
res://rooster.mp3
res://rooster
file://rooster.mp3
file://assets/sounds/rooster.mp3

In adb/logcat i see something like this (when using res://rooster.mp3)
showForNotification : isInteractive=true, isHeadUp=false, color=0, sbn = StatusBarNotification(pkg=com.XXX.YYYY user=UserHandle{0} id=1 tag=null key=0|com.XXX.YYY|1|null|10366: Notification(channel=sound-channel-id pri=1 contentView=null vibrate=null sound=android.resource://com.XXX.YYY/raw/rooster tick defaults=0x4 flags=0x11 color=0x00000000 number=0 vis=PUBLIC semFlags=0x0 semPriority=0 semMissedCount=0))

No luck, still plays default sound. (On ios its fine with res://rooster.wav)

Edit:
Let me know if this is not the place for this and I will create a new issue. Dont want to clutter this isse.

@timkellypa
Copy link
Contributor Author

timkellypa commented Oct 30, 2018

(edited after re-reading your post, since I see you answered my question about assets)

@TLMNicolas I don't mind continuing this conversation in this thread.

For my application, I've only used "file://". You want the path from "file" to start at the root of the actual JavaScript code in the "www" directory in the cordova project. So file://assets/sounds/rooster.mp3 was correct.

Another thing to note, and anybody using this plugin should know this: When an Android 8 notification channel is created, it cannot be modified by the app. So, if you are installing over your existing app without completely uninstalling it first, it will continue playing the default sound, even if you fix the configuration, add new alarms, etc.

@talal-alkaremie
Copy link

I appreciate your effort @timkellypa and @sanctus671
@sanctus671 I tried your code and it works perfectly except it plays the first sounds only. I tested your code on Huawei Nova 3e running Android 8

@timkellypa
Copy link
Contributor Author

@sanctus671 Just for an update, I addressed your issue for multiple channel support (now you can pass a channelId, and it'll use that when it creates your channel, instead of the one it generates), but unfortunately the tip of development for me is part of a new pull request, with another feature I needed for my app (auto launch):
#1714

This pull request contains all of the code in this branch, and extends it for auto-launch functionality (which is optional and not a default, so you should be able to use it without issue).

I apologize for not keeping my code more organized and keeping these pull requests separated as I should have. Everything's kind of intertwined, and my particular use case has some very specific requirements.

@aleixfargas
Copy link

Looks great!
What is the ETA to merge this PR into master?
I'm very interested on not depending on a fork on our project, so the master branch does not work well on latest android versions (as the following issues demonstrate us: #1589 , #1843 , #1855 )

@GitToTheHub GitToTheHub merged commit 9c61255 into katzer:master Mar 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants