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] Linux support #1208

Merged
merged 24 commits into from Jul 26, 2021
Merged

[flutter_local_notifications] Linux support #1208

merged 24 commits into from Jul 26, 2021

Conversation

proninyaroslav
Copy link
Contributor

@proninyaroslav proninyaroslav commented Jun 25, 2021

The Desktop Notifications Specification is used as an implementation.

I have separated the Linux plugin package from the main plugin package. This is necessary to correctly implement the Dart version of the plugin without using native code. For example, dbus package is used to communicate with the Desktop Notifications Specification and a few more auxiliary packages that allow to access native methods/environment.
Once this pull request is merged in the new version of the plugin, change the relative path to the package in pubspec.yaml: https://github.com/proninyaroslav/flutter_local_notifications/blob/ddc57043dcb6d68bd94d48b9fc601753ff4f899c/flutter_local_notifications/pubspec.yaml#L15

Implemented features:

  • initialize() method and LinuxInitializationSettings.
  • show() method and LinuxNotificationDetails.
  • cancel(), cancelAll() methods.
  • getCapabilities() method, which returns some of the capabilities implemented by the system notification service. Please see Desktop Notifications Specification#Backwards Compatibility.
  • getSystemIdMap() method, that returns a Map with the specified notification id as the key and the id, assigned by the system, as the value. Because by specification, the system itself generates notification id, an [app id: system id] map is used, which is cached within the user session.

Non-implemented features:

  • Notification actions. It's possible to implement them, but I didn't do this, because there is still no established/official API in the plugin. For example, one of the possible implementations that is not yet upstream WIP: [flutter_local_notifications ] Notification actions for iOS, macOS, Android #880.
  • Scheduled/pending notifications. As mentioned here [flutter_local_notifications] Linux support #1208 (comment), currently, the scheduler implementation is fraught with difficulties, because Linux doesn't have an appropriate API.
    As a possible option, we can consider interacting with systemd (similar to launchd on macOS) via D-Bus API of systemd (as done in the Desktop Notifications Specification). This will allow the scheduler to run as a system service. I haven't yet investigated how realistic it's to implement it in the plugin without changes from the application developer/user side.
  • getNotificationAppLaunchDetails(). To respond to notification after the application is terminated, the application should be registered as DBus activatable (see DBusApplicationLaunching for more information), and register action before activating the application. This is difficult to do in a plugin because plugins instantiate during application activation, so getNotificationAppLaunchDetails can't be implemented without changing the main user application.

Testing the Desktop Notifications Specification in real-world conditions is difficult, because different Linux distributions and DE may or may not implement some of the specification things. For example, on GNOME, you can't set a timeout, custom sound, or screen location. Therefore, I wrote extensive tests that validate the request setmantics and format required by the Desktop Notifications Specification. For real world testing of notifications, I would recommend using different DEs such as GNOME (e.g Ubuntu or Fedora), KDE (e.g Kubuntu), XFCE (e.g Xubuntu), MATE (e.g Ubuntu MATE), etc.

I also ask you to pay attention to these two methods:
https://github.com/proninyaroslav/flutter_local_notifications/blob/ddc57043dcb6d68bd94d48b9fc601753ff4f899c/flutter_local_notifications_linux/lib/src/typedefs.dart#L5
https://github.com/proninyaroslav/flutter_local_notifications/blob/ddc57043dcb6d68bd94d48b9fc601753ff4f899c/flutter_local_notifications_linux/lib/src/helpers.dart#L9
Because I split the Linux plugin version into a separate package, I had to copy these two methods for the plugin to work correctly. It might be worth leaving that as it is. Although you might consider moving the common methods into a separate package.

gnome_linux_notification
kde_linux_notification
2021-07-11 11-11-08

@proninyaroslav
Copy link
Contributor Author

proninyaroslav commented Jun 25, 2021

@MaikuB
At the moment I'm planning on using libnotify and C++. But there is an option to use DBus package, as is done in this project: https://github.com/canonical/desktop_notifications.dart. This will allow to write the Linux part entirely in Dart. What do you think about it?
Similarly, we can use win32 package for notifications, without using native C++ code, only Dart.

@MaikuB
Copy link
Owner

