[flutter_local_notifications] linux support #888

akemimadoka · 2020-11-03T13:40:09Z

show simple notification with payload with/without title and/or content
support notification with actions(buttons)
~~fetch information from notification which activates the application(not applicable due to limitations described in readme)~~
implement periodicallyShow and zonedSchedule

… on Linux

MaikuB · 2020-11-11T08:24:31Z

Thanks a lot for the PR! I'll take a look and get try to get help from the community to test it out as well

daniel-vera-g · 2020-11-11T16:09:08Z

Hey @akemimadoka are there any specific testing instructions?

I used the example in your forked PR repo and tried to run it: flutter run -d -v linux
But this failed.
I tested on Ubuntu 18.04.5 LTS
The relevant part of the log is probably at the end, nevertheless I included the whole log.
log.txt

akemimadoka · 2020-11-11T16:20:27Z

Hi @daniel-vera-g This error is caused by a low version glib(the reported error suggests that the missing function requires version 2.58), I'll test the minimum required version of glib, thanks for your testing!

jpnurmi

Nice stuff! I can see notifications popping up after creating a .desktop file. 👍

I wonder if one could possibly use a CMake rule to place a .desktop file into ~/.local/share/applications. It would make the example work out of the box, and serve as a starting point for apps that want to use the package on Linux. The downside is that it kind of pollutes user's .local/share, though.

P.S. There might be something wrong with "Schedule notification to appear in 5 seconds". The notification doesn't show up, and a couple of assertion failures get printed:

(flutter_local_notifications_example:508344): GLib-GIO-CRITICAL **: 23:25:59.245: g_application_send_notification: assertion 'G_IS_NOTIFICATION (notification)' failed

(flutter_local_notifications_example:508344): GLib-GObject-CRITICAL **: 23:25:59.245: g_object_unref: assertion 'G_IS_OBJECT (object)' failed

jpnurmi · 2020-11-11T21:35:36Z

flutter_local_notifications/linux/flutter_local_notifications_plugin.cc

+    GValue id = G_VALUE_INIT;
+    g_object_get_property(G_OBJECT(getApplication()), "application-id", &id);
+    std::string ret = g_value_get_string(&id);
+    g_value_unset(&id);
+    return ret;


this could use g_application_get_application_id()

jpnurmi · 2020-11-11T21:44:43Z

flutter_local_notifications/lib/src/platform_flutter_local_notifications.dart

+  Map<int, Map<String, LinuxNotificationButtonHandler>> _notificationButtonHandlerMap;
+  Map<int, Map<String, LinuxNotificationButtonHandler>> _periodicalNotificationButtonHandlerMap;


these could be final if they were initialized here instead of initialize()

jpnurmi · 2020-11-11T21:48:26Z

flutter_local_notifications/lib/src/platform_flutter_local_notifications.dart

@@ -660,3 +663,139 @@ class MacOSFlutterLocalNotificationsPlugin
    }
  }
 }
+
+class LinuxFlutterLocalNotificationsPlugin


there are some analysis warnings here and there in LinuxFlutterLocalNotificationsPlugin that could be easily solved 😉

jpnurmi · 2020-11-11T22:03:05Z

flutter_local_notifications/lib/flutter_local_notifications.dart

+export 'src/platform_specifics/linux/enums.dart';
+export 'src/platform_specifics/linux/icon.dart';
+export 'src/platform_specifics/linux/initialization_settings.dart';
+export 'src/platform_specifics/linux/method_channel_mappers.dart';


"method_channel_mappers" sounds like an internal helper. is it intentionally exported?

jpnurmi · 2020-11-11T22:06:36Z

flutter_local_notifications/lib/src/initialization_settings.dart

@@ -16,6 +18,8 @@ class InitializationSettings {
  /// Settings for iOS.
  final IOSInitializationSettings iOS;

+  final LinuxInitializationSettings linux;


add the same one-liner doc as the others around, to avoid affecting pub score?

jpnurmi · 2020-11-11T22:07:10Z

flutter_local_notifications/lib/src/notification_details.dart

@@ -16,6 +18,8 @@ class NotificationDetails {
  /// Notification details for iOS.
  final IOSNotificationDetails iOS;

+  final LinuxNotificationDetails linux;


same here :)

jpnurmi · 2020-11-11T22:30:14Z

flutter_local_notifications/linux/flutter_local_notifications_plugin.cc

+    return FL_METHOD_RESPONSE(fl_method_success_response_new(nullptr));
+  }
+
+  FlMethodResponse* show(FlMethodCall* call) {


this is probably the whole plugin's most important function, but it's a bit hard to follow because it's so long and does many things. would it be better to try to split it up into smaller chunks?

jpnurmi

Oops I pressed the wrong button, sorry!

MaikuB · 2020-11-11T23:17:49Z

@akemimadoka FYI I asked @jpnurmi to help review since he's being doing a lot of work for the Linux implementations for the Flutter plus plugins and I'm rusty when it comes to C/C++. If you could take a look at his review comments then that would be great :)

