-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Refactor and redesign repeating notifications #2492
Comments
This comment was marked as outdated.
This comment was marked as outdated.
Responding to the points in #2503:
That leaves us in a bit of an awkward position -- offering features that Android doesn't natively support, but not offering them on Darwin because... Apple doesn't support them. Not to mention Linux and Windows (see above). At this point, Android should probably stick around as-is for historical reasons, and other platforms can utilize
So just to review:
I realized the key to making a cleaner API is to embrace the differences between platforms instead of trying to cover them up. After playing around a bit, I came up with this new API I'm really happy with: New APIclass NotificationSchedule {
AndroidSchedule? android;
DarwinSchedule? ios;
DarwinSchedule? macos;
WindowsSchedule? windows;
// Linux and Web are not supported
}
sealed class AndroidSchedule { }
sealed class DarwinSchedule { }
sealed class WindowsSchedule { }
class ScheduleOnce implements AndroidSchedule, DarwinSchedule, WindowsSchedule {
TZDateTime date;
}
class ScheduleRepeating implements AndroidSchedule, DarwinSchedule {
TZDateTime date;
DateTimeComponents repeatsOn;
}
class PeriodicScheule implements AndroidSchedule, DarwinSchedule {
Duration interval;
}
class PeriodicScheduleWithStart implements AndroidSchedule {
Duration interval;
TZDateTime start;
}
void schedule({
required int id,
required NotificationSchedule schedule,
String? title,
String? body,
String? payload,
NotificationDetails? details,
); And I still think Several benefits here:
Updated examplesfinal location = getLocation(...);
final now = TZDateTime.now(location);
// Schedule a notification tomorrow at 8 pm
final tomorrowAt8 = ScheduleOnce(date: now.copyWith(hour: 20, minute: 00));
plugin.schedule(
id: 0,
schedule: NotificationSchedule(
android: tomorrowAt8,
ios: tomorrowAt8,
macos: tomorrowAt8,
windows: tomorrowAt8,
),
// title, body, details
);
// Schedule a repeating reminder, every day at noon
final repeatAtNoon = ScheduleRepeating(
date: now.copyWith(hour: 12, minute: 00),
repeatsOn: DateTimeComponents.time,
);
plugin.schedule(
id: 1,
schedule: NotificationSchedule(
android: repeatAtNoon,
ios: repeatAtNoon,
macos: repeatAtNoon,
// Trying to pass `windows: repeatAtNoon` is a compile-time error
windows: ScheduleOnce(date: now.copyWith(hour: 12, minute: 00)),
),
// title, body, details
);
// Schedule a reminder to take a break. Differs by platform.
plugin.schedule(
id: 2,
schedule: NotificationSchedule(
// Android: every 30 minutes, starting 30 minutes from now
android: PeriodicScheduleWithStart(
start: now.add(Duration(minutes: 30)),
interval: Duration(minutes: 30),
),
// Darwin: Every hour, starting from now
ios: PeriodicSchedule(interval: Duration(hours: 1)),
macos: PeriodicSchedule(interval: Duration(hours: 1)),
// Windows: In 1 hour from now
windows: ScheduleOnce(date: now.add(Duration(hours: 1)),
);
repeatsOn: NotificationRepeatInterval(Duration(minutes: 30)),
// title, body, details
); @MaikuB What do you think? I think it would be good in the long-term to replace |
Sentence here seems a bit off. I suspect you were trying to convey that there a features available on Android but not be enabled as Apple doesn't support them. If so, something to important here to remember here as developers pick Flutter to build apps across multiple platforms with the intent of having feature parity. It would be very unlikely that an app would go ahead and make a feature to do with notifications only available on, say, Android only (note: this is factoring that Flutter apps are more likely targeting only Android and iOS). Typically libraries like this plugin are built more to support cross-platform development. When it comes to cross-platform development (i.e. including outside the Flutter ecosystem), apps/developers that may have specialised use cases would go down the path of writing the code to interact with the native APIs. Part of what I'm getting at here is being conscious of building for edge cases and being careful of going down the rabbit hole of trying support the feature available for every platform. Might not be applicable in this case Regarding the proposal itself, I'd need to think about it some more but some initial thoughts and info to share
|
Actually, what I was getting at -- and please correct me if I'm wrong because I'm still a bit confused -- is that Android doesn't seem to have any feature that says "schedule a notification for this time". Android supports alarms, which aren't user-visible notifications but rather trigger background work, and the plugin uses those alarms to show a notification. So really, the Android implementation schedules background work which then shows the actual notification. Which is convenient, but it's inconsistent to then say "iOS doesn't support scheduled notifications with start offsets, only scheduled background work", because that's exactly the case with Android as well. The difference here is that iOS supports some scheduled notifications, just not with a start offset, and including a start offset means scheduling background work. So the plugin could schedule a background task with a start offset and use that to show a notification like it does on Android. Obviously, that's more code to maintain, and I'm not necessarily advocating that we should take that path, just mentioning the inconsistency between platforms.
Agreed. While it would be possible to switch the entire plugin to use background tasks on Darwin, I think sticking as close to the official APIs as possible makes maintenance easier, and the plugin was already built to do so. I don't really think this one use-case is enough of an argument to switch implementations completely. And the rest of my comment/proposal is more about separating out features between platforms to convey this better.
Right, but then consider that these methods advertise functionality that is just not available on certain platforms:
I'd say if a method needs per-platform configuration, then it should accept that as a parameter, rather than declaring different methods that have wildly different behaviors on different platforms. Admittedly, this is more of a concern with platforms besides Android/iOS, and that may not be a priority, but linux + windows + web = half the platforms this plugin will support.
Agreed that discoverability is important, so these would have to be documented, which shouldn't be so bad. I'm thinking a "which options are supported on which platforms" table on the
I'm open to workshopping the names. I originally had them like |
I'm writing up the usage guide now (see #2477), and quickly found that scheduling repeating notifications is complicated. I'm sure this is at least partially historical, but users have three methods to choose from:
zonedSchedule()
, with a non-nullmatchDateTimeComponents
periodicallyShow()
, with a givenRepeatInterval
periodicallyShowWithDuration()
, with a givenDuration
Fundamentally, there are four ways to show a scheduled notification:
The differences between the methods
From what I can tell, the main difference between
zonedSchedule()
andperiodicallyShow()
is thatperiodicallyShow()
starts now, whereaszonedSchedule()
takes aTZDateTime
parameter. It's true thatUNTimeIntervalNotificationTrigger
doesn't take in a date/time to start the timer and starts immediately. So there's no way to say "send a notification every 30 minutes, starting in 2 hours from now".To get around this, we can use an approach similar to Android, where we schedule a background task for the given date and time, which then schedules the first
UNTimeIntervalNotificationTrigger
.The only difference between
periodicallyShow()
andperiodicallyShowWithDuration()
is the ability to pass an arbitrary duration, but Android, iOS, and MacOS support an arbitrary duration, so we should just always allow an arbitrary duration, preferringDuration(days: 1)
toRepeatInterval.daily
. In fact, on Android and Darwin, the enum is converted to milliseconds, and on Linux and Windows periodic notifications are not supported at all.New proposal
Since this will be a breaking change anyway, I renamed some parameters. I removed
uiLocalNotificationDateInterpretation
since it only applies to devices running iOS 10 or earlier, but Flutter hasn't supported those versions since Flutter 3.13, and this plugin only supports Flutter ^3.19. I also think we should moveandroidScheduleMode
to be an optional parameter onAndroidNotificationDetails()
, defaulting to something reasonable like.inexact
, but I can see an argument not to since it doesn't apply toshow()
.Examples
@MaikuB What are your thoughts on implementing this for v20? It is breaking but it would significantly simplify the API, favoring one method over three, and rid us of a lot of unused code. Again, I'd be happy to contribute a PR
The text was updated successfully, but these errors were encountered: