[flutter_local_notifications_windows] Windows FFI plugin

Levi-Lesches commented 4 months ago

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.

MaikuB commented 4 months ago

FYI I've merged in https://github.com/MaikuB/flutter_local_notifications/pull/2373 that bumps the minimum Flutter SDK requirement to 3.13. From my understanding, this uses Dart 3.1. The other breaking changes I mentioned I was going to do that relate to the Android side was merged in as well

Note I still need to setup Windows as a dev environment. Looks like Flutter doesn't support ARM for Windows yet for me to use my macBook. However, I do have a PC I could set setup. In the meantime, I'll see what I can comment on for the PR.

General comments I have so far

Is it possible for the PR to update the GitHub workflow such that it can build the example app for Windows?
Are there unit tests that can be added/run?

Levi-Lesches commented 4 months ago

Oh, good to know about the breaking sdk change, I was afraid that would be a blocker. What motivated that? Just curious.

I can work MSIX packaging into the CI, sure. Two features (cancel by ID and get active notifications) don't work without being an msix so having that could be useful. Seems like it's just a few lines in melos.yaml. Is that being stored or published anywhere? Or is it just to verify that the examples still build?

I didn't add tests or major documentation yet. For docs, I added dart docs to all new code and some places that have different behavior on windows, but the main readme would still need to be reviewed again for windows details.

How should I go about unit tests? Notifications are visual, and the tests can't check active notifications due to not being run as an msix app. I can do some smaller tests, like:

assert invalid guids throw properly
assert invalid XML throws properly
try all notification details and make sure nothing throws
schedule a notification, check it's been scheduled. Wait, check that nothing is scheduled anymore

Am I missing any bigger picture test cases here?

MaikuB commented 4 months ago

Oh, good to know about the breaking sdk change, I was afraid that would be a blocker. What motivated that? Just curious.

To make the plugin easier to maintain. This also factors in how Flutter SDK updates are done and handles changes to Android ecosystem too e.g. when new Android Studio and AGP versions are released.

How should I go about unit tests? Notifications are visual, and the tests can't check active notifications due to not being run as an msix app. I can do some smaller tests, like

The visual side would be more of integration/UI testing so wouldn't be unit tests that I'm referring to. There are existing examples in repo and they check that appropriate details are passed via the platform channels. Note this was an open-ended question so it's not like I was calling out specifics but if possible/applicable, it's that kind of testing I was referring to. Based on what you shared, I think the first three bullet points align with that. The scheduled notification scenario sounds like an integration test. Not against having that but due to potential difficulty on covering integration tests, I hadn't asked about that

Levi-Lesches commented 4 months ago

The visual side would be more of integration/UI testing so wouldn't be unit tests that I'm referring to. There are existing examples in repo and they check that appropriate details are passed via the platform channels. Note this was an open-ended question so it's not like I was calling out specifics but if possible/applicable, it's that kind of testing I was referring to.

Got it. The nice thing with FFI is that with how much simpler it is VS method channels, the data is being passed along to the native sides just as if they were plain Dart functions. So I'm not sure how much testing is needed or even possible there, but I can definitely work on those sanity checks I mentioned in the bullets. Not sure if it's possible either to test that tapping on notifications triggers the callback", but like you said that's more of an integration test, and the example app should cover that pretty well.

