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] migrate to xdg-desktop-portal for notifications #1987

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Merrit
Copy link

@Merrit Merrit commented May 9, 2023

The xdg-desktop-portal is the new standard for desktop notifications
and is supported by GNOME, KDE, and other desktop environments. It
also works with Flatpak and Snap applications.

Resolves #1757

@Merrit
Copy link
Author

Merrit commented May 9, 2023

Created as a draft for a couple reasons:

That said, everything regarding sending notifications, body markup, actions, callbacks, etc seems to be working well - and crucially also working from within Flatpak, which was the big problem for my use case.

For anyone else needing Flatpak support, you can use a git override until this change can be merged:

dependency_overrides:
# Override flutter_local_notifications to use the fork with the fix for
# Flatpak until the fix is merged.
# See: https://github.com/MaikuB/flutter_local_notifications/issues/1757
flutter_local_notifications:
  git:
    url: https://github.com/Merrit/flutter_local_notifications.git
    ref: d5d529cb2278deba2ca8244025256348483a1cdd
    path: flutter_local_notifications

@MaikuB
Copy link
Owner

MaikuB commented May 11, 2023

Thanks for the PR. I'll take a look when I can block out some time to sit through this properly. Not though that I don't use Linux and I'm not particularly familiar with the ecosystem so I would suggest trying to see if there are others in the community that could help out with this

P.S. you may see me trying to update the plugin with a more urgent quick fix ahead of this PR given Flutter 3.10 just landed on stable and included Dart 3 so believe I need to to bump the max Dart SDK constraints

@Merrit
Copy link
Author

Merrit commented May 11, 2023

No worries, I don't expect everyone to be familiar with Linux. I wanted to make the PR smaller, but everything was pretty entwined. If there is anything I can do to help, just let me know! 😃

P.S. you may see me trying to update the plugin with a more urgent quick fix ahead of this PR given Flutter 3.10 just landed on stable and included Dart 3 so believe I need to to bump the max Dart SDK constraints

Sounds like a good idea 💙

@MaikuB
Copy link
Owner

MaikuB commented May 12, 2023

If there is anything I can do to help, just let me know!

If there are others in the community that you know that you could contact to help review and/or provide feedback on this then that would be great. On that note, since the issue was originally raised by you, do you think you'd be able to check this out @emersion?

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.

I left some general comments on the PR though didn't actually get to test it out. As a whole the changes make sense to me but don't want to lose functionality so would like to wait until the missing functionality is officially supported

@@ -52,10 +52,10 @@ class ReceivedNotification {
String? selectedNotificationPayload;

/// A notification action which triggers a url launch event
const String urlLaunchActionId = 'id_1';
const String urlLaunchActionId = 'id1';
Copy link
Owner

Choose a reason for hiding this comment

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

what the reason behind this change? i wasn't expecting example app to require this kind of the change given the intent behind the PR

Copy link
Author

Choose a reason for hiding this comment

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

Ah yeah, forgot to document that. I believe it didn't work, and troubleshooting indicated it was when it had an underscore.

flutter_local_notifications/pubspec.yaml Outdated Show resolved Hide resolved
# Temporarily use a fork of xdg_desktop_portal to support the ActionInvoked signal
xdg_desktop_portal:
git:
url: https://github.com/Merrit/xdg_desktop_portal.dart.git
Copy link
Owner

Choose a reason for hiding this comment

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

I saw you shared some details on how there are some proposals given the missing functionality etc. You probably figured but I would say until that is addressed then the changes in the PR wouldn't be part of a stable release of the plugin

@Merrit
Copy link
Author

Merrit commented May 16, 2023

As a whole the changes make sense to me but don't want to lose functionality so would like to wait until the missing functionality is officially supported

Sounds reasonable :)

FYI, I did ask some people if anyone could help review but so far no takers.

