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

portal-impl: Only return found implementation if it launched #985

Conversation

jadahl
Copy link
Collaborator

@jadahl jadahl commented Mar 13, 2023

If no portal backend for a given interface is found, a fallback is always tried anyway, despite that fallback not being listed as compatible with the current desktop environment.

Sometimes it's good that a fallback is returned; e.g. the xdg-desktop-portal-gtk file chooser backend is technically usable anywhere, however, some backends might be specifically designed to only work in a specific desktop environment, e.g. xdg-desktop-portal-gnome.

In order to avoid creating portals with non-functional backends, make sure it's possible to create a proxy object for the interface and D-Bus name, and that it launched successfully (i.e. has no name owner after creating the proxy).


Opening this against xdg-desktop-portals-1.16 because hopefully #955 will make this obsolete.

Not sure how this will work on non-systemd systems; it relies on using g_dbus_proxy_get_name_owner() being set after D-Bus activation did its thing, which is true for the portals I have managed to test (xdg-desktop-portal-{gnome,gtk,kde})

If no portal backend for a given interface is found, a fallback is
always tried anyway, despite that fallback not being listed as
compatible with the current desktop environment.

Sometimes it's good that a fallback is returned; e.g. the
xdg-desktop-portal-gtk file chooser backend is technically usable
anywhere, however, some backends might be specifically designed to only
work in a specific desktop environment, e.g. xdg-desktop-portal-gnome.

In order to avoid creating portals with non-functional backends, make
sure it's possible to create a proxy object for the interface and D-Bus
name, and that it launched successfully (i.e. has no name owner after
creating the proxy).
@smcv
Copy link
Collaborator

smcv commented Mar 13, 2023

it relies on using g_dbus_proxy_get_name_owner() being set after D-Bus activation did its thing

That should be equally valid for traditional activation in dbus-daemon: in traditional activation, the signal that the service is ready is exactly the name being requested.

It would be good to get a dbus-monitor trace with each of dbus-daemon and dbus-broker, to make sure that the NameOwnerChanged signal happens before the successful method return.

@jadahl
Copy link
Collaborator Author

jadahl commented Mar 20, 2023

Gather some logs on a system using dbus-broker (F38 beta):

method call time=1679309435.630877 sender=:1.2 -> destination=org.freedesktop.systemd1 serial=121 path=/org/freedesktop/system
d1; interface=org.freedesktop.systemd1.Manager; member=StartUnit
   string "xdg-desktop-portal-gnome.service"
   string "replace"

method return time=1679309435.633568 sender=:1.1 -> destination=:1.2 serial=1691 reply_serial=121
   object path "/org/freedesktop/systemd1/job/554"

signal time=1679309435.831621 sender=org.freedesktop.DBus -> destination=(null destination) serial=4294967295 path=/org/freedesktop/DBus; interface=org.freedesktop.DBus; member=NameOwnerChanged
   string "org.freedesktop.impl.portal.desktop.gnome"
   string ""
   string ":1.89"

In other words, it looks like NameOwnerChanged happens after the method is returned. For GDBus that's not a problem; it calls GetNameOwner after StartServiceByName returned and gets the name that way.

@jadahl
Copy link
Collaborator Author

jadahl commented Mar 30, 2023

@smcv any concerns left?

Copy link
Member

@GeorgesStavracas GeorgesStavracas left a comment

Choose a reason for hiding this comment

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

Let's go with this for 1.16. For 1.18, the plan is to land Emmanuele's rework anyway.

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

3 participants