One more problem I'm noticing is that CI is rejecting this because package:flutter_local_notifications_windows is not on Pub yet. We can either publish now and not actually merge it into main branch until later, or introduce a pubspec override until we're ready to merge (either way, CI won't like it until there's no override and it's on Pub).

MaikuB commented 4 months ago

The Windows package doesn't need to be published on pub.dev yet. The plugin and CI uses https://melos.invertase.dev to setup the appropriate "local" pubspec overrides when dealing with mono repos like this one. AFAIK, should be a matter of adding the Windows package to the melos workspace. See https://github.com/MaikuB/flutter_local_notifications/blob/master/melos.yaml where you can find the other packages listed

MaikuB commented 4 months ago

Something else that came to mind regarding unit tests is that checking other FFI-based plugins can provide some pointers based on what they've done

Levi-Lesches commented 4 months ago

@MaikuB I've added unit tests, configured things with melos to the best of my knowledge, and updated the example some more. Now it builds into a .msix file, which should help showcase the how to do that.

If you need the MSIX for yourself I've uploaded one here. Let me know if you need anything else to review.

Levi-Lesches commented 4 months ago

Another update: I added docs to both READMEs, the one on the _windows repository and the one on the main repository.

I also managed to remove the dependency on Flutter for the Windows package and platform interface, which means anyone can use it for Dart-only applications! I can see this being useful for CLI cases

A note on unit tests, they are run concurrently, but the logic isn't meant to be thread-safe. That leads to two things:

a flaky test with updating notifications, because sometimes the notification to be updated may be cancelled by a different test. I added 5 retries so it still passes. It only fails occasionally and its only ever needed 1 retry
To fix this, I tried running dart test -j 1, but this still runs tests in different threads/isolates.
I added a file, bin/test.dart that shows an interesting crash that involves two isolates running two tests. Not sure what to do with it but I left it in

I don't think we need to fix the failing test because the retries are enough and no user is going to be using this library in that way. Just something to be aware of.

MaikuB commented 4 months ago

Thanks will try to take a look through this weekend. Whilst I have a PC, I'd prefer to get this setup on my MacBook Pro by having Windows 11 ARM running via Parallels. Do you happen to know if this would work with Flutter?

Levi-Lesches commented 4 months ago

Seems they support building standard x64 executables and running in emulation mode. Here's the issue for building arm64 executables: https://github.com/flutter/flutter/issues/62597

I saw the ci issues, I'll get to those shortly

MaikuB commented 3 months ago

Thanks. I was able to get this up and running though. I may not get to review everything this weekend so to will leave comments as I go through it

MaikuB commented 3 months ago

FYI I've noticed that when I debug the example app that the following can be seen in debug output

Launching lib\main.dart on Windows in debug mode...
√ Built build\windows\x64\runner\Debug\example.exe
Connecting to VM Service at ws://127.0.0.1:54517/CrhXIgkgKHg=/ws
Unable to parse JSON message:
The document is empty.

════════ Exception caught by services library ══════════════════════════════════
The following assertion was thrown during a platform message callback:
Attempted to send a key down event when no keys are in keysPressed. This state can occur if the key event being sent doesn't properly set its modifier flags. This was the event: RawKeyDownEvent#f2796(logicalKey: LogicalKeyboardKey#f3b7b(keyId: "0x10000010a", keyLabel: "Num Lock", debugName: "Num Lock"), physicalKey: PhysicalKeyboardKey#5b142(usbHidUsage: "0x00070053", debugName: "Num Lock"), repeat: false) and its data: RawKeyEventDataWindows#32fe2(keyCode: 144, scanCode: 57413, characterCodePoint: 0, modifiers: 0)
'package:flutter/src/services/raw_keyboard.dart':
Failed assertion: line 862 pos 7: 'event is! RawKeyDownEvent || _keysPressed.isNotEmpty'

Either the assertion indicates an error in the framework itself, or we should provide substantially more information in this error message to help you determine and fix the underlying cause.
In either case, please report this assertion by filing a bug on GitHub:
  https://github.com/flutter/flutter/issues/new?template=2_bug.yml

When the exception was thrown, this was the stack:
#2      RawKeyboard.handleRawKeyEvent (package:flutter/src/services/raw_keyboard.dart:862:7)
#3      KeyEventManager.handleRawKeyMessage (package:flutter/src/services/hardware_keyboard.dart:1161:30)
#4      BasicMessageChannel.setMessageHandler.<anonymous closure> (package:flutter/src/services/platform_channel.dart:235:49)
#5      _DefaultBinaryMessenger.setMessageHandler.<anonymous closure> (package:flutter/src/services/binding.dart:581:35)
#6      _invoke2 (dart:ui/hooks.dart:344:13)
#7      _ChannelCallbackRecord.invoke (dart:ui/channel_buffers.dart:45:5)
#8      _Channel.push (dart:ui/channel_buffers.dart:135:31)
#9      ChannelBuffers.push (dart:ui/channel_buffers.dart:343:17)
#10     PlatformDispatcher._dispatchPlatformMessage (dart:ui/platform_dispatcher.dart:750:22)
#11     _dispatchPlatformMessage (dart:ui/hooks.dart:257:31)
(elided 2 frames from class _AssertionError)
════════════════════════════════════════════════════════════════════════════════

MaikuB commented 3 months ago

Putting aside how there's a way to provide XML, does this "implement" the entire that consumers can use the constructor to specify all the available options that the Windows SDK provides. Some examples that come to mind are how to set a hint crop or hero image like shown at https://learn.microsoft.com/en-us/windows/apps/design/shell/tiles-and-notifications/adaptive-interactive-toasts?tabs=appsdk

Levi-Lesches commented 3 months ago

Putting aside how there's a way to provide XML, does this "implement" the entire that consumers can use the constructor to specify all the available options that the Windows SDK provides.

Just to clarify, you're asking if there are Dart options for all the XML options? Yes! If something's missing I'd be happy to add it, but I went through the entire XML schema and hand-translated everything. The demo app, under Windows-specific examples, showcases almost all of these. Eg,

how to set a hint crop

WindowsImage.crop

or hero image

WindowsImagePlacement.hero.

Levi-Lesches commented 3 months ago

FYI I've noticed that when I debug the example app that the following can be seen in debug output

This is actually a common Flutter Desktop (at least, Windows) error I keep getting in all my apps, even fresh ones. I'm not sure exactly what causes it but it seems to appear more when I use the keyboard during app launch, hot reloads, or hot restarts, and that makes me think it's a debug mode problem. I wasn't able to find useful info on it online but it is an assertion which will get compiled out in release builds

MaikuB commented 3 months ago

Just to clarify, you're asking if there are Dart options for all the XML options?

Yep that's what I mean. Thanks for clarifying as it looks like I missed so will need to check the XML options more thoroughly

Levi-Lesches commented 2 months ago

@MaikuB Addressed all comments, this should be ready to go now

MaikuB commented 2 months ago

Thanks for making the changes. I left one comment. Other thing was I wasn't too sure if you were done addressing the visibility of methods to with XML conversion. Since you mentioned it's good to go then FYI that there are still some methods that are publicly visible. I've commented on the parts that I could see are still visible to be more explicit. Apologies for spam on that but though it help make them easier to find. FYI that the way to verify is to follow the steps here to generate the API docs for the Windows plugin.

Just noticed that there's also errors on the dependency/version conflict that wasn't there before. Presumably ffigen got bumped that it's no longer compatible with what was stated before on Dart 3.1 being the minimum

Edit: when I checked the API docs for the Windows plugin and Linux one, I've noticed an existing problem where the docs for the stub is what is shown. Not a showstopper for the PR right now as it's an existing problem already that I've not seen anyone raise with the Linux implementation. Not sure how to solve this one but open to ideas. Off the top of my head, I suspect either the stub needs to have the same API docs that would be a bit of a hack or see if there's a way that only the real FlutterLocalNotificationsWindows class exists. This would delegates all the API calls to another class and conditional import logic based on support FFI determines if said class is a stub or real one. I've created a PR for fixing the Linux one that can be used to see what I mean. Be keen to know what you think. I've verified this by temporarily adding a web target to the example and confirmed that running flutter build web worked

Levi-Lesches commented 2 months ago

when I checked the API docs for the Windows plugin and Linux one, I've noticed an existing problem where the docs for the stub is what is shown. Not a showstopper for the PR right now as it's an existing problem already that I've not seen anyone raise with the Linux implementation. Not sure how to solve this one but open to ideas.

When a subclass overrides a member, that member inherits its docs from the overriden version in the parent class, so long as docs are not explicitly provided on the override itself. This seems to be why the Linux version shows the stub docs -- the stub's overrides all have doc comments. To fix this, we should just not provide documentation on the stub class and let in inherit directly from the base class.

That just leaves the documentation on the class itself. I think the simplest fix would then be to copy the base class's documentation onto the stub class, which is a lot less work than copying every single member. So now:

all three classes (base, stub, and FFI) all have the same one-liner on the class declaration itself
any time a method is overriden, docs are not provided, and are instead inherited from the parent class
the plugin class shown on the docs site is the stub class, with comments from the base class
there is no way to get DartDoc to show the FFI code snippets when using conditional imports, only the stub snippets

Levi-Lesches commented 2 months ago

Just noticed that there's also errors on the dependency/version conflict that wasn't there before. Presumably ffigen got bumped that it's no longer compatible with what was stated before on Dart 3.1 being the minimum

Yup, from package:ffigen v10.0.0 (currently at 13.0.0):

Stable release targeting Dart 3.2 using new dart:ffi features available in Dart 3.2 and later.

Those specific features aren't used by us in this exact case, but the features from ffigen 10.0 -- 13.0 (soon to be 14.0) will be, especially exhaustive enum checking. I think it would be worth bumping the minimum SDK version to 3.2. Now that we've passed 3.0, I'm not sure I see much risk in bumping the minor versions, and most other packages tend to stay current with the latest Dart releases. I bumped it in my latest commit so it should work now

Levi-Lesches commented 2 months ago

Thanks for making the changes. I left one comment. Other thing was I wasn't too sure if you were done addressing the visibility of methods to with XML conversion. Since you mentioned it's good to go then FYI that there are still some methods that are publicly visible. I've commented on the parts that I could see are still visible to be more explicit. Apologies for spam on that but though it help make them easier to find. FYI that the way to verify is to follow the steps here to generate the API docs for the Windows plugin.

I actually figured it would probably be more convenient to leave them public. Since we offer a way to show notifications as raw XML, this would allow developers to build that XML using elements from the public API. Let's say they want to use their own <image>, they can still build the rest of the notification XML using .toXml() of everything else. It's not so important to me but I figured it would only reduce duplicate work and make custom XML easier. I'm not worried about stability or breaking changes because these methods are anyway compliant with the official Windows APIs which aren't breaking anytime soon.

If we really wanted to keep everything private, I could move them all to extensions, and then hide them in the exports section. But I feel like that's going to extra lengths to prevent possibly useful code from being used or seen. Not to mention, having the code snippets available in the docs site would also be useful to those looking to build their own XML.

Regarding the naming convention, I switched everything to .buildXml()

MaikuB commented 2 months ago

When a subclass overrides a member, that member inherits its docs from the overriden version in the parent class, so long as docs are not explicitly provided on the override itself.

Good callout as I admittedly forgot about that. Works for me and will update the approach used for Linux as well. Thanks!

Those specific features aren't used by us in this exact case, but the features from ffigen 10.0 -- 13.0 (soon to be 14.0) will be, especially exhaustive enum checking. I think it would be worth bumping the minimum SDK version to 3.2. Now that we've passed 3.0, I'm not sure I see much risk in bumping the minor versions, and most other packages tend to stay current with the latest Dart releases. I bumped it in my latest commit so it should work now

Thanks though building for 3.13 will fail as the main plugin itself has a minimum of 3.13 so that and pipeline would need updating as part of the PR too. I've withheld bumping for this plugin for a while and would still need to be careful of it in the future as it could affect the minimum supported OS versions

If we really wanted to keep everything private, I could move them all to extensions, and then hide them in the exports section

Yep would like to keep these hidden. These methods depend on an existing object passed through so AFAICT there's some prerequisites on what needs to be done before calling these build methods. When I was referring to breaking changes, it was more around how if these methods are part of the public API surface, updates to them (e.g. renaming) are considered breaking changes. The hiding is to help reduce the burden on a maintenance perspective. As for your point on hide, if all the extensions were moved to notification_to_xml.dart then they will be hidden without extra code. This is because the approach to determine what is publicly visible is done by determining which implementation files in the src folder are exported

Levi-Lesches commented 2 months ago

Thanks though building for 3.13 will fail as the main plugin itself has a minimum of 3.13 so that and pipeline would need updating as part of the PR too. I've withheld bumping for this plugin for a while and would still need to be careful of it in the future as it could affect the minimum supported OS versions

Bumped pubpsec.yaml and GitHub workflow environments to use Dart 3.2 and Flutter 3.16 (which comes with Dart 3.2)

Yep would like to keep these hidden.

Got it, moved all that to extensions in lib/src/details/xml that are not exposed publicly. It caused an issue in two cases where I use subclasses, since making buildXml a member of the parent class would make it public. I tried using @internal from package:meta, but then I decided to seal their abstract classes and use a switch case over the subtypes.

On another note, I restricted the tests to run on Windows only, but the current workflows are running on ubuntu-latest. I'll leave that to you to handle since I think Windows CI is billed at a 2x rate compared to the Linux CI. But at least now, dart test on ubuntu-latest will skip the Windows tests automatically, and we can run the tests locally for now. I'm actually not even sure how notification privileges will work on GitHub servers. I don't see a specific issue, but it sounds like one of those things that could work differently compared to, say, my laptop.

abdelaziz-mahdy commented 2 months ago

Thanks though building for 3.13 will fail as the main plugin itself has a minimum of 3.13 so that and pipeline would need updating as part of the PR too. I've withheld bumping for this plugin for a while and would still need to be careful of it in the future as it could affect the minimum supported OS versions

Bumped pubpsec.yaml and GitHub workflow environments to use Dart 3.2 and Flutter 3.16 (which comes with Dart 3.2)

Yep would like to keep these hidden.

Got it, moved all that to extensions in lib/src/details/xml that are not exposed publicly. It caused an issue in two cases where I use subclasses, since making buildXml a member of the parent class would make it public. I tried using @internal from package:meta, but then I decided to seal their abstract classes and use a switch case over the subtypes.

On another note, I restricted the tests to run on Windows only, but the current workflows are running on ubuntu-latest. I'll leave that to you to handle since I think Windows CI is billed at a 2x rate compared to the Linux CI. But at least now, dart test on ubuntu-latest will skip the Windows tests automatically, and we can run the tests locally for now. I'm actually not even sure how notification privileges will work on GitHub servers. I don't see a specific issue, but it sounds like one of those things that could work differently compared to, say, my laptop.

i would like to note that public repos actions are free, so i think the rate is not a problem? https://docs.github.com/en/billing/managing-billing-for-github-actions/about-billing-for-github-actions#about-billing-for-github-actions

MaikuB commented 2 months ago

@Levi-Lesches sorry for late response, was recovering from surgery. It looks like there's still a conflict. Based on my understanding of the constraints set by ffigen 13.0, this actually needs Flutter 3.19/Dart 3.3 (reference: table under stable at https://docs.flutter.dev/release/archive). Are you able to help resolve this

I'm actually not even sure how notification privileges will work on GitHub servers.

I could be mistaken but presumably as they're unit tests, the priviledges to be concerned about. Given what @abdelaziz-mahdy has said, you think you could push an update to have the tests run on Windows? On a related note, is it possible to make updates so that so that the example app is built on Windows as part of the GitHub workflow too? Would require updates to melos.yaml and validate.yml

I've also noticed there's a lot of files with changes picked up if I run dart format . from the root of the repo. Are you able to run that (or melos run format) and push the changes? I'll look into how to make this smoother separately in another PR to help others in the future (e.g. see if the workflow can help catch this)

Edit: I've merged in https://github.com/MaikuB/flutter_local_notifications/pull/2398 that should help highlight formatting issues in the future

Levi-Lesches commented 2 months ago

was recovering from surgery

Sorry to hear, hope that's going well 🙂

Based on my understanding of the constraints set by ffigen 13.0, this actually needs Flutter 3.19/Dart 3.3 (reference: table under stable at https://docs.flutter.dev/release/archive). Are you able to help resolve this

That's annoying, I checked the changelog and found mention of 3.2 but not 3.3! I changed everything to Dart 3.3 and Flutter 3.19 now.

I could be mistaken but presumably as they're unit tests, the priviledges to be concerned about.

I didn't mock everything out, as I was interested in maintaining tests that the XML does in fact conform to the Windows API. So the unit tests actually do send full XML to the Windows API and try to send notifications. If this doesn't work on the server, I'd still like to keep them, but maybe we'll just run them manually when changing XML code and configure the CI to only run "safe" tests. Speaking of, I added a global retry: 5 to all the tests, given that there are some concurrency issues.

you think you could push an update to have the tests run on Windows? On a related note, is it possible to make updates so that so that the example app is built on Windows as part of the GitHub workflow too? Would require updates to melos.yaml and validate.yml

Hit an annoying issue where "skip these tests on Windows" was actually causing Linux tests to fail. Filed https://github.com/dart-lang/test/issues/2277 to propose to change that, but in the meantime I exclude _windows in the standard unit tests and run it separately as test:unit:windows, only on windows-latest. Needs another workflow approval.

I've also noticed there's a lot of files with changes picked up if I run dart format . from the root of the repo. Are you able to run that (or melos run format) and push the changes?

Done and pushed

On another note, I had previously tried to make the Dart package completely separate from Flutter so it can be used in CLI apps. I still believe there is value to that, but when running dart pub lish -n we get an error that is never an issue in practice:

Package validation found the following error:
* pubspec.yaml allows Flutter SDK version 1.9.x, which does not support the flutter.plugin.platforms key.
  Please consider increasing the Flutter SDK requirement to ^1.10.0 (environment.sdk.flutter)

  See https://flutter.dev/docs/development/packages-and-plugins/developing-packages#plugin

Basically, even though having a flutter: section in the Pubspec of a non-Flutter package is harmless, theoretically, someone can use a very early version of Flutter (1.9.x) which would have issues parsing this section. Really, this can never happen because we demand an sdk: ^3.3 and Flutter 1.9 is pre null-safety. So Pub wants us to demand flutter: ^1.10.0, but that means declaring a dependency on Flutter at all, which I was really hoping to avoid. See also this issue.

My recommendation is to run dart pub lish -n first, see if there are any warnings besides the above, and address them first. Then, once this is the only warning left, running dart pub lish --skip-validation to ignore the warning, and preserve CLI ability.

MaikuB commented 2 months ago

I'll take a look and come back to you on these but thought I'd reply in case you missed it since it wasn't brought up in your last message and mention that builds are failing due to the meta version specified

Levi-Lesches commented 2 months ago

Good eye -- fixed

MaikuB commented 2 months ago

Sorry to hear, hope that's going well 🙂

Thanks minor surgery and still bit of discomfort but all good besides that :)

On another note, I had previously tried to make the Dart package completely separate from Flutter so it can be used in CLI apps.

I forgot to ask but is this realistically possible? In the case of the platform interface, this would normally be a transitive dependency. In the case of the Windows plugin, I don't know if you could build a CLI app that shows notifications on the Windows platform? Presumably what you mean is the equivalent of using Visual Studio to create a console app and referencing the Windows APIs to display notifications

My recommendation is to run dart pub lish -n first, see if there are any warnings besides the above, and address them first

This is what I get besides the Flutter one

    error - lib/src/plugin/ffi.dart:122:7 - The named parameter 'data' isn't defined. Try correcting the name to an existing named parameter's name, or defining a named parameter with the name 'data'. - undefined_named_parameter
    error - lib/src/plugin/ffi.dart:200:9 - The named parameter 'data' isn't defined. Try correcting the name to an existing named parameter's name, or defining a named parameter with the name 'data'. - undefined_named_parameter

I've taken a look at the issue you linked and conscious it's been open for a while. I do have a number of thoughts relating to this to and may be missing some additional context and other information so do let me know if I'm missing anything. If issue linked addressed and there are valid cases for CLI apps, I wonder if it'd be better for the "greater good" to go ahead with the Flutter requirement and revisit this. Those who use this plugin would be those looking to build a multiplatform apps. Other aspect of this is if the issue you raised is addressed, then this presumably requires newer version of the Dart SDK. If so, this would cause the Flutter SDK requirement to go even higher and limit the reach even further.

If it's possible to build a Dart CLI app that can show notifications on Windows then this sounds like it one target a very small audience if such an audience even exists. I'm basing this on the assumption that this implies that this is a developer who works on the Windows platform and rather than building a Windows console application, has decided to use Dart. Is this what it would mean? If so, this seems like an odd choice to me. In my experience, companies that build dedicated solutions targeting Windows would employ devs and leverage based on .NET

On a different note, it looks like Windows example is failing to built I'm not sure what's the cause there. However, I'm noticing that the integration tests are failing to run for Android. It seems as though the deprecation warning is being treated as a error but can't reproduce this locally even if I run the integration tests via melos. Given I had raised a PR recently to update one of the workflows and didn't have this issue, I wonder if something got changed that impacted this?

MaikuB commented 2 months ago

It's been a while since I've worked on Windows apps so didn't get to spend much time on it but ran into the following problems relating my previous post

I had trouble getting a console app to create a notification. I was hoping to copy some of the code for the Windows App SDK samples for showing notifications to achieve this but trouble with having appropriate namespaces and NuGet packages resolve properly
I tried to create Dart CLI app by creating a separate folder in the cloned repo, reference only the Windows plugin, bootstrap melos etc. Upon trying to run the CLI app, I saw Error: Dart library 'dart:ui' is not available on this platform. as an error message. It looks like this happens as the dependencies the plugin eventually need it and that's only available for apps that depends on Flutter

Based on the above it looks like trying to support CLI apps isn't actually possible, at least not in the current state either

gryznar commented 1 month ago

@Levi-Lesches thank you for your work on this! I highly appreciate Windows support :)

