GNOME Bugzilla – Bug 786693
wayland: fix fontconfig monitoring
Last modified: 2018-05-02 18:55:28 UTC
Under X11, we are using and X setting that contains a 'fontconfig timestamp', and that gets updated by gnome-settings-daemon. This doesn't work under Wayland, since we only monitor those xsettings that are backed by a gsetting. After some discussion on #gtk+, we suggest to use a dbus signal instead. It can be emitted by the same gsd plugin that maintains the xsettings.
Created attachment 365989 [details] [review] 'gtk-fontconfig-timestamp' for Wayland Replies on new org.gnome.SettingsDaemon.FontConfig. See Bug #786693. Comments and feedbacks welcome.
Thanks for working on this!
Created attachment 368197 [details] [review] 'gtk-fontconfig-timestamp' for Wayland (timestamp as int64) Second version: handle timestamp as int64, inline with new gnome-settings-daemon patch. Comments and feedbacks welcome.
Review of attachment 368197 [details] [review]: This looks good to me now.
Review of attachment 368197 [details] [review]: I don't think this is a good enough interface for GTK+ to use, but best discuss it in a location where it will be implemented (in gnome-settings-daemon) ::: gdk/wayland/gdkscreen-wayland.c @@ +919,3 @@ + if (proxy == NULL) + { + if (error != NULL) This will warn for every GTK+ app running under a non-GNOME Wayland server. @@ +946,3 @@ + screen_wayland->fc_settings.timestamp = (guint)timestamp; + else if (timestamp > G_MAXUINT) + g_warning ("Could not handle fontconfig init: timestamp too long"); too long? @@ +975,3 @@ + G_DBUS_PROXY_FLAGS_NONE, + NULL, + GSD_DBUS_FONTCONFIG_NAME, This is highly GNOME specific.
The gnome-settings-daemon patch is all done. The D-Bus service will be on "org.gtk.Settings": <node name='/org/gtk/Settings'> <interface name='org.gtk.Settings'> <property name='FontconfigTimestamp' type='x' access='read'/> <property name='Modules' type='s' access='read'/> <property name='EnableAnimations' type='b' access='read'/> </interface> </node> And those values will be updated at run-time, as needed.
Created attachment 368982 [details] [review] 'gtk-fontconfig-timestamp' for Wayland (dbus's org.gtk.Settings interface) Third version: uses new dbus interface org.gtk.Settings and mute warning on dbus connection opening failure.
Review of attachment 368982 [details] [review]: Thanks for looking into this again, we're nearly there. It also looks easy enough to extend and add support for the 2 other GtkSettings if you're interested. ::: gdk/wayland/gdkscreen-wayland.c @@ +70,3 @@ GHashTable *settings; GsdXftSettings xft_settings; + GsdFcSettings fc_settings; Why use a struct here, and not directly a timestamp? The struct will be initialised to 0 when it's instantiated. @@ +116,3 @@ + if (screen_wayland->dbus_proxy) + g_object_unref (screen_wayland->dbus_proxy); You can replace both lines with g_clear_object(); @@ +713,3 @@ + if (strcmp (name, "gtk-fontconfig-timestamp") == 0) + { + if (wayland_screen->fc_settings.timestamp > 0) Why the guard here? Wouldn't you want to return the "0" instead of an error here? @@ +872,3 @@ + return; + + g_variant_get (changed_properties, "a{sv}", &iter); You can replace this loop with g_variant_lookup_value() Don't forget to exit early if the timestamp wasn't found. @@ +914,3 @@ + if (proxy == NULL) + { + g_clear_error (&error); Either you should not use an error at all (you don't do anything with it), or you should have a few cases and print errors if the error is something other than the call being cancelled, or org.gtk.Settings not existing. @@ +969,3 @@ + GTK_SETTINGS_DBUS_PATH, + GTK_SETTINGS_DBUS_NAME, + NULL, You'll also want to add a cancellable here, which you would cancel and destroy in the finalize function.
Created attachment 369118 [details] [review] 'gtk-fontconfig-timestamp' and 'gtk-modules' for Wayland Fourth version: Handle both 'gtk-fontconfig-timestamp' and 'gtk-modules'. (In reply to Bastien Nocera from comment #8) > It also looks easy enough to extend and add support for the 2 other > GtkSettings if you're interested. Hummm, isn't 'gtk-enable-animations' already implemented as a gsetting? > ::: gdk/wayland/gdkscreen-wayland.c > @@ +70,3 @@ > Why use a struct here, and not directly a timestamp? The struct will be > initialised to 0 when it's instantiated. I've kept the struct so that dbus settings are grouped, that make it clear that they'll be handle together. > @@ +116,3 @@ > You can replace both lines with g_clear_object(); Done. > @@ +713,3 @@ > Why the guard here? Wouldn't you want to return the "0" instead of an error > here? You're right, the distinction between a somehow fake initial value and 0 is of little interest here. > @@ +872,3 @@ > You can replace this loop with g_variant_lookup_value() > Don't forget to exit early if the timestamp wasn't found. Done. > @@ +914,3 @@ > Either you should not use an error at all (you don't do anything with it), > or you should have a few cases and print errors if the error is something > other than the call being cancelled, or org.gtk.Settings not existing. Agreed, my bad. > @@ +969,3 @@ > You'll also want to add a cancellable here, which you would cancel and > destroy in the finalize function. Done.
Review of attachment 369118 [details] [review]: Looks good otherwise. ::: gdk/wayland/gdkscreen-wayland.c @@ +929,3 @@ + proxy = g_dbus_proxy_new_for_bus_finish (result, NULL); + + g_clear_object (&screen_wayland->dbus_cancellable); No, don't do this, keep it around. Instead, you'll want to cancel it it in dispose(). @@ +955,3 @@ + } + + g_variant_unref (value); g_variant_unref() doesn't like being passed NULL. @@ +963,3 @@ + { + modules = g_variant_get_string (value, &length); + screen_wayland->dbus_settings.modules = g_strndup (modules, length); You're leaking the old value of dbus_settings.modules. Seeing as the string will be nul-terminated, free the old string, and use g_variant_dup_string() to assign the new value. @@ +966,3 @@ + } + + g_variant_unref (value); Ditto.
(In reply to Martin Blanchard from comment #9) > (In reply to Bastien Nocera from comment #8) > > It also looks easy enough to extend and add support for the 2 other > > GtkSettings if you're interested. > > Hummm, isn't 'gtk-enable-animations' already implemented as a gsetting? Yes, incorrectly so in the Wayland backend. In the X11 backend, animations are force-disabled if the GSettings is false, then controlled by gnome-settings-daemon, which will disable them if a software renderer is used, or an external VNC client is connected. > > ::: gdk/wayland/gdkscreen-wayland.c > > @@ +70,3 @@ > > Why use a struct here, and not directly a timestamp? The struct will be > > initialised to 0 when it's instantiated. > > I've kept the struct so that dbus settings are grouped, that make it clear > that they'll be handle together. That's fine. If a GTK+ reviewer doesn't like it, we can unbundle it. > > @@ +713,3 @@ > > Why the guard here? Wouldn't you want to return the "0" instead of an error > > here? > > You're right, the distinction between a somehow fake initial value and 0 is > of little interest here. Well, 0 is a valid value. This will break if somebody rewinds their clock back to the 1970, but the value itself is of little interest, we could use an always incrementing serial, but a timestamp will likely make races and debugging easier.
Created attachment 369162 [details] [review] 'gtk-fontconfig-timestamp' and 'gtk-modules' for Wayland Fifth version: Keep dbus connection's cancellable alive all along GdkWaylandScreen's lifetime. (In reply to Bastien Nocera from comment #10) > @@ +929,3 @@ > + g_clear_object (&screen_wayland->dbus_cancellable); > > No, don't do this, keep it around. > Instead, you'll want to cancel it it in dispose(). Ok. > @@ +955,3 @@ > + } > + > + g_variant_unref (value); > > g_variant_unref() doesn't like being passed NULL. Ouch... > @@ +963,3 @@ > + { > + modules = g_variant_get_string (value, &length); > + screen_wayland->dbus_settings.modules = g_strndup (modules, length); > > You're leaking the old value of dbus_settings.modules. > > Seeing as the string will be nul-terminated, free the old string, and use > g_variant_dup_string() to assign the new value. Well, I'm expecting this to be called only once, at object's initialisation time. Are there any case where this would be called again for the same GdkWaylandScreen instance? Anyway, string gets freed now.
(In reply to Bastien Nocera from comment #11) > (In reply to Martin Blanchard from comment #9) > > (In reply to Bastien Nocera from comment #8) > > > It also looks easy enough to extend and add support for the 2 other > > > GtkSettings if you're interested. > > > > Hummm, isn't 'gtk-enable-animations' already implemented as a gsetting? > > Yes, incorrectly so in the Wayland backend. In the X11 backend, animations > are force-disabled if the GSettings is false, then controlled by > gnome-settings-daemon, which will disable them if a software renderer is > used, or an external VNC client is connected. That makes sense. Shouldn't that be handled in a separate bug though?
Review of attachment 369162 [details] [review]: Just some nits. You don't need to upload a new version, I'd wait until a GTK+ developer can review and merge this now. ::: gdk/wayland/gdkscreen-wayland.c @@ +99,3 @@ GdkWaylandScreen *screen_wayland = GDK_WAYLAND_SCREEN (object); + if (screen_wayland->dbus_proxy && screen_wayland->dbus_setting_change_id) Linefeed after && Add > 0 to change_id conditional. @@ +947,3 @@ + g_warning ("Could not handle fontconfig init: timestamp out of bound"); + + g_variant_unref (value); You need to call those outside the if block, otherwise you'll be leaking the value if it doesn't match the expected type. @@ +959,3 @@ + screen_wayland->dbus_settings.modules = g_variant_dup_string (value, NULL); + + g_variant_unref (value); Same thing here.
(In reply to Martin Blanchard from comment #13) > (In reply to Bastien Nocera from comment #11) > > (In reply to Martin Blanchard from comment #9) > > > (In reply to Bastien Nocera from comment #8) > > > > It also looks easy enough to extend and add support for the 2 other > > > > GtkSettings if you're interested. > > > > > > Hummm, isn't 'gtk-enable-animations' already implemented as a gsetting? > > > > Yes, incorrectly so in the Wayland backend. In the X11 backend, animations > > are force-disabled if the GSettings is false, then controlled by > > gnome-settings-daemon, which will disable them if a software renderer is > > used, or an external VNC client is connected. > > That makes sense. Shouldn't that be handled in a separate bug though? Up to you. It can be a separate patch in this same bug though.
Created attachment 369588 [details] [review] 'gtk-fontconfig-timestamp' and 'gtk-modules' for Wayland Sixth version. Plug the potential leak.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gtk/issues/886.