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 support #2349

Closed
wants to merge 55 commits into from

Conversation

Levi-Lesches
Copy link

@Levi-Lesches Levi-Lesches commented Jun 25, 2024

Hi, I'm picking this up from @kennethnym and @lightrabbit. From their work, we already have basic initialization set up. I'm referring to https://learn.microsoft.com/en-us/windows/apps/design/shell/tiles-and-notifications/adaptive-interactive-toasts?tabs=appsdk and https://learn.microsoft.com/en-us/uwp/schemas/tiles/toastschema/schema-root for the rest of my implementation.

@MaikuB, There's a small TODO below, but feel free to start reviewing!

TODO:

  • Validate message passing back to Dart
  • Check Windows version before playing audio
  • update progress bars dynamically
  • validate that headers work as intended
  • Remove title, body, and payload from the C++ side
  • Implement getActiveNotifications and any other missing methods
  • Update example to use Windows notifications as well
  • Test getActiveNotifications on Windows MSIX
  • Test pendingNotificationRequests
  • Consolidate Notification --> XML --> Notification logic
  • Check message passing in the background
  • Clean up the C++ code
  • Support multiple progress bars
  • Remove platformSpecifics from method channel
  • Remove path dependency on platform interface package
  • Prevent recurring notifications in example app when on Windows
  • Think of safer ways to handle file URIs
  • Look into GET_NOTIFICATION_APP_LAUNCH_DETAILS support
  • Test action image and image logic on exe and MSIX

Note: WindowsImage requires a Windows-formatted URI of an absolute path. Instead of putting that on the user, I accept a dart:io File object and do that internally. That marks the first use of dart:io inside this package, but:

  • it does not support web anyway (until Flutter Web support #481)
  • simply importing dart:io does not crash a build anymore, but using it does
  • the file isn't actually used until .toXml() is called, which is only called by the Windows plugin.

If, in the future, this package supports web, people can still use File in their WindowsNotificationsDetails, and the app will compile, but the absolute path will not be looked up unless they're on Windows.

@Levi-Lesches
Copy link
Author

Also, I built off the work from Kenneth and lightrabbit, but I was wondering about a v2 of this plugin with pigeon or FFI. Since this version is already functional, let's try to get this out first, but just a thought for later.

@Levi-Lesches
Copy link
Author

Levi-Lesches commented Jul 2, 2024

@MaikuB All done! I don't anticipate making any more API changes from here, maybe just some more comprehensive documentation in some spots.

@MaikuB
Copy link
Owner

MaikuB commented Jul 2, 2024

Thanks. Note I do want to prioritise a beta that includes changes like bumping the minimum Flutter version and potentially get rid of deprecated properties that are on Android side.

When I can look at this one, it's possible I reach out here on how to test as well as I've not been involved in Windows development since UWP was first introduced

@Levi-Lesches
Copy link
Author

Thanks. Note I do want to prioritise a beta that includes changes like bumping the minimum Flutter version and potentially get rid of deprecated properties that are on Android side.

Sounds reasonable, I'll do my best to keep this PR up to date with any potential merge conflicts.

When I can look at this one, it's possible I reach out here on how to test as well as I've not been involved in Windows development since UWP was first introduced

I've updated the example to showcase all the new features version is capable of and hide unsupported examples when running on Windows, so if you have one, testing is as easy as flutter run -d windows. If you can't test on Windows, let me know and I can upload a video.

Do you have an ETA on a review? No rush, because I'm using a pub override to my local copy, but I'm still thinking of possibly rewriting this as an FFI plugin (C++ logic would stay the same, but we'd drop all the serialization and method channel logic). However, I'm satisfied with how the plugin works now (adding FFI won't change the functionality), and it's about time I got back to the project that needed Windows notifications in the first place, so a rough timeline of when you want to go through this would be helpful.

@MaikuB
Copy link
Owner

MaikuB commented Jul 7, 2024

I've updated the example to showcase all the new features version is capable of and hide unsupported examples when running on Windows, so if you have one, testing is as easy as flutter run -d windows. If you can't test on Windows, let me know and I can upload a video.

I mentioned I might reach as I think your original message mentioned some limitations that relate to MSIX (?)

Do you have an ETA on a review?

If there's a plan to rewrite this to use FFI then I'd prefer to wait until that is done. Reason for this is because I maintain as a spare-time effort (e.g. check PRs on weekends) so would be better for me to avoid reviewing and test functionality twice

@Levi-Lesches
Copy link
Author

If there's a plan to rewrite this to use FFI then I'd prefer to wait until that is done.

I pinged you there, but for people following, I pushed #2366 for the FFI version. TL;DR, it's much cleaner, less Flutter magic, safer, and faster to write with automatically generated bindings. The (minor) downsides are needing a C-compatible API (no vectors or maps for data that really wants it), and notification callbacks needing Dart 3.1 or higher.

@Levi-Lesches
Copy link
Author

Closing for now with the FFI implementation in #2366. I like the code there much better, but there's an important note there about Dart versions.

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.

7 participants