notification: Add the desktop-file-id key for portal backends #1488
Conversation
There was a problem hiding this comment.
Looks good to me. This improves the situation but doesn't resolve the issue when a flatpak exports multiple desktop-files, but this does move in the correct direction so I'm fine with this changes.
We probably can do some clean up e.g. remove xdp_get_app_id_from_desktop_id() and replace fallback of "" for host apps with NULL .The clean ups should probably happen in a different MR.
| g_autoptr(GDesktopAppInfo) desktop_appinfo = NULL; | ||
|
|
||
| desktop_appinfo = g_desktop_app_info_new (desktop_file); | ||
| if (desktop_appinfo) |
There was a problem hiding this comment.
do we actually need this dance of assigning the value first to desktop_appinfo?
There was a problem hiding this comment.
The casts don't like getting NULL and there is no guarantee that we'll be able to find the desktop file that we "detected" so I don't see how.
src/notification.c
Outdated
| @@ -1077,6 +1078,10 @@ handle_add_in_thread_func (GTask *task, | |||
| return; | |||
| } | |||
|
|
|||
| desktop_file = xdp_app_info_get_desktop_file (call_data->app_info); | |||
| if (desktop_file) | |||
There was a problem hiding this comment.
probably we should check here whether desktop_file == "". Or maybe a follow up MR could make sure that the desktop_file is never set to "".
There was a problem hiding this comment.
Changed it so we never get an empty string or ".desktop" as desktop_file_id but in general, it needs to handle not finding the file anyway so this doesn't really change anything.
This allows clients to access either the GAppInfo or the desktop file when needed.
This makes it possible for portal impl to properly look up a localized name of the app. The app id that is sent as the first argument can sometimes be mapped to the desktop file but this isn't generally true and snap app ids for example cannot be used for this.
swick
force-pushed
the
wip/notification-impl-desktop-file
branch
from
027de6a
to
80f66fe
Compare
|
It was decided against this last time the same thing was proposed, as it is many places assumed the app ID resolves to a desktop file. See #904. |
|
This is in the backend i.e. the |
That is what was proposed last time too. |
|
Sorry, but assuming that an app id maps to a desktop file is just plain wrong. Not sending the desktop file is wrong. Nothing will change for impls which currently get away with assuming the app id maps to a desktop file. |
|
Resolving an app id that is derived in xdg-desktop-portal to a desktop file is assumed to work in the multiple portal backends, I don't know why you are claiming it's "plain wrong", it's how it's designed to work. It's not only a notification portal thing. |
|
There are literally two choices here:
What we currently have is that desktop files and app ids can be different but the stack assumes that they are not. If we're not going for 1. then I want to know what the plan for 2. is. |
|
And it's a fact that you can not generally resolve app ids to desktop files because there is at least one case where it doesn't work. Trying to do so anyway is therefor wrong. You can argue that it shouldn't be this way, but the facts here are simple. |
|
For flatpak, the only time it doesn't work fully as expected is apps with multiple "apps" inside, where the sub-app are distinguished by a suffix, while sharing the same app id (that still resolves to a desktop file) as the prefix. But that is a different story to what is done here. |
|
Portals are used not only by flatpak. |
To fix #916, we need to get the application name from a
org.freedesktop.impl.portal.Notification.AddNotificationcall which is currently not possible because we only get an app id which does not always map to a dekstop file (https://github.com/flatpak/xdg-desktop-portal-gtk/blob/366d3349041c2e06658c7c5a8b5bfd5f345d8157/src/fdonotification.c#L348).This PR adds the
desktop-file-idkey toAddNotificationimpl calls to make it possible for backends to show an app name.