MaikuB commented Jun 28, 2021

Is it possible to schedule notifications on Linux? I'm not familiar with Linux but my research shows that it's possible but wasn't done in #888. The reason I bring this up is I don't know if you'd need to go with the C++ approach to do so.

I'm also not familiar with what the performance impact will be like but if it's minimal enough that Canonical themselves are happy to use DBus and it suits the purposes then that's fine. Note that I'm not familiar with the Linux APIs and it's been a long time since I looked at C++ so DBus would be my preference from maintenance perspective. On that note, if you're looking at C++, what could also work is for the Linux implementation is hosted and published separately and it becomes an endorsed implementation.

@proninyaroslav
Copy link
Contributor Author

@MaikuB

Is it possible to schedule notifications on Linux?

  1. Without using native code - using the Cron scheduler. Сron management is generally seen as a systems administration task, and not a developer task, because the scheduler is configured through the configuration file. I would not rely on such specific things that don't have an API.
  2. Using the native code - GLib, which is used in Flutter. It has a g_timeout_add method, for example. But the problem is that this timer is alive while the process is alive. If the process is killed, then the timer will not work. One of the options is to save the data to disk in order to restart the timer after restarting the app, but if the app is not restarted, then this doesn't make any sense.

In any case, I don't want to implement scheduling at the current stage, because none of the options is optimal. The option (2) takes place, but it doesn't guarantee that the notification will ever be shown, since it requires a live process.

I'm also not familiar with what the performance impact will be like but if it's minimal enough that Canonical themselves are happy to use DBus and it suits the purposes then that's fine.

I originally planned to use libnotify (C++ library) as it is the "standard" notification library in Linux. But I noticed that it has some limitations, for example, it's impossible to get the notification ID, which means we can't save ID to the disk and close the notification after restarting the app, because everything is stored in memory. But DBus access seems to be much more powerful.
libnotify also uses DBus, as the only standard way to show notifications on all GNU/Linux distributions is the Desktop Notifications Specification. DBus is just a way to interact with it. If we use dbus package, then in fact it is just a wrapper in which we simply pull calls the native code using Dart FFI. Similarly, we can write our own C++ code and also make DBus calls.

Based on my thoughts, I see two ways to implement the plugin. Which one is more preferable?

  1. Use dbus package without resorting to C++ (for now), because at this stage I don't plan to implement scheduled notifications.
  2. Use C++ to access DBus, with the prospect of implementing scheduled notifications or other features that will require native code.

@MaikuB
Copy link
Owner

MaikuB commented Jun 28, 2021

My own preference would be dbus so as to not have to deal with C++, particularly when there isn't a good option when it comes to being able to schedule notifications that will fire when expected even if the app has been killed. Not sure if there are other pros and cons to be aware that you could enlighten me on though. For what's it worth, I can see some of the Flutter Community Plus Plugins are using dbus as well

@proninyaroslav
Copy link
Contributor Author

proninyaroslav commented Jun 28, 2021

@MaikuB

particularly when there isn't a good option when it comes to being able to schedule notifications that will fire when expected even if the app has been killed

Yes, it's common practice in Linux development to run a daemon - a background process that starts with the system (or starts later by user). But this is clearly not the responsibility of the Flutter plugin, this decision is up to the app developer.

Okay, so I'll go into implementing the Dart version with dbus package. Porting the code from Dart to C++ shouldn't be difficult if the need arises later, because in both cases we'll use the Desktop Notifications Specification and DBus to display notifications. On the other hand, this is not required, because you can always write the missing part in C++ without completely rewriting the old code. In both cases, we don't lose anything.

@proninyaroslav
Copy link
Contributor Author

@MaikuB
Hi! I ran into a specific problem: the fact is that the Desktop Notifications Specification doesn't imply the use of a specified ID. When I send a notification, I receive a system ID in response, which is unique within the session (as long as the PC user is logged in). This ID can be used to replace the notification or close it.

