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_windows] Windows FFI plugin #2366

Merged
merged 123 commits into from
Nov 12, 2024

Conversation

Levi-Lesches
Copy link
Contributor

@Levi-Lesches Levi-Lesches commented Jul 11, 2024

Update: I went in and simplified the code since yesterday, it's now in a reviewable state. If it would help, I can rebase + commit the first 50 or so commits and start with a cleaner slate.

This is a spin off of #2349, which implements the same logic but with an FFI plugin instead of method channels. I'm happy with it so far, here's the gist:

The C/C++ code

  • src/ffi_api.h: a C API between C++ and Dart
  • src/plugin.hpp: A C++ class that holds Windows-specific API handles
  • src/ffi_api.cpp: C++ code to implement the C API using the C++ class

The Dart code

  • a new package, flutter_local_notifications_windows that implements the same functionality as before.
  • lib/src/details holds all the platform-specific details
  • lib/src/ffi holds basic FFI stuff, like generated bindings and utils
  • lib/src/plugin holds the Windows plugin interface, the real FFI implementation, and a stub, because dart:ffi will break web apps (note, the API does reference dart:io but it still compiles on web)

Notes

  • Overall, the only thing that changed was how to go from Dart <--> C++. Before, it was all JSON-based message passing, and a big HandleMethodCall function that had to handle every possibility. I'm glad that's gone. Now we have a similar mechanism with Dart <--> C <--> C++ and package:ffigen sets everything up for us.
  • It's all backed with compile-time safety as opposed to the runtime failures I was facing with method channels. For example, passing a string now looks like "hello".toNativeString(), but other types require allocating memory manually.
  • Functionally, everything is working how it was before, with the method channels

Benefits

  • the C++ code is ~150 lines smaller, and went from being one big 250 line file to two 150 line files, one of which can be skimmed as it's just Windows registry config.
  • The C++ code no longer uses method channels or any of that associated logic. There's a lot of magic there involving serialization which also comes with a runtime cost. The new code directly passes values and pointers from Dart to C++, eliminating all the message passing. In fact, the new API is inherently synchronous instead of async (not that it matters in this context, since the other platforms are still async).
  • The Windows implementation now lives in flutter_local_notifications_windows instead of hijacking the original package and half-sharing some of the original implementations. That means no more platform checks, all the logic can be safely dealt with in a Windows-only context.

Summary of changes

  • Windows C/C++ code: +600 lines
  • Windows Dart API for customizing notification details): +800 lines
  • Windows Dart plugin code, excluding generated code: +300 lines
  • Main plugin integrations: < 50 lines
  • Example Dart code: +500 lines

That's only ~2,200 additions. the other half GitHub is reporting is automatically generated, like the example's new windows folder.

Important

FFI is a more recent development than method channels. As such, some more advanced FFI features are locked behind some more recent Dart versions. I already had to downgrade package:ffigen from a beta 13.0 to a whopping 7.0, but more pressing is that for C to call a Dart function from another thread (eg, when a notification is pressed), you need to use features from Dart 3.1. The current constraint is only 2.17.

Including this new implementation will force Dart ^3.1.0 on end-users. At this point, 2.17 is two years old, 3.1 is about one year old, and null safety is almost 3.5 years old. Age aside, I also don't believe going from 2.17 to 3.1 is so burdensome, especially since 2.17 is already after null-safety, and the Dart team's official position is that 3.0 is not really a breaking change in practice. In other words, I'm not sure I see a situation where someone is really stuck. They can either upgrade any pre-null-safety code, not upgrade this package further, or fork this package to add null Windows or future updates as needed.

Overall, I'd recommend bumping the SDK version of the front-facing package to 3.1.0 as well to better advertise this, and to benefit from any features, since 2.17 and beyond -- better null promotion, FFI for mobile platforms, class modifiers, macros, and more.

@Levi-Lesches
Copy link
Contributor Author

@MaikuB I merged and formatted, want to give it another go?

@Levi-Lesches
Copy link
Contributor Author

Looks like the Android tests are indeed passing, but the new version of Flutter (3.24) requires a new bootstrapping mechanism:

Running Gradle task 'assembleDebug'...                          
ERROR: ERROR: You are applying Flutter's app_plugin_loader Gradle plugin imperatively using the apply script method, which is deprecated and will be removed in a future release. Migrate to applying Gradle plugins with the declarative plugins block: https://flutter.dev/to/flutter-gradle-plugin-apply


ERROR: ERROR: 


ERROR: ERROR: You are applying Flutter's main Gradle plugin imperatively using the apply script method, which is deprecated and will be removed in a future release. Migrate to applying Gradle plugins with the declarative plugins block: https://flutter.dev/to/flutter-gradle-plugin-apply


ERROR: ERROR: 

These have always appeared in the logs but only in the new version are actually failing the tests. I can help with those in a separate PR if you'd like, but I think this one is ready to merge despite that.

@MaikuB
Copy link
Owner

MaikuB commented Nov 12, 2024

These have always appeared in the logs but only in the new version are actually failing the tests. I can help with those in a separate PR if you'd like, but I think this one is ready to merge despite that.

Yeah I left this as plugin needed a min version bump to migrate the example app. Once your PR is merged in then should be fine to migrate and yes happy to take a PR on that :)

@MaikuB MaikuB merged commit 697b007 into MaikuB:master Nov 12, 2024
@Levi-Lesches Levi-Lesches deleted the windows-ffi branch November 12, 2024 11:09
@MaikuB
Copy link
Owner

MaikuB commented Nov 12, 2024

Merged in now. Thanks a lot! Happy to take PR on the example app but note I'll do one PR around relating to

export 'package:flutter_local_notifications_platform_interface/flutter_local_notifications_platform_interface.dart';
. I missed mentioning in PR and didn't want to hold it up further and the reason for being selective before was to avoid having the helper methods being part of the public API of the plugin itself. Have found another approach to handle it though

@MaikuB
Copy link
Owner

MaikuB commented Nov 12, 2024

Should also mention my plan right now is to release it after the example app issue is fixed and the release would be done as part of prerelease

Edit: looks like everything was all green for my recent PR #2454 despite the issue where example hasn't migrated so could be that when it failed last time that something else happened e.g. GitHub runner having an issue

@Levi-Lesches
Copy link
Contributor Author

Levi-Lesches commented Nov 12, 2024

Sounds good. If the builds are still green then I might push it lower down on my to-do list for now, so if you have more time on your hands then maybe don't wait for me.

@MaikuB
Copy link
Owner

MaikuB commented Nov 12, 2024

All good. I'll be on holidays for a while soon as anyway :)

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.

Use FFI for Windows implementation
10 participants