MaikuB commented 3 weeks ago

@Levi-Lesches no rush and could be that you've been busy. Wanted to ping in case you missed some comments/questions I left. Looks like @autoantwort left a comment on the PR as well

Edit: I also wanted to nudge as I'll be going on holidays in a few weeks time

Levi-Lesches commented 2 weeks ago

@MaikuB Thanks for the ping, I was kinda going through it with some personal matters.

I fixed a bug that was causing Windows builds to fail before a certain Windows 11 version (ie, Windows 10)
Merged master and resolved all merge conflicts
Ran all tests on my end, including Android, so hopefully the workflows should stay green now
I reverted the whole Flutter/pure Dart thing. I'm a bit of a purist but that unresolved Pub issue is really annoying

Should be ready to merge now! Note that it depends on _platform_interface: 8.1.0, not 8.0.0, because my branch has changes that are not yet published, so both will need to be published to Pub

MaikuB commented 1 week ago

Thanks @Levi-Lesches. I'll be trying to get the current prerelease out first and then come back to this one. In the interest of time. I may push directly to this fork if there are merge conflicts to resolve afterwards due to this

MaikuB commented 1 week ago

@Levi-Lesches looks like the Windows build tasks failed. You able to see what the issue is?

Levi-Lesches commented 1 week ago

