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

Multiple entries with same notification ID in pending notifications #898

Closed
jongkb opened this issue Nov 12, 2020 · 4 comments · Fixed by #899
Closed

Multiple entries with same notification ID in pending notifications #898

jongkb opened this issue Nov 12, 2020 · 4 comments · Fixed by #899

Comments

@jongkb
Copy link

jongkb commented Nov 12, 2020

Describe the bug

To Reproduce

  1. Schedule a repeating notification (e.g. matching time component i.e. daily notification) with an id 200, few minutes into the future.
  2. Wait till notification is fired.
  3. Two entries with notification ids 200 were returned when get pending notification requests is called.

Other observation (could be related?):
After a few days of notification firings, on rebooting, multiple notifications were fired at once. Also happens on package update.

Issue does not happen when either one of this is true:

  1. Notification id is smaller than 128
  2. One-shot notification.

Expected behavior

  1. Only one pending notification with id 200 should be returned.
  2. No side effect i.e. no multiple notification firings at once on reboot/update after several notification firings.

Plugin code causing the problem

Suspected improper Integer equality test of notification id before saving to preferences.

Suggested fix

To use equals() method for Integer equality test instead.

@MaikuB
Copy link
Owner

MaikuB commented Nov 12, 2020

Thanks for finding this and providing the info as I was able to use this to reproduce the problem. Your suspicion is correct and I have a PR for this #899. The side effect you're seeing is due to old notifications being rescheduled and the behaviour of Android's AlarmManager for old scheduled work is to have them appear immediately. This should be fixed by a check that I'll add to make sure only future notifications are shown. From my testing with the example app, the PR should fix the issue. Do you think you can try out the PR/branch and let me know if it fixes the issue for you as well before I merge this in?

@jongkb
Copy link
Author

jongkb commented Nov 12, 2020

I've tested the PR/branch with my app. Works as expected now. Thank you so much for looking into this.

@MaikuB
Copy link
Owner

MaikuB commented Nov 15, 2020

FYI that I'll need to revert the check for future notifications within the Android native code. There's a scenario where might be possible for a device to be off whilst the notification should appear and it should show up when the device turns on again

@jongkb
Copy link
Author

jongkb commented Nov 15, 2020

Noted on that. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants