-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
v0.10.0; multiple Android and iOS fixes #2000
base: master
Are you sure you want to change the base?
Conversation
- Only remove pending alarms when rescheduling. - Call cancel() explicitly to retain old behavior.
- Store base date for repeating notifications that are rescheduled. - On reboot, do not fire high priority notifications that already fired.
... updated 'appShortcutBadgerVersion'
src/android/LocalNotification.java
Outdated
|
||
if (android.os.Build.VERSION.SDK_INT >= android.os.Build.VERSION_CODES.O){ | ||
int importance = NotificationManager.IMPORTANCE_HIGH; | ||
NotificationChannel notificationChannel = new NotificationChannel(NOTIFICATION_CHANNEL_ID, "NOTIFICATION_CHANNEL_NAME", importance); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is creating a Channel with the name "NOTIFICATION_CHANNEL_NAME" in Android >= 8. The channel name should be configurable or should use the same logic as creating a normal notification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Method Options.getChannel
is doing, what we need.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be fixed now, because I've added a new argument for 'options' (merged with plugin default options)
src/android/LocalNotification.java
Outdated
* | ||
* @param command The callback context used when calling back into JavaScript. | ||
*/ | ||
private void dummyNotifications(CallbackContext command) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is just creating a channel and should be renamed to reflect this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it is not optimal, though I'd prefer to merge this first before revisiting the purpose of this function
|
||
mNotificationManager = (NotificationManager) context.getSystemService(Context.NOTIFICATION_SERVICE); | ||
|
||
if (android.os.Build.VERSION.SDK_INT >= android.os.Build.VERSION_CODES.O){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method has only an effect on android >= 8. The code above should be moved in this if-statement, because it has no effect, if the if-statement is not executed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved it inside the if-case
src/android/LocalNotification.java
Outdated
fireEvent("dummyNotifications"); | ||
NotificationManager mNotificationManager; | ||
NotificationCompat.Builder mBuilder; | ||
String NOTIFICATION_CHANNEL_ID = "10004457"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this channel-Id? It should be configurable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It uses 'channeId' now from custom or default options. Only if this is not defined it falls back to the old ID (I have no idea why this ID was chosen by the other contributer).
@fquirin There is a bug in the method "dummyNotifications". It creates a channel "NOTIFICATION_CHANNEL_NAME". The channel name and id is not configurable. |
@fquirin You could add in the notable changes for "fixed gradle" that "compile" is replaced by "implementation" for the notifications plugin and for the badge-plugin by using a fixed fork. |
I merged the android 10 updates from @timkellypa and 2 gradle pr's. It seems everything ok for you. I communicated with the author of this plugin, if he can make a new release of cordova-plugin-badge. So if he did this, we can link this pr to the original plugin back. |
</engines> | ||
|
||
<!-- dependencies --> | ||
<dependency id="cordova-plugin-device" /> | ||
<dependency id="cordova-plugin-badge" version=">=0.8.5" /> | ||
<dependency id="cordova-plugin-badge-fix" version=">=0.8.10"/><!-- TODO: return to 'cordova-plugin-badge' when fixed --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a reminder: it points to 'cordova-plugin-badge-fix' for the time being
I added a comment so we don't forget :-) and resolved the README conflict |
Very good. Do you know, why we can see still the Android 10 updates from @timkellypa in your pr though i merged them already? |
Do you mean the commit from 'Jan 30, 2021'? I think that is just some branch merging, idk, but it seems to show no actual changes 🤔. All the 2020 commits seem to have disappeared (as expected). |
Yeah i see this, they are disappeared. Very good, I'm happy :) |
Hi @fquirin , I've barely developed Android native code in 8 years, so I'm probably not the best person to ask for advice ^^' That's why I didn't submit a PR to your branch. Yes, I don't know if an Android version check is needed or not, maybe in old versions the I think that if the user rejects the permission then the About I'm currently with another project too, if I have time in the following weeks I can try to perform a few more tests. Thanks again for your work, and thanks @GitToTheHub too for assuming the maintenance of this plugin :) Cheers, |
Thank you :) |
Great to see the changes proposed here. What needs to be done to have this finalised? |
First I need access to cordova-plugin-badge from the author, to make a new release. Then we need the fixes for this pr from the other contributors. |
Just for the record, this is the code I've ended up using in our fork to request the POST_NOTIFICATIONS runtime permission if needed. Maybe the same can be achieved using the If the permission is already granted or device is Android 12-, nothing will happen. If it isn't, then the user will be asked when:
In that case, the user will be able to allow or deny the permission. If it's denied, the next time any of the functions above is called the user will be asked a second time. If it's denied again, then it will be denied forever, the user won't be asked again. I'm not an Android native developer so I might have done something wrong, the first tests were ok but we still need to do some exhaustive testing about this. Feel free to cherry-pick this commit in the PR if you want :) |
I think so, the code looks pretty straight forward. Currently I'm more concerned about the issue with click-events and
Top 👍 |
My previous commit had an error ^^' here's the fix: |
Thank you :) |
@fquirin katzer finally published a release of cordova-plugin-badge. Could you test if everything is fine with it?
Do you think it’s so hard to manage? Maybe we could ask @powowbox for help. |
Hi, |
Nice, I'll try to run some tests as soon as I can :-).
Well I don't know yet. I think the first step has to be to create a dedicated test app to make sure there is nothing else causing the bugs I see.
I commented this change a few weeks ago with the following:
If this remains true for a clean test app we need to start digging to find the problem :-|. |
I have some good news @GitToTheHub :-). Did a clean build with the original (updated) 'cordova-plugin-badge' dependency and saw no errors so far. In addition it seems my I found some more Android fixes in a PR to my own fork that look important, but I'd prefer to add them later in a new PR. |
Hey @fquirin, this sounds very cool. So we can make this plugin working again. Thank you :) |
Seems like this issue is fixed - great job! Is there an ETA for when this will merged? |
@fquirin @GitToTheHub I understand time is scarce and it is difficult to contribute to OS projects. However, I would really appreciate if we can get this plugin working. I am receiving a lot of complaints on my apps due to this plugin issues, so any help to move this forward is appreciated. I am not very proficient in native code, but if you provide pointers I could contribute as well. |
@jayseb11 |
Hey guys, sorry for the late reply. I'm currently drowning in work, but I'll try to fix this as soon as I can. |
@fquirin Cool, thank you :) |
@@ -138,6 +144,8 @@ | |||
<uses-permission android:name="android.permission.ACCESS_NOTIFICATION_POLICY" /> | |||
<uses-permission android:name="android.permission.RECEIVE_BOOT_COMPLETED" /> | |||
<uses-permission android:name="android.permission.WAKE_LOCK" /> | |||
<uses-permission android:name="android.permission.POST_NOTIFICATIONS"/> | |||
<uses-permission android:name="android.permission.SCHEDULE_EXACT_ALARM" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Schedule exact alarm is no longer allowed on android 13 for non alarm apps. https://developer.android.com/about/versions/14/changes/schedule-exact-alarms#calendar-alarm-clock)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have do it in a separate PR.
@fquirin What do you think?
@fquirin Hi Florian, could you find maybe some time to finish this PR? This would be greate :) Regards Manuel |
Latest v0.10.0 is a merge of @timkellypa, @bhandaribhumin (with changes by @powowbox) and @fquirin.
Notable changes:
POST_NOTIFICATIONS
permission and runtime popup added