The error is definitely new to me and doesn't occur when I run on my machine. From Googling around, the error FileTracker : error FTK1011: could not create the new file tracking log file seems to be linked to the Windows file length limit...

EDIT 1 Seems this is the first time the CI is running since we added Windows builds to it, so this could be the issue

EDIT 2 @MaikuB Since every workflow run requires your approval, can you add some steps between checkout and build in the windows build jobs to rename the directories to something much shorter? That should fix the issue

MaikuB commented 1 week ago

@Levi-Lesches the approval is need for first-time contributors and in your case, it's due to the PR not having being landed. I won't get to make these changes for a while as I've only time to try to land https://github.com/MaikuB/flutter_local_notifications/pull/2442. If you have time, you may be able to test your proposal by temporarily changing the workflow (e.g. run on push) and see how it fixes the issue. A shame to see the file/path length limit is still a PITA for Windows :(

Edit: looks like this is related to https://github.com/actions/runner/issues/1676. With the issue still being open, I'm not sure if there are solutions available

Levi-Lesches commented 1 week ago

While it's true that the base path is flutter_local_notifications\flutter_local_notifications and cannot be changed, I'm experimenting with changing flutter_local_notifications\example into f\e, which should save some vital characters

@MaikuB GitHub isn't handling daylight savings properly on my end, so my comments now are actually being sent upwards in the chat! Anyway, I fixed the issue now, see this which has the fix vs this which does not. Try running it again now. I also fixed the merge conflict

