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 Daily Notifications #61

Closed
jeffscaturro-aka opened this issue Jun 19, 2018 · 17 comments
Closed

Multiple Daily Notifications #61

jeffscaturro-aka opened this issue Jun 19, 2018 · 17 comments

Comments

@jeffscaturro-aka
Copy link
Contributor

Thanks so much for writing this plugin, it's incredibly useful. Hopefully those final issues get cleared out with flutter soon so it can be officially adopted.

Onto the not so great part, I have two different and independent notifications scheduled to be shown daily calling showDailyAtTime . One is shown in the morning, and the other in the evening. They both have unique notification ids and titles, while the body is the same.

If I schedule them to display relatively close to the current time, they will both execute and display. However the next day only the last one scheduled will display, which is the evening one in our case. The code execution is as follows:

  • (await) cancel all existing notifications through plugin
  • (await) schedule morning reminder
  • (await) schedule evening reminder

Have you seen anything similar or know of any current issues that may cause this? Thanks.

@MaikuB
Copy link
Owner

MaikuB commented Jun 19, 2018

Thanks for the compliments though I don't expect it to be an official plugin :)

Anyway, I've not heard of any such issues. Sounds like you're saying both notifications were showing and then one would disappear? If so, perhaps this is the OS doing it? I don't normally leave my notifications to the next day to see but there shouldn't be anything to do with the API that would result in the behaviour you're describing.

@jeffscaturro-aka
Copy link
Contributor Author

Right, I'd schedule the morning & evening notifications, say around mid-day. I'd get the evening notification that night. The next morning there would not be a notification, however in the evening there would be one. I am testing this on an iOS device (iPhone) currently, so not sure if this is seen on Android yet. I wanted to check prior to diving too deep to see if it was familiar at all or not. I have some more testing planned for Tuesday & Wednesday and can update this issue with any findings and ultimately close it out. If any changes come up I'll be sure to drop a PR!

Thanks again.

@MaikuB
Copy link
Owner

MaikuB commented Jun 19, 2018

Might be worth scheduling just one in the morning to see what happens but yeah if you find something wrong then a PR would be much appreciated :)

@jeffscaturro-aka
Copy link
Contributor Author

Just wanted to check back in. I did not receive the morning notification again. My current lead is scheduling the notification at a time past the notification's time.

We are scheduling a notification for the morning (say 8am), but that scheduling is being done around the afternoon. This causes the trigger to use today's day, which won't get triggered. Would this not getting triggered affect future notifications to display?

@MaikuB
Copy link
Owner

MaikuB commented Jun 20, 2018

Yeah I think you're onto something there. Tagging @javiercbk in case he may know too or have run into the same issue. I suspect what needs to be done is to change the components used as per the line code you linked to.

May well be that the fix is that on that line you linked to, if a day has been specified (for weekly notifications) then the components used are NSCalendarUnitDay | NSCalendarUnitHour | NSCalendarUnitMinute | NSCalendarUnitSecond, otherwise use NSCalendarUnitHour | NSCalendarUnitMinute | NSCalendarUnitSecond. This something you can fork, test on and submit a PR for? If you check the Dart docs, pubspec allows for referencing packages that are local or git-based https://www.dartlang.org/tools/pub/dependencies

@jeffscaturro-aka
Copy link
Contributor Author

I can definitely do that. I should be able to get some time tonight or tomorrow on it. Thanks!

@MaikuB
Copy link
Owner

MaikuB commented Jun 20, 2018

Sure, I'm pretty confident that will solve the issue but figured it would be better to have another pair of eyes on it :)

jeffscaturro-aka added a commit to jeffscaturro-aka/flutter_local_notifications that referenced this issue Jun 21, 2018
@jeffscaturro-aka
Copy link
Contributor Author

@MaikuB I pushed up a commit (auto referenced above) that removes specifying NSCalendarUnitYear|NSCalendarUnitMonth|NSCalendarUnitDay completely. Since we already set the weekday if notificationDetails.day != nil, we should be good for omitting NSCalendarUnitDay in weekly notifications, but let me know if otherwise. I was doing some research and it looks like we don't need to specify the year, month, and day for these.

I scheduled a daily one for a minute later and it displayed; but I won't know if it fixes the case that was causing trouble for us until the morning (I couldn't devise a way to test this scenario without waiting a day).

I don't suspect this would cause regressions with other calls into showUserNotification but I am not as familiar with the package as you.

@MaikuB
Copy link
Owner

MaikuB commented Jun 21, 2018

With regards to regression, it shouldn't be an issue. Though to be safe, I think you should specify the calendar property of the date components be Gregorian as I suspect this could cause issues for apps/devices that use a different calendar system and that leaving it blank will just cause the date components be interpreted in the current calendar system of the app/device

jeffscaturro-aka added a commit to jeffscaturro-aka/flutter_local_notifications that referenced this issue Jun 21, 2018
@jeffscaturro-aka
Copy link
Contributor Author

Theoretically it should work; I'll know if this works in practice early tomorrow, and if so I'll get a PR up.

@MaikuB
Copy link
Owner

MaikuB commented Jun 21, 2018

Yep though for the us in western world, the default will most likely be Gregorian that's why it'll work. I'd suggest making that change to set the calendar now if you can so you can see if it still works as it's something I'd like to see done. Particularly when you look at documentation for the Swift bridge to NSDateComponents here and talks about the behaviour on what happens if the calendar is left out. I'd expect the same behaviour for the NSDateComponents class and the documentation for the latter says it's meaningless in itself unless a calendar is specified.

@jeffscaturro-aka
Copy link
Contributor Author

I included the calendar assignment in 6c4e6a6.

@MaikuB
Copy link
Owner

MaikuB commented Jun 21, 2018

Been looking a bit into this too and I reckon the same change will be needed in showLocalNotification. Unfortunately I've only got a simulator for this given this affects only older iOS versions so will need to wait to verify that it works.

Edit: scratch that, should be fine. Old notification APIs took a date rather than looking at the date components and then you specify how often it should repeat.

@jeffscaturro-aka
Copy link
Contributor Author

Just got my morning notification! 🎉 Just going to do a sanity regression test on the weekly notification and will create a PR.

@jeffscaturro-aka
Copy link
Contributor Author

Closing issue with Open PR.

MaikuB added a commit that referenced this issue Jun 22, 2018
[#61] iOS: Refactor date components used in repeat notification scheduling.
@MaikuB
Copy link
Owner

MaikuB commented Jun 22, 2018

FYI, I published a new version with your fix included :)

@jeffscaturro-aka
Copy link
Contributor Author

Awesome! Thank you!

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

No branches or pull requests

2 participants