fix(android): LocalNotification on not working properly

[kheftel]

I've read the contributing docs and I hope I'm doing this properly, please feel free to educate me on how to best approach this.

I needed to use the cron-like on notifications on a project of mine to make daily notifications happen at a certain time (like every day at 8:30 am) and noticed the Android implementation didn't work properly. On iOS, it all appears to be handled by DateComponents, but on Android it's got a more complex custom implementation.

It's a bit hard to find documentation, but after examining the native code, it appears that the on style notifications are supposed to let you:

• pass a minute and have a notification repeat each hour (passing 15 would trigger every hour at :15)
• pass an hour and a minute and have a notification repeat each day (passing 14 and 30 would result in a notification at 2:30pm each day, exactly my use case and what led me to investigate this)
• pass a day, hour, and minute and have a notification repeat for example on the 5th of every month at 8:30am.
• pass a month, day, hour, and minute and have a notification repeat each year for example on May 1st at 3pm.

I could be wrong in my understanding, of course.

On the iOS side, Apple's documentation for DateComponents isn't very clear, tbh.

Googling turned up some problems people were having (link, link), and I ran into my own issues as well. When trying to have a notification fire on the hour, it sometimes fired not on the hour, and if I passed hour and minute, I would sometimes get a notification that triggered at the next whole hour instead of the next day, and when I passed hour and omitted minute, I ended up with a notification that repeated ad infinitum as soon as I dismissed it, etc.

The existing logic appeared to be incomplete. It calculates the next time the notification should fire by first taking the current moment, setting the provided hour, minute etc, seeing if the resulting trigger time is in the past, and then bumping it by a certain amount. But it was only comparing to see if one unit was less than another (i.e. if hour < hour or if minute < minute), and sometimes it bumped by the wrong amount. Now it compares the timestamp of the two dates and explicitly remembers how much each iteration of the notification should be advanced by, according to the largest unit specified.

I've fixed it for my use case, i.e. I can pass hour and minute, and if device time is 2:15pm and I schedule it for 2:30pm, it goes off 15 min later, and if device time is 2:30 pm and I schedule it for 2:15pm, it schedules for the next day at 2:15pm. I left a logging statement commented out in the code for now that helped me test it and verify it's working - I expect to remove it before merge. The logic should be sound for the longer-repeating cases as well, though I haven't tested those on-device yet (I can certainly do that if I'm going about this right).

Please let me know how to proceed next, I am very impressed with this platform and am excited to be building my app on it, and if I can also contribute something back, so much the better. Thanks!

[kheftel]

Ok, I tested up to monthly recurring events and everything seems to be working.

My testing repro is available here. In case you want to try it out, you'd have to install @capacitor/core and @capacitor/android into it from the core and android folders in a local cloned version of my capacitor fork. I don't if it's possible to install monorepo modules from github, and my testing was not automated, and on a device, so, sorry if it's messy. I basically set up a bunch of notifications at various repeating intervals in the past and future and then played with the device clock.

Let me know if there's anything else I can do, if I should re-base on main or on v2.3.0. I have an app that's going into beta that will be using this feature branch for now, and when/if this gets merged/resolved I'll switch it back to the main repo again. Thanks!

[imhoffd]

@carlpoole Care to take a look?

@kheftel Yeah, please change the base branch to 2.x, not main. No need to recreate for main, we'll merge fixes in after each 2.x release. Thanks!

[kheftel]

I borked it with an accidental force push, changing it to a draft while i fix it, sorry!

[kheftel]

ok, based it on 2.x branch, testing didn't seem to break anything, should be ready to look at now.

[carlpoole]

I tested the changes and this appears to be working as expected. I went ahead and removed the test log and the commented out method for minor cleanup. Thanks for contributing! 🎉 👍

[imhoffd]

@jcesarmobile Here's a fix for 2.x. Good to go?

[kheftel]

thanks @carlpoole!

Thanks for changing the logs to debug, I assume that's what the other capacitor modules/plugins do. I also didn't see any commented-out things in other parts of the code, so, that makes sense that you would do that as part of capacitor's workflow. I didn't want to remove them before reviewers had seen them tho.

[jcesarmobile]

I've recommended a minor improvement, but changes look, can be merged as is. I'll let kheftel decide if my suggestion makes sense.

[kheftel]

absolutely @jcesarmobile! Actually, I just pushed a new commit that zeroes out both second and millisecond, and does it in a slightly different place.

[imhoffd]

Thanks, @kheftel! Great work 💪