The xdg-desktop-portal is the new standard for desktop notifications
and is supported by GNOME, KDE, and other desktop environments. It
also works with Flatpak and Snap applications.

Resolves MaikuB#1757
@Merrit Merrit force-pushed the linux_notifications_portal branch from 03b9104 to afd8cb7 Compare September 27, 2023 17:50
@Merrit
Copy link
Author

Merrit commented Apr 9, 2024

Update

My PR for the supporting package hasn't had any movement. Canonical appears to have abandoned the package - hasn't seen any activity in a long time, and when I reached out to them directly to ask about the status of the project it was ignored.

In addition it appears that #1757 was incorrect about org.freedesktop.Notifications not being available in Flatpak - it seems to be undocumented because they prefer you use the new implementation, but you can use it by adding --talk-name=org.freedesktop.Notifications to the finish-args of the flatpak manifest, at which point flutter_local_notifications works just fine as-is.

Given these developments I suggest we close this PR to keep the repo clean, and update #1757 with the information to use --talk-name=org.freedesktop.Notifications. Migrating to org.freedesktop.portal.Notification can be revisited if and when the new new spec proposal gets implemented, and actually has the desired features.

@emersion
Copy link
Contributor

emersion commented Apr 9, 2024

Note that --talk-name=org.freedesktop.Notifications basically punches a hole through the sandbox. It makes the sandbox insecure to some extent, e.g. because it allows the sandboxed app to manipulate notifications of other apps (by guessing their IDs, which is a simple counter). So it's more of a workaround than a proper solution.

@Merrit
Copy link
Author

Merrit commented Apr 9, 2024

Yeah, I understand they want people to use the new implementation for good reasons. Given the lack of support for needed features though, IMHO workaround seems proper until v2 of the new portal is available.

@Levi-Lesches
Copy link
Contributor

Hi @Merrit, I was looking into #2480, specifically flatpak/xdg-desktop-portal#983. That issue was closed 3 weeks ago, and there is a new discussion about adding support for features that didn't make it into v2. I know that canonical is taking their time with your other PR, but does that theoretically unblock this?

I don't actually use Linux as a daily driver, just taking an interest 😁

@Levi-Lesches
Copy link
Contributor

Levi-Lesches commented Dec 4, 2024

@Merrit

My canonical/xdg_desktop_portal.dart#86 hasn't had any movement. Canonical appears to have abandoned the package - hasn't seen any activity in a long time, and when I reached out to them directly to ask about the status of the project it was ignored.

I just commented on there yesterday and they responded that they didn't merge the PR because it's still a draft. Can you mark it as ready for review, sign the CLA, and ping them one more time?

@Merrit
Copy link
Author

Merrit commented Dec 4, 2024

I suppose I can do that yeah, I don't have the bandwidth at the moment to implement any changes if requested though.

@Levi-Lesches
Copy link
Contributor

Levi-Lesches commented Dec 13, 2024

Seems the canonical PR was merged! canonical/xdg_desktop_portal.dart#86

@Merrit You should be able to replace the pubspec override with just xdg_desktop_portal: 0.1.13 now.

Does that mean this is ready for review now? Again I'm not actually personally affected as I prefer Windows and AppImage instead of FlatPak, but as emersion said, this affects the sandbox and issue #1757

@Merrit
Copy link
Author

Merrit commented Dec 27, 2024

Looks like the default action of clicking the notification, rather than the buttons isn't working - will have to be addressed as I found out here.

In addition in my earlier comment I pointed out some missing functionality, namely:

  • body images
  • sound control
  • action icons
  • position on screen

These may have been added to the portal spec (I haven't checked over what was changed though), but we'll have to confirm if those are supported by the current dart package or not before this (or a new) PR can proceed.

@emersion
Copy link
Contributor

position on screen

That is not part of org.freedesktop.Notifications, FWIW.

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.

[flutter_local_notifications_linux] Add support for org.freedesktop.portal.Notification
4 participants