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

[flutter_local_notifications] Allow specifying start time of periodic notifications. #2503

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

BadgerHobbs
Copy link

Firstly, thank your the continued work supporting and maintaining this package, it is much appreciated.

Current Challenge

Currently if you schedule a notification using periodicallyShow the notification will be sent every day since the creation of the notification. As I understand this is the currently accepted and documented behaviour.

This means that you cannot set a periodic notification to start after a specific date, such as required in the below scenario.

Example Scenario

  • Current date is 2024-01-01 12:00:00.
  • Schedule a zonedSchedule notification for 2024-01-05 12:00:00 to tell the user that a task is due.
  • Schedule a periodicallyShow notification from 2024-01-06 12:00:00 to tell the user that a task is overdue every day.

Current Outcome

  • User receives an overdue notification every day from the 2024-01-01 12:00:00 onwards.

Desired Outcome

  • User receives an overdue notification every day from 2024-01-06 12:00:00 onwards.

An unresolved question here on StackOverflow describes a similar issue.

Changes

Inspired by the work/changes proposed by @henriklagergren in this discussion, the following change has been made based off the latest master branch.

The existing periodicallyShow and periodicallyShowWithDuration methods have been updated with an optional parameter to replace the default calledAt value of clock.now().millisecondsSinceEpoch. This is to make the change non-breaking.

Testing

I have tested and validated the change on Android, and can at least confirm that the notifications to start at the date time now provided to the periodicallyShow and periodicallyShowWithDuration methods. This results in the desired outcome of the example scenario being presented.

I do not currently have an iOS device, so am unable to confirm that the behaviour is identical. From the testing of @anilslabs mentioned here it appears that this may not be the case.

@Levi-Lesches
Copy link
Contributor

Interesting! I'm also thinking about re-organizing those methods in general, see #2492. That research led me to realize that iOS fundamentally doesn't support this feature. Rather, like with Android, we'd need to work in a background task handler to schedule these tasks at the right time. I believe that's a fine approach, but it does require more code and I don't have an iOS device either :)

@BadgerHobbs
Copy link
Author

BadgerHobbs commented Dec 23, 2024

Interesting! I'm also thinking about re-organizing those methods in general, see #2492. That research led me to realize that iOS fundamentally doesn't support this feature. Rather, like with Android, we'd need to work in a background task handler to schedule these tasks at the right time. I believe that's a fine approach, but it does require more code and I don't have an iOS device either :)

Ah, classic iOS not supporting the feature. Sounds like a background task approach would in theory work, not sure how the implementation would be impacted by the handling of background tasks though.

From my experience they are run inconsistently at best, and sometimes not at all.

Would need someone with more experience/expertise to chime in on that one.

@Levi-Lesches
Copy link
Contributor

Levi-Lesches commented Dec 24, 2024

I left a comment in #2492 about how to possibly support iOS, MacOS, and Windows

@MaikuB
Copy link
Owner

MaikuB commented Dec 24, 2024

The feature was never done and requests on this had been closed as Apple doesn't support it like @Levi-Lesches mentioned.

@BadgerHobbs yep background tasks is an approach and I do remember consistency issues from it being subject to the OS optimisations e.g. around battery. If this is still the case then it'll likely not meet the use case. I've already seen a number of issues in Android with developers using inexact timing and still question why it doesn't fire at the exact timing.

Another thing to note is there is already a work manager plugin that makes use of background tasks. I've seen the community leverage this with this plugin already. My preference would be to keep the separation that way

@BadgerHobbs
Copy link
Author

The feature was never done and requests on this had been closed as Apple doesn't support it like @Levi-Lesches mentioned.

@BadgerHobbs yep background tasks is an approach and I do remember consistency issues from it being subject to the OS optimisations e.g. around battery. If this is still the case then it'll likely not meet the use case. I've already seen a number of issues in Android with developers using inexact timing and still question why it doesn't fire at the exact timing.

Another thing to note is there is already a work manager plugin that makes use of background tasks. I've seen the community leverage this with this plugin already. My preference would be to keep the separation that way

Thanks for the reply. Yeah I think this functionality is going to be very difficult to implement reliably on iOS. A different approach might just be to do network based notifications as that may be more reliable (though not something I've investigated).

As it stands my intention is to maintain the current functionality in my fork as it is useful in my app, and I can just disable the functionality for iOS users as it's not super critical.

What's the current approach for asymmetric features across platforms, sounds like it's something just generally avoided?

@Levi-Lesches
Copy link
Contributor

What's the current approach for asymmetric features across platforms, sounds like it's something just generally avoided?

I updated my proposal in #2492 to address this with an API I'm really happy with, please check it out and leave feedback!

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.

3 participants