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

Refactor the internal APIs to reduce boilerplate and casts #2467

Open
Levi-Lesches opened this issue Nov 18, 2024 · 0 comments
Open

Refactor the internal APIs to reduce boilerplate and casts #2467

Levi-Lesches opened this issue Nov 18, 2024 · 0 comments

Comments

@Levi-Lesches
Copy link
Contributor

Levi-Lesches commented Nov 18, 2024

Here are the relevant parts of the current API I'm looking to change

  • details are defined in an implementation-specific package and are completely unrelated between platforms
  • there is a platform plugin abstract class defined in its own package
  • each platform implements their plugin in a separate package
  • each plugin expects its own details
  • At launch, the platform plugin, if there is one, registers itself as FlutterLocalNotificationsPlatform.instance
  • the main plugin defines a "collection of details"
  • the main plugin defines a "master plugin" that uses the correct details in the correct plugin

The key point of this issue is that there is no way to abstract over any of this. Since all the details types are completely unrelated, each plugin must override each FlutterLocalNotificationsPlatform method to accept its own version of the details, and the master plugin must also override each one to supply the platform implementation with the correct details. In other words, we get a lot of functions like

Future<void> show(...) {
  if (defaultTargetPlatform == TargetPlatform.android && ...) {
    // call resolve() with details.android
  } else if (defaultTargetPlatform == TargetPlatform.ios && ...) {
    // call resolve() with details.ios
  }  else if (...) { 
    // and so on
  }
}

This class alone takes up 500 lines of code and is full of runtime checks. I believe there's a better way:

In the platform_interface package:

  • declare a base class for all details, DetailsBase
  • declare a "collection of details", DetailsCollectionBase, with T? forPlatform<T extends DetailsBase>()
  • declare a base class for all plugins, PluginBase that uses DetailsCollectionBase when needed
Implementation of the platform interface
// ignore_for_file: public_member_api_docs, one_member_abstracts

abstract class DetailsBase { }

abstract class DetailsCollectionBase {
  T? forPlatform<T extends DetailsBase>();
}

abstract class PluginBase {
  static PluginBase? instance;

  void show(int id, covariant DetailsCollectionBase details);
}

In each platform's implementation/package:

  • Subclass DetailsBase with that platform's details
  • Subclass PluginBase and implement all the methods
Example of an Android implementation
// ignore_for_file: public_member_api_docs, one_member_abstracts, always_specify_types, avoid_print, lines_longer_than_80_chars

import 'platform.dart';

class AndroidDetails extends DetailsBase {
  int get androidId => 3;
}

class AndroidPlugin extends PluginBase {
  @override
  void show(int id, DetailsCollectionBase details) {
    final androidDetails = details.forPlatform<AndroidDetails>();
    print(androidDetails?.androidId);
  }
}

In the main package:

  • import all the platform implementations
  • implement DetailsCollectionBase with a member for each platform, like NotificationDetails
  • Implement Plugin by forwarding all methods to PluginBase
    • Alternatively, just expose PluginBase.instance for end-users directly
Implementation of the main package
// ignore_for_file: public_member_api_docs, one_member_abstracts, always_specify_types, avoid_print, lines_longer_than_80_chars

import 'dart:io';

import 'android.dart';
import 'platform.dart';

class DetailsCollection extends DetailsCollectionBase {
  DetailsCollection(this.android);
  final AndroidDetails? android;

  @override
  T? forPlatform<T extends DetailsBase>() {
    if (Platform.isAndroid) {
      return android as T?;
    } else {
      return null;
    }
  }
}

class Plugin extends PluginBase {
  @override
  void show(int id, DetailsCollection details) =>
    PluginBase.instance?.show(id, details);
}

This would all be non-breaking, but a lot more code can be saved by making a slightly breaking change: moving all platform-specific parameters into their respective PlatformNotificationDetails or lifting them to the platform interface. For example, .cancel(int id, {String? tag}) uses a tag parameter, and then specifically has to check whether to pass it (Linux) or not (otherwise). If all platforms accepted a tag parameter, this could be avoided. Similarly, zonedSchedule has an extra required parameter, androidScheduleMode, whereas that should maybe be an optional parameter on AndroidNotificationDetails with a reasonable default, like .exact.

@MaikuB What do you think? I'm in the middle of adding web support and I noticed a pattern like this would help reduce new code to the main package, and possibly some other boilerplate code as well, like the initialization settings. Feel free to pop any of these code samples into an IDE, they should have no analysis issues. Of course, I'll take care of the PR.

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

1 participant