Currently I see three ways out of the situation:

  1. Change the flutter_local_notifications_platform_interface API. Allow the plugin to return the ID that was assigned by the system. The developer will be able to save it to cancel or replace the notification later. In this case, the developer can't control the uniqueness of the ID, because the system uses its own counter, which is valid within the session. This can be specified in the documentation.
  2. Don't change the flutter_local_notifications_platform_interface API, but make a custom show method in the Linux plugin implementation. In this case, the developer can call the resolvePlatformSpecificImplementation method and use the custom show method to get the system ID, if needed.
  3. Make in-memory Map to map the specified ID to the real ID that is assigned by the system. It makes no sense to make this map persist, because, as I said earlier, the system ID is unique within the session and after turning off the PC or re-login, it will be non-valid and will be used for other notifications in the future.

I think I'm leaning towards option (1) or (2).

@MaikuB
Copy link
Owner

MaikuB commented Jul 3, 2021

Thanks for the info. Sounds familiar from a previous PR I saw where it was persisting IDs used, which would've been option 3. I'd be leaning towards 2 or 3 but more towards 3. Going with option 1 could cause problems down the line if a similar change would need to happen down the line for other platforms. Whilst option 2 is closer to how the platform behaves, the developer experience become awkward to use in an app targeting multiple platforms. If you do a custom show method then I would guess that calling the existing show method via the cross-platform plugin API would need to throw an error has well. This is why I'm leaning a lot more towards option 3. The Linux implementation of the plugin could expose the ID mapping to those that are interested via read-only view of the map (https://api.dart.dev/stable/2.12.4/dart-collection/UnmodifiableMapView-class.html) in case someone wants to use the generated IDs directly. Note that be able to implement cancelAll() would require saving the IDs in one form or another as well.

@proninyaroslav
Copy link
Contributor Author

proninyaroslav commented Jul 4, 2021

@MaikuB
Thank you for response. I will implement option (3). Most likely I will make this map persist in the tmp directory (XDG_RUNTIME_DIR, which is cleared when exiting the user session) so that if the app is closed and reopened, it can still know which notifications belong to it.

@proninyaroslav proninyaroslav marked this pull request as ready for review July 10, 2021 18:18
@proninyaroslav
Copy link
Contributor Author

proninyaroslav commented Jul 10, 2021

@MaikuB
I duplicated this message in the header of the pull request.

The Desktop Notifications Specification is used as an implementation.

I have separated the Linux plugin package from the main plugin package. This is necessary to correctly implement the Dart version of the plugin without using native code. For example, dbus package is used to communicate with the Desktop Notifications Specification and a few more auxiliary packages that allow to access native methods/environment.
Once this pull request is merged in the new version of the plugin, change the relative path to the package in pubspec.yaml: https://github.com/proninyaroslav/flutter_local_notifications/blob/ddc57043dcb6d68bd94d48b9fc601753ff4f899c/flutter_local_notifications/pubspec.yaml#L15

Implemented features:

  • initialize() method and LinuxInitializationSettings.
  • show() method and LinuxNotificationDetails.
  • cancel(), cancelAll() methods.
  • getCapabilities() method, which returns some of the capabilities implemented by the system notification service. Please see Desktop Notifications Specification#Backwards Compatibility.
  • getSystemIdMap() method, that returns a Map with the specified notification id as the key and the id, assigned by the system, as the value. Because by specification, the system itself generates notification id, an [app id: system id] map is used, which is cached within the user session.

Non-implemented features:

  • Notification actions. It's possible to implement them, but I didn't do this, because there is still no established/official API in the plugin. For example, one of the possible implementations that is not yet upstream WIP: [flutter_local_notifications ] Notification actions for iOS, macOS, Android #880.
  • Scheduled/pending notifications. As mentioned here [flutter_local_notifications] Linux support #1208 (comment), currently, the scheduler implementation is fraught with difficulties, because Linux doesn't have an appropriate API.
    As a possible option, we can consider interacting with systemd (similar to launchd on macOS) via D-Bus API of systemd (as done in the Desktop Notifications Specification). This will allow the scheduler to run as a system service. I haven't yet investigated how realistic it's to implement it in the plugin without changes from the application developer/user side.
  • getNotificationAppLaunchDetails(). To respond to notification after the application is terminated, the application should be registered as DBus activatable (see DBusApplicationLaunching for more information), and register action before activating the application. This is difficult to do in a plugin because plugins instantiate during application activation, so getNotificationAppLaunchDetails can't be implemented without changing the main user application.

