portal-impl: Use fallback portals for Settings as well #1358

liskin · 2024-05-10T20:34:27Z

Since 1.18.2, we have a hardcoded fallback to xdg-desktop-portal-gtk as a last resort if no other portal is defined by a configuration file. This fallback mechanism, however, isn't implemented for the org.freedesktop.impl.portal.Settings interface.

The consequence is that everything seems to work just fine, but user preferences like color scheme are ignored in applications that use the portal API to retrieve them (not just flatpak apps, also libadwaita apps). That comes as a surprise to users; and since the portal stuff otherwise works fine, it's not entirely straightforward to figure out that configuring portal.conf would fix it.

I believe that if find_portal_implementation implements fallbacks, so should find_all_portal_implementations, for consistency and to avoid surprises.

The fallback logic this commit implements closely resembles the logic in find_portal_implementation: we don't look for fallbacks if there is a portal configured in portals.conf (or if portals.conf explicitly says there should be none), and we only use the x-d-p-gtk as a last resort if we haven't found any impl(s) using the legacy UseIn key.

(The above should be a self-contained description but there's some additional thoughts at #1199 (comment))

Fixes: d18c563 ("portal-impl: Hard-code x-d-p-gtk as a last-resort fallback")
Related: #1102

tommie · 2024-08-13T20:31:48Z

I like this. I just spent an hour or two figuring out why GNOME Software Center refused to go into dark mode under XFCE: https://tommie.github.io/a/2024/08/xfce-gnome-dark-mode

Turns out it was because Settings requires the portals.conf (Xubuntu 24.04; XDP 1.14.4) and libadwaita seems to not fall back on gsettings once it's tried XDP.

However, I'm curious why the UseIn is deprecated. Is there a replacement? Assuming one machine supports multiple desktop environments for users to choose from, it seems like a global configuration isn't expressive enough. The workaround is a user-specific configuration. Forcing users to create a config file feels like a regression.

smcv · 2024-08-14T11:23:51Z

However, I'm curious why the UseIn is deprecated. Is there a replacement? Assuming one machine supports multiple desktop environments for users to choose from, it seems like a global configuration isn't expressive enough.

Agreed, and that's why there's a per-desktop-environment configuration (which supersedes UseIn), documented in https://flatpak.github.io/xdg-desktop-portal/docs/portals.conf.html. XFCE provides an xfce-portals.conf since https://gitlab.xfce.org/xfce/xfce4-session/-/issues/181; if that hasn't made it into Xubuntu, please report that to your distro vendor.

tommie · 2024-08-14T12:41:24Z

xfce-portals.conf

Oh, sorry, I missed the part where it said that the prefix is matched against the desktop name. So that's the replacement.

Thanks for the link!

tommie · 2024-08-14T13:15:48Z

Apologies for the tangential noise on this PR. Just to close this off:

Ubuntu 24.04 is unlucky to have xfce4-session 4.18.3, while the new config file is in 4.18.4. 4.18.4 is in Debian testing.

I also found your https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1050802.

So I filed https://bugs.launchpad.net/ubuntu/+source/xfce4-session/+bug/2076999.

swick · 2024-10-04T11:54:34Z

src/portal-impl.c

@@ -691,5 +693,56 @@ find_all_portal_implementations (const char *interface)
        }
    }

+  if (impls->len)


Suggested change

if (impls->len)

if (impls->len > 0)

Just to be more clear.

swick · 2024-10-04T11:56:32Z

src/portal-impl.c

+        {
+          PortalImplementation *impl = l->data;
+
+          if (!g_strv_contains ((const char **)impl->interfaces, interface))


Suggested change

if (!g_strv_contains ((const char **)impl->interfaces, interface))

if (!portal_impl_supports_iface (impl, interface))

swick · 2024-10-04T11:58:43Z

src/portal-impl.c

+        }
+    }
+
+  if (impls->len)


Suggested change

if (impls->len)

if (impls->len > 0)

swick · 2024-10-04T11:59:16Z

src/portal-impl.c

+      if (!g_str_equal (impl->dbus_name, "org.freedesktop.impl.portal.desktop.gtk"))
+        continue;
+
+      if (!g_strv_contains ((const char **)impl->interfaces, interface))


Suggested change

if (!g_strv_contains ((const char **)impl->interfaces, interface))

if (!portal_impl_supports_iface (impl, interface))

swick · 2024-10-04T11:59:58Z

The change seems fine to me but I'd like @ebassi opinion on it.

liskin · 2024-10-05T10:20:41Z

Rebased on top on today's main and applied the suggestions.

