[flutter_local_notifications] (rebased) Apply named parameters for functions in lib/src/flutter_local_notifications_plugin.dart

[TaeBbong]

Proposal for applying named parameters for functions in flutter_local_notifications_plugin.dart

Related Issue : #2609

Hi, this is same PR with previous PR #2610 This is rebased version with feature branch.

As @Levi-Lesches mentioned that this work is massive breaking changes, this would be hard to merged in short time.

Would you mind to check and review for this proposal in next major release? @MaikuB If this isn’t ideal, I’m also considering an alternative approach:

(Alternative way) Keep original form of functions, and add same-working copied functions with named parameter.

• Example

Future<void> zonedSchedule(
    int id,
    String? title,
    String? body,
    TZDateTime scheduledDate,
    NotificationDetails notificationDetails, {
    required AndroidScheduleMode androidScheduleMode,
    String? payload,
    DateTimeComponents? matchDateTimeComponents,
  });
Future<void> zonedScheduleWithNamed({
    required int id,
    required TZDateTime scheduledDate,
    required NotificationDetails notificationDetails,
    required AndroidScheduleMode androidScheduleMode,
    String? title,
    String? body,
    String? payload,
    DateTimeComponents? matchDateTimeComponents,
  });

Feel free to check about this request! I'm really happy to communicate and work with great dev like you guys:)

[MaikuB]

Thanks for the PR. Wanted to let you know I'll be prioritising looking at other PRs first as this one would introduce breaking changes that would require major release. Overall I understand the intent as it was something I thought about before. Note that even if I agree with going with the changes, it may take some time before the changes would be considered for merging as 19.0.0 is where Windows support was added and gives more time for fixes to be done as patch releases on 19

[TaeBbong]

Thanks for reply! I'm happy to get positive review from you. I fully understand this request would breaks other changes & features. I just hope this request get merged at some day:)

[Levi-Lesches]

I think one thing that would help this would be to use new functions that are aliases of the old ones, and then deprecating the old ones:

@deprecated("Use notify() instead")
Future<void> show(int id, String? title, String? body, NotificationDetails? details, {String? payload}) => notify(
  id: id,
  title: title,
  body: body,
  details: details,
  payload: payload,
);

Future<void> notify({
  required int id,
  String? title,  // note: consider making this required and non-nullable
  String? body,
  NotificationDetails? details,
  String? payload,
});

and for periodicallyShow(), periodicallyShowWithDuration(), and zonedSchedule(), I proposed in #2492:

class 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, 
);

We could do all this and have the existing methods just forward to these ones (or vice versa) and get a few benefits: named parameters, clearer names, mutually exclusive parameters, and of course, not needing a breaking change

@MaikuB thoughts? I'd also be happy to do the PR if you'd like

[TaeBbong]

@Levi-Lesches Things you suggested are pretty close with my "alternative ways". With @deprecated annotation, we can safely migrate old functions to new functions with named parameters. I can work on this job too, so feel free to ask!

[MaikuB]

@TaeBbong I have plans to do a major release that would allow for breaking changes. One has already been merged in. Would you be able to help re- check if this PR will need further updates? Presumably, updating your fork would be a good starting point

[TaeBbong]

@MaikuB Can you check and re-run iOS integration test?? It seems to be temporary issue for CI.

[Levi-Lesches]

Actually it's not a temporary issue. Some update at some point broke iOS builds without a build number in the pubspec version.

@MaikuB in my web PR I added a build number to the example app -- this is why (forgot to reply to your comment there). At this point it's probably best to make a fresh PR just for that and merge it into all PRs to fix the check.

[MaikuB]

@Levi-Lesches it's not needed as the error is a red herring. It's only meant to be for applications that are published. Moreover I merged a PR of yours recently with those changes removed and it was all green. It still had the same log entry. The YAML is on the Flutter side too so if it would fail, it's odd that it would fail for iOS only. I'd say this needs more investigation but I do recall odd timeout issues

[MaikuB]

There you go, reran it and it was fine. The logs did mention a connection issue at the end. The one about version in pubspec was higher up in the logs and not where it ended when it failed

Edit: thanks @TaeBbong for updating the PR. I'll look to merge this later in the week

[Levi-Lesches]

Hm that's frustrating, I was hoping it would be a quick fix. Guess I was over-confident because I thought I remembered fixing something by adding the build number, but I guess not.

Back to the idea of making a breaking change to the APIs, would you be open to merging PRs for these issues?

• #2492
• #2690
• #2692
• #2693

Doing at least some of these can make the API more user-friendly, and it would probably be better to batch these where possible. I'm happy to make the PRs myself I just wanted some feedback first

[MaikuB]

@TaeBbong I just realised something and that it looks the application of named parameters has not been done holistically. It should also apply to the platform interface and the platform-specific implementations of the plugin for Android, iOS, macOS, Linux and Windows (e.g. classes like AndroidFlutterLocalNotificationsPlugin). Is this something you could look into?

[MaikuB]

@Levi-Lesches I'll review those get back to you. It would be nice to group them but they could potentially go into separate major releases

[TaeBbong]

@MaikuB Got it! I'm gonna look into those things and re-submit PR.