Testing the Desktop Notifications Specification in real-world conditions is difficult, because different Linux distributions and DE may or may not implement some of the specification things. For example, on GNOME, you can't set a timeout, custom sound, or screen location. Therefore, I wrote extensive tests that validate the request setmantics and format required by the Desktop Notifications Specification. For real world testing of notifications, I would recommend using different DEs such as GNOME (e.g Ubuntu or Fedora), KDE (e.g Kubuntu), XFCE (e.g Xubuntu), MATE (e.g Ubuntu MATE), etc.

I also ask you to pay attention to these two methods:
https://github.com/proninyaroslav/flutter_local_notifications/blob/ddc57043dcb6d68bd94d48b9fc601753ff4f899c/flutter_local_notifications_linux/lib/src/typedefs.dart#L5
https://github.com/proninyaroslav/flutter_local_notifications/blob/ddc57043dcb6d68bd94d48b9fc601753ff4f899c/flutter_local_notifications_linux/lib/src/helpers.dart#L9
Because I split the Linux plugin version into a separate package, I had to copy these two methods for the plugin to work correctly. It might be worth leaving that as it is. Although you might consider moving the common methods into a separate package.

gnome_linux_notification
kde_linux_notification
2021-07-11 11-11-08

@MaikuB
Copy link
Owner

MaikuB commented Jul 11, 2021

Thanks for the PR. Will take me sometime to go through it. I had an Ubuntu VM setup last time but had issues in testing out the example app with changes done from another PR. Was told there's some steps to follow but wasn't clear to me what I need to do. Is that the case here too? If so, would you be able to provide guidance on steps I'd need to follow as I'm not a Linux user

@proninyaroslav
Copy link
Contributor Author

proninyaroslav commented Jul 11, 2021

Was told there's some steps to follow but wasn't clear to me what I need to do. Is that the case here too?

In this case, no further action is required. You can immediately launch the debug version and test the notifications.

@proninyaroslav
Copy link
Contributor Author

Also, in the PR header, I described in detail all the nuances and how to test it in the best way.

@MaikuB
Copy link
Owner

MaikuB commented Jul 17, 2021

I've taken a look and this is awesome. So far it looks good and I've had some minor comment but I'm holding off on "finalising a review" as I've just across this canonical/desktop_notifications.dart#2. As such, I reached out to one of team members at Canonical to see if they would be up for maintaining a Linux implementation, have it endorsed and your PR would become the basis for this (e.g. you submit a PR to their repository). Curious to know what your thoughts are as well as I think that would be in the best interest of the community to have Canonical's own backing. Not sure when I'll hear back though.

@proninyaroslav
Copy link
Contributor Author

@MaikuB
Thank you for review.

In fact, desktop_notifications is a thin wrapper around DBus calls and Desktop Notification Specification. In other words, if I used desktop_notifications inside a Linux plugin, then I would not make DBus calls myself, and simply pass arguments to the desktop_notifications instance. desktop_notifications is just trying to be a generic abstraction over the Desktop Notification Specification. If you want to use desktop_notifications as a standalone package inside flutter_local_notifications, some adaptation is needed. For example:

  • desktop_notifications exposes some details of the dbus package, while I hid them behind my own abstraction.
  • desktop_notifications is heavily tied to the dbus package, and this complicates testing, because we can't mock DBus calls.
  • desktop_notifications must also be mockable to test without any additional wrapper classes.

If these problems are eliminated, it's quite possible to use desktop_notifications, if further supporting of "raw" DBus calls inside flutter_local_notifications seems difficult.

If you want to combine the codebases of both plugins, then in fact my PR does this, because I use the Desktop Notification Specification as well as desktop_notifications (remember that this package is just a thin wrapper around the specification).

@MaikuB
Copy link
Owner

MaikuB commented Jul 17, 2021

@proninyaroslav thanks, I'm aware of that. This is more on if they'd be keen on hosting and maintaining the flutter_local_notifications_linux plugin

@proninyaroslav
Copy link
Contributor Author

@MaikuB
I think they shouldn't have any problems, if of course they are interested in it.