GeorgesStavracas

Superficial comments only, but I also would like to hear what @ebassi thinks of it

GeorgesStavracas · 2024-10-05T13:01:51Z

src/portal-impl.c

+  desktops = get_current_lowercase_desktops ();
+
+  /* Fallback to the old UseIn key */
+  for (i = 0; desktops[i] != NULL; i++)


These days I've been preferring the following syntax:

for (size_t i = 0; desktops[i] != NULL; i++)

GeorgesStavracas · 2024-10-05T13:02:13Z

src/portal-impl.c

+  /* Fallback to the old UseIn key */
+  for (i = 0; desktops[i] != NULL; i++)
+    {
+     for (l = implementations; l != NULL; l = l->next)


Ditto:

for (GList *l = implementations; l != NULL; l = l->next)

GeorgesStavracas · 2024-10-05T13:02:42Z

src/portal-impl.c

+   * try to fall back to x-d-p-gtk, which has historically been the portal
+   * UI backend used by desktop environments with no backend of their own.
+   * If it isn't installed, that is not an error: we just don't use it. */
+  for (l = implementations; l != NULL; l = l->next)


Ditto:

for (GList *l = implementations; l != NULL; l = l->next)

GeorgesStavracas · 2024-10-05T13:03:18Z

src/portal-impl.c

+  GList *l;
+  int i;


If you apply the previous suggestions, you'll be able to remove these variables from here

I did apply them—in a separate commit, as I updated find_portal_implementation to match as well.

Since 1.18.2, we have a hardcoded fallback to xdg-desktop-portal-gtk as a last resort if no other portal is defined by a configuration file. This fallback mechanism, however, isn't implemented for the org.freedesktop.impl.portal.Settings interface. The consequence is that everything seems to work just fine, but user preferences like color scheme are ignored in applications that use the portal API to retrieve them (not just flatpak apps, also libadwaita apps). That comes as a surprise to users; and since the portal stuff otherwise works fine, it's not entirely straightforward to figure out that configuring portal.conf would fix it. I believe that if find_portal_implementation implements fallbacks, so should find_all_portal_implementations, for consistency and to avoid surprises. The fallback logic this commit implements closely resembles the logic in find_portal_implementation: we don't look for fallbacks if there is a portal configured in portals.conf (or if portals.conf explicitly says there should be none), and we only use the x-d-p-gtk as a last resort if we haven't found any impl(s) using the legacy UseIn key. (The above should be a self-contained description but there's some additional thoughts at flatpak#1199 (comment)) Fixes: d18c563 ("portal-impl: Hard-code x-d-p-gtk as a last-resort fallback") Related: flatpak#1102

liskin mentioned this pull request May 10, 2024

portal-impl: Hard-code x-d-p-gtk as a last-resort fallback #1199

Merged

swick reviewed Oct 4, 2024

View reviewed changes

liskin force-pushed the settings-fallback branch from 2e159e1 to 36ef9be Compare October 5, 2024 10:19

GeorgesStavracas requested changes Oct 5, 2024

View reviewed changes

liskin added 2 commits October 5, 2024 18:24

portal-impl: Cosmetics

52f5814

liskin force-pushed the settings-fallback branch from 36ef9be to 52f5814 Compare October 5, 2024 17:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

portal-impl: Use fallback portals for Settings as well #1358

portal-impl: Use fallback portals for Settings as well #1358

liskin commented May 10, 2024

tommie commented Aug 13, 2024

smcv commented Aug 14, 2024

tommie commented Aug 14, 2024

tommie commented Aug 14, 2024

swick Oct 4, 2024

swick Oct 4, 2024

swick Oct 4, 2024

swick Oct 4, 2024

swick commented Oct 4, 2024

liskin commented Oct 5, 2024

GeorgesStavracas left a comment

GeorgesStavracas Oct 5, 2024

GeorgesStavracas Oct 5, 2024

GeorgesStavracas Oct 5, 2024

GeorgesStavracas Oct 5, 2024

liskin Oct 5, 2024

	if (!g_strv_contains ((const char **)impl->interfaces, interface))
	if (!portal_impl_supports_iface (impl, interface))

portal-impl: Use fallback portals for Settings as well #1358

Are you sure you want to change the base?

portal-impl: Use fallback portals for Settings as well #1358

Conversation

liskin commented May 10, 2024

tommie commented Aug 13, 2024

smcv commented Aug 14, 2024

tommie commented Aug 14, 2024

tommie commented Aug 14, 2024

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

swick commented Oct 4, 2024

liskin commented Oct 5, 2024

GeorgesStavracas 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