Levi-Lesches commented 1 week ago

Okay, @MaikuB I fixed the issue and merged, try now

EDIT: This message got messed up due to DST, I actually sent it later in the thread 🤷‍♂️

Levi-Lesches commented 1 week ago

@MaikuB So close! Not sure if you missed the CI turning green or want to wait for next week on this, so I'll just drop a reminder. Also not sure if you noticed the chat going wonky during the shift to DST too or if that was just on my end, so I figured I'd leave a message that can actually be at the bottom of the thread this time

MaikuB commented 1 week ago

When I checked after your message, it was still red but the results may have been cached. I still need to retest before merging just in case.

Regarding the timing issue on GitHub, yeah I noticed it but thought it was just me as I've seen a similar issue on Slack that was to do with Chromium browsers but sounds like it was a general problem

Levi-Lesches commented 1 week ago

Thanks, lmk if you need anything else. On the bright side... I guess we know now that the scheduling features on Windows don't throw crazy errors during DST transitions!

Levi-Lesches commented 4 days ago

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

Levi-Lesches commented 4 days ago

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 commented 4 days ago

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 commented 4 days ago

Merged in now. Thanks a lot! Happy to take PR on the example app but note I'll do one PR around relating to https://github.com/MaikuB/flutter_local_notifications/blob/697b00745585aeda488cc8f21024c50acfa48c09/flutter_local_notifications/lib/flutter_local_notifications.dart#L2. 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 commented 4 days ago

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 https://github.com/MaikuB/flutter_local_notifications/pull/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 commented 4 days ago

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 commented 4 days ago

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

MaikuB / flutter_local_notifications