@MaikuB
Copy link
Owner

MaikuB commented Jul 17, 2021

Yep that's what I want to find out :)

@robert-ancell
Copy link
Contributor

robert-ancell commented Jul 19, 2021

Thanks for reaching out @MaikuB! I'm the Canonical developer who has been working on dbus / desktop_notifications etc.

Our motivation is to enable Flutter developers on Linux, and for that reason I've been working from the bottom up of the stack to make it easier for everyone to access to the services on Linux. It's great to see you've found the dbus library useful here. We're not currently working on any integration for flutter_local_notifications, but we would like it to exist so it's great to see your work here @proninyaroslav. There's a lot to build/maintain, so we always like seeing others do the work.

desktop_notifications is intended to be a dependency for any Dart package that needs to access the XDG notifications spec. It's not intended to include any higher level code than that - that's left for projects like this. Regarding your specific issues:

  • It's wrapped as lightly as possible so may not be in the ideal form you want to consume it it. I wouldn't expect a project like flutter_local_notifications to expose any details of desktop_notifications, rather just use it to make the function calls / signal handling simpler.
  • All our D-Bus based libraries use locally run D-Bus servers to mock up services. See the desktop_notifications tests for an example of how we do this. I tend to prefer to do the mockups this way as it means the library doesn't require any test specific interfaces and this ensures that all codepaths are being tested. You should be able to make similar mockups in your code.

Do please file issues if you have any trouble using desktop_notifications or features that we should add. However, should you not find desktop_notifications useful in your case, that's fine too! As long as people get working notifications, we're happy.

@MaikuB
Copy link
Owner

MaikuB commented Jul 19, 2021

Thanks so much for replying on this @robert-ancell. I realise it's a lot to maintain. As I saw an issue open on the desktop_notifications repository that you had upvoted as well, I thought it might be worth reaching out in case it meant it was under consideration. This helps clarify a lot and will proceed with looking at this PR :)

@prateekmedia

This comment has been minimized.

@proninyaroslav
Copy link
Contributor Author

proninyaroslav commented Jul 21, 2021

@prateekmedia
What are the benefits other than an extra layer of abstraction? Now flutter_local_notification_linux fully complies with Desktop Notification Specification, using only dbus package.

@prateekmedia

This comment has been minimized.

@MaikuB
Copy link
Owner

MaikuB commented Jul 21, 2021

@prateekmedia That point was with regards to what was written by @robert-ancell earlier

@prateekmedia
Copy link

Oops, I think I misunderstood the discussion.

@proninyaroslav
Copy link
Contributor Author

@prateekmedia
What 4k lines are we talking about? The base plugin code is only ~300 lines in size, which is roughly the size of the lines in desktop_notifications (370). The rest of the code is data classes (LinuxNotificationDetails and etc), tests, examples.

Copy link
Owner

@MaikuB MaikuB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't find any major issues. Left some comments/questions :)

@MaikuB MaikuB merged commit d1cd8f2 into MaikuB:master Jul 26, 2021
@MaikuB
Copy link
Owner

MaikuB commented Jul 26, 2021

Thanks so much for this. Merged it in and will do an initial release of the Linux plugin and then publish the main plugin soon after

android: androidPlatformChannelSpecifics,
iOS: iOSPlatformChannelSpecifics,
macOS: macOSPlatformChannelSpecifics);
final LinuxNotificationDetails linuxPlatformChannelSpecifics =
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@proninyaroslav I left a comment on this elsewhere that I think may have been lost but wanted to flag that I found this sound wouldn't play when I tried on an Ubuntu VM. Do you have the same problem and are you able to help with this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this is not standardized. You can get different behavior in different DEs. For example, on GNOME (which is used by Ubuntu) I was unable to achieve mp3 playback, it was causing the system to crash. I checked the /usr/share/sounds/freedesktop directory and it turns out that the system uses the Ogg format for various system sounds. Therefore, you can use ThemeLinuxSound to specify the name of the system sound (for example, ThemeLinuxSound('message')), or using .oga audio files. I think that in the example it's worth trying to replace the mp3 sound with Ogg format.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the reply. The ogg format sounds like a good idea 🙂

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.

None yet

4 participants