MaikuB · 2020-11-12T06:58:11Z

flutter_local_notifications/lib/src/platform_specifics/linux/icon.dart

@@ -23,7 +23,8 @@ class LinuxFileIcon implements LinuxIcon {
 }

 /// Represents an icon from binary data
-/// The binary data should be in an image file format which supported by glib, such as common formats like jpg and png
+/// The binary data should be in an image file format which supported by glib, 
+/// such as common formats like jpg and png
 class BytesLinuxIcon implements LinuxIcon {


This reads slight awkward. Could this perhaps be renamed to something else like ByteDataLinuxIcon? Would also make it similar to https://api.flutter.dev/flutter/dart-typed_data/ByteData-class.html

MaikuB · 2020-11-12T07:02:48Z

@akemimadoka there's a .cirrus.yml file at the root of the repo. Would you be able to add a task to the build script that would be build the Linux example app? Should be able to use the macOS task here as an example to follow as it involves switching to the dev branch to allow Flutter to build for desktop platforms

MaikuB · 2020-11-15T02:01:31Z

I'll need to setup a VM to be able to give this a spin but FYI that I'll be merging a PR soon (#907) to provide more info on the guidelines around contribution to make reviews easier going forward. This includes info on API documentation, which follows the guidelines from here. I mention this as some changes in the PR will be needed e.g. to follow this rule.

This PR will also turn on the rule around public member API documentation. Will let you know when it's merged, after which you can update your fork so that the analyser can highlight the issues it finds.

MaikuB · 2020-11-15T03:35:58Z

You should be able to update your fork to pull in the latest changes now

MaikuB · 2020-11-18T09:49:05Z

Sorry for the delay, I made some progress on getting a Ubuntu VM via Parallels on my MacBook but ran into issues with getting even the basic counter app running. A window appears but won't show the elements that it should and I'm seeing a lot GDK_IS_WINDOW (window) messages Tried both the dev and master channel but still no luck. I tried the master channel as I saw a similar issue was reported with a fix that should've landed on master. It may take some more time for me to figure out how to resolve this to give the PR a spin unless anyone here has some suggestions.

jpnurmi · 2020-11-18T09:58:22Z

This is just a guess, but perhaps either enabling 3D hardware acceleration for the VM or running Flutter in software rendering mode helps?

MaikuB · 2020-11-18T10:06:30Z

Ok looks like turning off 3D acceleration setting in Parallels helped. Thanks :)

MaikuB · 2020-11-18T10:57:02Z

Sorry another newbie question 😅
I've run into the same issue reported earlier around GLib. I installed Flutter via the snap store and looks like it's referring to GLib 2.0. How would I go about updating GLib? I'm assuming it's the steps in the CI script are what updates GTK to get a newer version of GLib along with it i.e.

- apt update
- apt install cmake ninja-build clang pkg-config libgtk-3-dev -y

Another thing is rather than having the app fail to build due to APIs not being available because of the use of older versions of GLib, is it possible to put a guard in the code around this (e.g. via a macro) to check that a minimum version of GLib is installed? If it's possible, then I would suggest to throw an exception that could be caught on the Flutter side when app calls an API from the plugin that needs a newer version of GLib and include details of the minimum version in the exception.

Would be good to also to add info in the docs on how to update glib too as it sounds like this could be a more common issue.

MaikuB · 2020-11-18T10:58:43Z

flutter_local_notifications/lib/src/platform_specifics/linux/method_channel_mappers.dart

+}
+
+extension LinuxNotificationDetailsMapper on LinuxNotificationDetails {
+  Map<String, Object> toMap() {


analysis is picking up that this should use => here

MaikuB · 2020-11-18T11:04:44Z

flutter_local_notifications/lib/src/platform_specifics/linux/notification_details.dart

+
+  /// Label of this button.
+  final String label;
+  /// Identifier of this button, will be passed to callback.


Can you run flutter format . from the root of the plugin? Regular formatting would add empty lines between properties and running the code does seem to show that some files would be changes as a result. Note that if happen to be using VS Code that you can turn format on save on, which is an option that is off by default

MaikuB · 2020-11-18T11:06:04Z

flutter_local_notifications/linux/flutter_local_notifications_plugin.cc

+    return gtk_window_get_application(GTK_WINDOW(getTopLevel()));
+  }
+
+  [[maybe_unused]] std::string getApplicationId() const {


Should this method be here? Haven't seen any reference to it

akemimadoka · 2020-11-19T03:59:52Z

Hi @MaikuB It seems on Ubuntu 18 latest version of glib on the official apt source is 2.56, lower than 2.58 which some time zone API requires. I think it could be solved by loading some symbols dynamically, and so we can show a user-friendly prompt.

MaikuB · 2020-11-19T07:12:19Z

@akemimadoka Is it possible to use an #if or #ifdef directive based on the glib version?

akemimadoka · 2020-11-19T07:46:03Z

@MaikuB Yes, it's possible, but if we check the version at runtime, we can guarantee that the product can be run on a different environment from the building one.

MaikuB · 2020-11-19T08:03:53Z

@akemimadoka What I'm proposing is that a runtime check still occurs. The goal is to still allow developers to still use the plugin even if they're using older versions of GLib as they may not care for being schedule a notification, in which case, the code shouldn't fail to compile because the plugin makes use of APIs in newer versions of GLib. This is where the use of something like an #if directive comes in. If they're calling zonedSchedule whilst on a lower version of GLib, then have the plugin throw an error on the C++ side that would end up trigger a PlatformException on the Flutter side with a message that says they need to upgrade GLib. If they meet the minimum version of GLib is met then the plugin will attempt to schedule their notification. Hope that is clearer.

Edit: in terms of code the C++ implementation for zonedSchedule would look something like

#ifdef <meets_minimum_glib_version>
 // schedule notification
#else
 // throw exception
 #endif

MaikuB

I've left some additional comments based on what I can understand from the code whilst I'm still sorting out my Linux dev environment. Main areas are around show on the C++ side handling multiple ways of scheduling notification and questions on what happens if the app is restarted as it appears that things like cancelling all notifications would fail if the app had restarted

MaikuB · 2020-11-19T07:14:26Z

flutter_local_notifications/lib/src/platform_flutter_local_notifications.dart

+      for (final LinuxNotificationButton button in notificationDetails.buttons)
+        button.buttonId : button.handler,
+    } : null;
+    return _channel.invokeMethod('show', <String, Object> {


This should be the only method that specifies show as the method being called when calling invokeMethod. This makes it easier to match the code on the Dart side and would make it less confusing for readers of the code as convention within the ecosystem has been that the string matches the name of the method. Currently other methods are also passing show.

It also prevents the show method on the C++ side from being responsible for triggering notifications in multiple ways as is the case right now. This makes it more risky to change code as a common method is being used for the various plugin APIs. I get why it was done as I did a similar thing in the past but that caused issues with maintenance. Making the change should also resolve the comment @jpnurmi raised on how the implementation of show in C++ was rather long

MaikuB · 2020-11-19T07:28:53Z

flutter_local_notifications/lib/src/platform_flutter_local_notifications.dart

+    if (_notificationButtonHandlerMap.remove(id) != null) {
+      return _channel.invokeMethod('cancel', id);
+    } else if (_periodicNotificationButtonHandlerMap.remove(id) != null) {
+      return _channel.invokeMethod('cancelPeriodicNotification', id);


Call _channel.invokeMethod('cancel', ...) for similar reasons I mentioned about how _channel.invokeMethod( 'show', ...) was called in multiple places. An additional parameter could be passed to indicate the handler should be cancelled on the Dart side.

MaikuB · 2020-11-19T08:38:50Z

flutter_local_notifications/lib/src/platform_flutter_local_notifications.dart

+
+  @override
+  Future<void> cancelAll() async {
+    for (final int id in _notificationButtonHandlerMap.keys) {


What happens if the user triggers a notification that is displayed, leaves the notification there and restarts the app? If I'm not mistaken, looks as though cancelling notifications and handling buttons would rely on having the same app session active. FYI that cancelling notifications and handling when the user taps on a notification (note: other platforms don't support action buttons at the moment) for all other platforms can be done even if the user restarted the app so if what I described is indeed how it works right now (as I'm still having some trouble with my Linux environment to test) then a solution will need to be found for Linux to behave the same. I imagine, this would potentially need to rely on persisting some information about the notification

akemimadoka · 2020-11-19T08:54:51Z

@MaikuB If we check the version at build time and a user uses it in an environment where the version of glib doesn't meet the requirement, it will fail at runtime with a not user-friendly prompt like "symbol not defined". Both solutions are not hard to implement. If the situation which I described above is not too serious, I will implement it with macros.😸

MaikuB · 2020-11-23T10:38:39Z

FYI I still have issues running the app. The furthest I got was to remove code that makes use of timezone APIs and then I was able to run the app but couldn't get a notification, which I did by running the flutter run command. I know the docs mention creating a desktop file, which I tried to do but didn't help. I may have done it incorrectly though.

Another thing is with the recent changes I've seen, it looks like information about notification ids only remain in memory and that scheduling notifications only work if the app is still running. This is what I'd expect to be able to do with around cancelling notifications and scheduling notifications

Have one or more notifications showing. Restart the app and cancelAll should remove all notifications
Schedule a notification whilst the app is running. Kill the app and the notification should appear even if the app isn't running. This applies even if this was a recurring notification.

From what I can tell from this PR, neither of these are supported. Please correct me if I'm wrong

akemimadoka · 2020-11-23T13:21:21Z

To implement these features, some limitations should be noted:

We cannot remove notifications without id by GTK API. It can be solved by using some dirty methods like post some javascript script to gnome-shell, but less portability.
As far as I know, It can be implemented by using cron or systemd, but it's obviously making portability worse.
So in my opinion I think there is a trade-off between feature and portability.

MaikuB · 2020-11-24T08:06:21Z

https://pub.dev/documentation/path_provider_linux/latest/path_provider_linux/PathProviderLinux/getApplicationSupportPath.html

We cannot remove notifications without id by GTK API. It can be solved by using some dirty methods like post some javascript script to gnome-shell, but less portability.

Yep that is I could see from the API docs. This is why I tried to ask about storing the ids. I imagine it should be possible for applications to store data that it needs. The Linux shared_preferences plugin allows for this (see https://github.com/flutter/plugins/blob/592b5b27431689336fa4c721a099eedf787aeb56/packages/shared_preferences/shared_preferences_linux/lib/shared_preferences_linux.dart#L32), which in turn calls this API. I would imagine a similar implementation could be used here to save the ids.

2. As far as I know, It can be implemented by using cron or systemd, but it's obviously making portability worse.
So in my opinion I think there is a trade-off between feature and portability.

I'm not that familiar with Linux so I'm not sure what you refer to here around portability. From a user's perspective, they shouldn't have to keep their application and device running to see a scheduled notification. This would also achieve feature parity with how other platforms work as those platforms provide ways for scheduled notifications to work regardless of the application's state. I doubt it'll be of much use if the notification can only be shown when the application is running e.g. using notifications for a periodic reminder

MaikuB · 2020-11-25T06:49:53Z

flutter_local_notifications/example/lib/main.dart

+class NotificationIdPersister extends LinuxNotificationNotifier {
+  @override
+  void onNewNotificationCreated(int notificationId) {
+    SharedPreferences.getInstance().then((SharedPreferences value) async {


To clarify, I wasn't suggesting to use the shared preferences plugin. In fact it should be avoided to minimise dependencies where possible. What I was referring is to look at where they save the info and have the plugin do the same. I would imagine these two lines of Dart code

final processName = path.basenameWithoutExtension( await File('/proc/self/exe').resolveSymbolicLinks()); final directory = Directory(path.join(xdg.dataHome.path, processName));

have an equivalent in C++

I agree, so I'm only using it in the example program, users can choose their favorite method to persist the data.
In the plugin, we are only notifying the user by LinuxNotificationNotifier.

Sorry, I was reviewing on my phone and thought the only changes would've been to the plugin so missed that this was for the example app. Note though what I was also referring to is having the plugin manage all of this. The core reason for this is goes to why a cross-platform library/SDK is typically chosen as it allows feature parity to achieved with minimal effort In other words, allowing users to get their apps and running quicker. Having the plugin manage it removes additional burden from the user. If how data is stored is that much of a concern, then that tends to be when the data needs to be stored securely and would likely require a custom implementation.

Another issue is that the plugin is now relying the on user to provide a collection of notification ids. This makes it a bit awkward as if a user is picking a library to manage notifications, I dare say the vast majority of users would be expecting the plugin to do so on its own. Having to require users to pass this info means the user becomes the source of truth (note: they could pass erroneous values) when it should really be the plugin. I also just noticed that the method for pending notification requests isn't implemented and doing so would most likely require the plugin to save the notification details anyway. If the user needed to persist this data via the LinuxNotificationNotifier and then feed it back to the plugin for the API to get the pending notification requests to work, this makes it even more awkward.

If some of what I've raised so far including being able to have scheduled notifications occur even when the app isn't running becomes too much an effort to do, feel free to close the PR. Reason being is I imagine you're doing this on your spare time so can understand if you decide to drop working on this. Though in the case of handling scheduled notifications, perhaps one way is to simply mark those methods as not being implemented. Not sure how many applications would find it useful to be able to display a notification immediately though

cc @jpnurmi: if you happen to have time, would appreciate your thoughts on what I've raised here and in the thread so far but can understand if you're not able to do so

Hi @MaikuB, sorry I've been a bit busy lately, but I'll try to catch up with what's been going on here later.

MaikuB · 2021-01-01T23:41:12Z

Closing this as changes were requested in a number of places (e.g. how scheduling notification works and having the notification ids saved to storage by the plugin itself) but haven't been actioned for a while. Feel free to re-open for further discussion

akemimadoka added 2 commits November 3, 2020 21:31

initial linux support

03a46ee

Introduce linux limitations

a1aac0e

MaikuB mentioned this pull request Nov 7, 2020

Desktop support (Linux, Windows) #746

Open

2 tasks

akemimadoka added 3 commits November 10, 2020 12:22

implement periodicallyShow

543d8c9

implement zonedSchedule

89be469

introduce limitations on implementing getNotificationAppLaunchDetails…

c3a8ad7

… on Linux

akemimadoka marked this pull request as ready for review November 11, 2020 07:23

Fix button action bugs

783a0b9

akemimadoka added 4 commits November 11, 2020 16:27

Fix icon bugs

f38460f

Use g_autoptr

5b29ec9

Code format

738d8de

Fix readme

53f631c

jpnurmi approved these changes Nov 11, 2020

View reviewed changes

jpnurmi suggested changes Nov 11, 2020

View reviewed changes

Code format, fix bugs from zonedSchedule

dd6d96b

MaikuB reviewed Nov 12, 2020

View reviewed changes

akemimadoka added 4 commits November 12, 2020 15:58

rename BytesLinuxIcon to ByteDataLinuxIcon, add ci

d7e69e3

try fixing ci

9163283

add dependencies to ci

09ecd86

add gtk dependencies

84f1683

akemimadoka force-pushed the master branch from ec8003d to 84f1683 Compare November 16, 2020 06:13

akemimadoka added 2 commits November 16, 2020 14:16

Merge remote-tracking branch 'origin/master'

15ce2da

Add documentation

b6d24d2

MaikuB reviewed Nov 18, 2020

View reviewed changes

MaikuB requested changes Nov 19, 2020

View reviewed changes

Simplify implementation and code reformat

2096542

Add callback to make users can persist notifications id after a restart

276af10

MaikuB reviewed Nov 25, 2020

View reviewed changes

MaikuB closed this Jan 1, 2021

MaikuB mentioned this pull request Jun 28, 2021

[flutter_local_notifications] Linux support #1208

Merged

		Map<int, Map<String, LinuxNotificationButtonHandler>> _notificationButtonHandlerMap;
		Map<int, Map<String, LinuxNotificationButtonHandler>> _periodicalNotificationButtonHandlerMap;

[flutter_local_notifications] linux support #888

[flutter_local_notifications] linux support #888

Conversation

akemimadoka commented Nov 3, 2020 • edited

MaikuB commented Nov 11, 2020

daniel-vera-g commented Nov 11, 2020

akemimadoka commented Nov 11, 2020

jpnurmi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpnurmi left a comment

Choose a reason for hiding this comment

MaikuB commented Nov 11, 2020

Choose a reason for hiding this comment

MaikuB commented Nov 12, 2020

MaikuB commented Nov 15, 2020 • edited

MaikuB commented Nov 15, 2020

MaikuB commented Nov 18, 2020

jpnurmi commented Nov 18, 2020 • edited

MaikuB commented Nov 18, 2020

MaikuB commented Nov 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akemimadoka commented Nov 19, 2020

MaikuB commented Nov 19, 2020

akemimadoka commented Nov 19, 2020

MaikuB commented Nov 19, 2020 • edited

MaikuB left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akemimadoka commented Nov 19, 2020

MaikuB commented Nov 23, 2020

akemimadoka commented Nov 23, 2020

MaikuB commented Nov 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaikuB commented Jan 1, 2021

akemimadoka commented Nov 3, 2020 •

edited

MaikuB commented Nov 15, 2020 •

edited

jpnurmi commented Nov 18, 2020 •

edited

MaikuB commented Nov 19, 2020 •

edited