GNOME Bugzilla – Bug 768498
portal support for glib
Last modified: 2016-07-13 03:34:23 UTC
I've pushed a portal2 branch that supports a number of the portal interfaces from https://github.com/flatpak/xdg-desktop-portal: application launching, network monitoring, proxy resolving and notifications.
I've reviewed the branch, and it looks good to me.
Hi, One issue I saw with the commits on gproxyresolverportal.c is the unconditional call to the following (sorry for the formatting as I copied and pasted) in g_proxy_resolver_portal_init(): resolver->resolver = gxdp_proxy_resolver_proxy_new_for_bus_sync (G_BUS_TYPE_SESSION, G_DBUS_PROXY_FLAGS_DO_NOT_AUTO_START, "org.freedesktop.portal.Desktop", "/org/freedesktop/portal/desktop", NULL, NULL); On systems that don't have flatpak/xdg-desktop-portal support (such as Windows) this is going to cause the system to wait infinitely for this gdbus op, so I guess we want to check for glib_should_use_portal () beforehand? With blessings, thank you!
Created attachment 331195 [details] [review] gproxyresolverportal.c: Init portal proxy resolver backend after checking for portal availability Hi, My stab at the patch for this. With blessings, thank you!
Review of attachment 331195 [details] [review]: The glib_should_use_portal() function is really glib_is_the_application_running_inside_a_flatpak(). I think portal-related classes should not be built on platforms that are not targeted by Flatpak.
Created attachment 331232 [details] [review] gio: Build portal items only on *NIX Hi Emmanuele, I agree with you on this regard, so here comes the new patch to exclude it on non-*NIX. I didn't go for checking for flatpak and xdg-desktop-portal in configure.ac as that seems to introduce a circular dependency, that can be resolved, but seems to be confusing and bothersome as that would involve building GLib twice if starting from scratch. With blessings, thank you!
Review of attachment 331232 [details] [review]: ::: gio/gappinfo.c @@ +37,1 @@ This part looks fine ::: gio/giomodule.c @@ +1120,3 @@ g_type_ensure (g_portal_notification_backend_get_type ()); + g_type_ensure (g_network_monitor_portal_get_type ()); + g_type_ensure (g_proxy_resolver_portal_get_type ()); This doesn't look quite right to me; the portal code has nothing to do with either netlink or cocoa.
Review of attachment 331232 [details] [review]: ::: gio/giomodule.c @@ +1120,3 @@ g_type_ensure (g_portal_notification_backend_get_type ()); + g_type_ensure (g_network_monitor_portal_get_type ()); + g_type_ensure (g_proxy_resolver_portal_get_type ()); Hi Matthias, Hmm, I thought I had it in the right places for those, guarded by G_OS_UNIX, no? The line numbers can be deceiving though... I will do a new patch though quite soon, as there are newer portal code that is being added at this time, and I think I want to rearrange gio/Makefile.am? With blessings, thank you!
Created attachment 331291 [details] [review] gio: Build portal items only on *NIX (take ii) Hi, This is the updated patch which: -Was rebased, due to Cosimo's work -gio/Makefile.am is cleaned up a bit -Checked that the changes in giomodule.c is correct With blessings, thank you!
Review of attachment 331232 [details] [review]: ::: gio/giomodule.c @@ +1120,3 @@ g_type_ensure (g_portal_notification_backend_get_type ()); + g_type_ensure (g_network_monitor_portal_get_type ()); + g_type_ensure (g_proxy_resolver_portal_get_type ()); Hmm, looking at the source file, it appears you are right; sorry for the false alarm.
Comment on attachment 331291 [details] [review] gio: Build portal items only on *NIX (take ii) Hi Matthias, I pushed the patch as b5258d9, thanks for the review and ack. With blessings, thank you!