GNOME Bugzilla – Bug 786694
wayland: fix fontconfig monitoring
Last modified: 2018-02-27 15:37:02 UTC
+++ This bug was initially created as a clone of Bug #786693 +++ 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 365988 [details] [review] Introduce org.gnome.SettingsDaemon.FontConfig Timestamp is exposed as the 'ConfigTimestamp' property of a (new) org.gnome.SettingsDaemon.FontConfig dbus interface. 'PropertiesChanged', from org.freedesktop.DBus.Properties, is emitted on update. It feels wrong to implement this in the xsettings plugin. Think it deserves a dedicated plugin; xsettings could rely on this new one. But needs more work... Comments and feedbacks welcome.
Thanks for working on this!
Review of attachment 365988 [details] [review]: ::: plugins/xsettings/gsd-xsettings-manager.c @@ +81,3 @@ +"<node name='/org/gnome/SettingsDaemon/FontConfig'>" +" <interface name='org.gnome.SettingsDaemon.FontConfig'>" +" <property name='ConfigTimestamp' type='i' access='read'/>" Should maybe be 'u' instead of 'i' ?
Review of attachment 365988 [details] [review]: .
(In reply to Matthias Clasen from comment #3) > Should maybe be 'u' instead of 'i' ? Not sure about what to do here: xsettings plugin is using an int value retrieved from time(), gtk+'s setting in uint, and GLib's time seems to be represented by int64... In my opinion, exposing an int64 from g_get_real_time() would be the best option, but wouldn't help on gtk+ side. The int type has only been kept for some sort of compatibility with the original xsetting. So should we simplify and opt for gtk+'s uint?
Created attachment 368196 [details] [review] Introduce org.gnome.SettingsDaemon.FontConfig (timestamp as int64) Third version: fix stupid bug in previous version...
Created attachment 368188 [details] [review] Introduce org.gnome.SettingsDaemon.FontConfig (timestamp as int64) Second version: time is represented as int64 and valued using g_get_real_time(). Comments and feedbacks welcome.
It's too late to land this for GNOME 3.28. The gnome-shell patch will need reviewing before this can land, assuming the code is correct. At a glance, you'll want to go through the patch and remove the unnecessary whitespace changes.
Created attachment 368734 [details] [review] Introduce org.gnome.SettingsDaemon.FontConfig (timestamp as int64) Fourth version: removed a couple of extra whitespaces.
(In reply to Bastien Nocera from comment #8) > It's too late to land this for GNOME 3.28. The gnome-shell patch will need > reviewing before this can land, assuming the code is correct. May have missed something here, sorry: what is this gnome-shell patch you are talking about?
(In reply to Martin Blanchard from comment #10) > (In reply to Bastien Nocera from comment #8) > > It's too late to land this for GNOME 3.28. The gnome-shell patch will need > > reviewing before this can land, assuming the code is correct. > > May have missed something here, sorry: what is this gnome-shell patch you > are talking about? Never mind, the gtk+ patch rather. I'll review it later.
Review of attachment 368734 [details] [review]: 1) Get rid of the array 2) Every time a timestamp change has happened, queue sending the new value for half-a-second (or a second?), reschedule if a new timestamp arrives in that time 3) Don't use g_get_real_time() ::: plugins/xsettings/gsd-xsettings-manager.c @@ +80,3 @@ +static const gchar introspection_xml[] = +"<node name='/org/gnome/SettingsDaemon/FontConfig'>" +" <interface name='org.gnome.SettingsDaemon.FontConfig'>" Is this really the name of the interface we want to use? What about GTK+ apps under other Wayland desktops? @@ +284,3 @@ GSettings *plugin_settings; FcMonitor *fontconfig_monitor; + GArray *fontconfig_timestamps; I've stared at the code for a long while, and can't figure out why you need to send a "changed" signal for each and every fontconfig timestamp change. Why do that? You could just keep the last one, and send it out when it changes, or aggregate/coalesce the changes into one event if the changes are too close together. You need to remember that this would wake up every single GTK+ program, so less notifications is better. @@ +1189,3 @@ GList *list, *l; const char *session; + gint64 timestamp = g_get_real_time (); You shouldn't user g_get_real_time(), this will break if the time on the machine is changed. Any reason why g_get_monotonic_time() can't be used? @@ +1453,3 @@ + manager = GNOME_XSETTINGS_MANAGER (user_data); + + if (manager->priv->dbus_connection == NULL) This isn't necessary, you get a connection in the arguments of the function above, and you don't use this variable anyway.
The XSettings in the xsettings plugin are either: - in GSettings, and GTK+ will read those directly under Wayland - hardcoded in gnome-settings-daemon, for older versions of GTK+, and hardcoded in newer versions of GTK+ 3.x - "computed" in the xsettings plugin Those last ones are what we're interested in here. They are: - fontconfig timestamps (Fontconfig/Timestamp) - whether to enable animations (used for remote displays, Gtk/EnableAnimations) - GTK+ modules list (Gtk/Modules) - whether gnome-shell (and its builtin menu) is running (Gtk/ShellShowsAppMenu) - a number of font and DPI settings (see xft_settings_set_xsettings()) That last two are handled directly in GTK+ in Wayland talking to the compositor and checking its capabilities, or in gdk/wayland/gdkscreen-wayland.c. Which leaves us with those first 3 settings (for now). So, on top of the fontconfig specific changes above, it would be nice if: - the D-Bus name showed that the settings are GTK+ specific, say "org.gtk.Settings" - also export Gtk/EnableAnimations and the modules list through D-Bus
Created attachment 368775 [details] [review] xsettings: Expose monitored settings through D-Bus A number of things that gnome-settings-daemon monitors for all the GTK+ applications aren't currently available for GTK+ Wayland clients, only through XSettings. As we already monitor fontconfig configurations, enabled GTK+ modules and whether applications should use animations, export those through a D-Bus interface. See https://bugzilla.gnome.org/show_bug.cgi?id=786693 Based on patch by Martin Blanchard <tchaik@gmx.com>
The patch above is completely untested, and the GTK+ patch needs rewriting. You might need to also apply the patch from bug 793721 for this one to apply.
Created attachment 368833 [details] [review] xsettings: Expose monitored settings through D-Bus A number of things that gnome-settings-daemon monitors for all the GTK+ applications aren't currently available for GTK+ Wayland clients, only through XSettings. As we already monitor fontconfig configurations, enabled GTK+ modules and whether applications should use animations, export those through a D-Bus interface. See https://bugzilla.gnome.org/show_bug.cgi?id=786693 Based on patch by Martin Blanchard <tchaik@gmx.com>
Created attachment 368834 [details] [review] xsettings: Use temporary variable for GTK+ modules dir So that we can override it later.
Created attachment 368835 [details] [review] xsettings: Make it possible to override GTK+ modules path
Created attachment 368836 [details] [review] xsettings: Add tests for D-Bus interface FIXME, EnableAnimations doesn't work because the test can't create a "boolean" property, and I don't know why.
Created attachment 368931 [details] [review] xsettings: Add tests for D-Bus interface
Created attachment 368943 [details] [review] xsettings: Make it possible to ignore llvmpipe So that we can test out vino support.
Created attachment 368944 [details] [review] xsettings: Add tests for D-Bus interface
Created attachment 368945 [details] [review] tests: Rename work directory There's more than just a power test now.
Created attachment 369014 [details] [review] xsettings: Add tests for D-Bus interface
Created attachment 369015 [details] [review] tests: Fix concurrent launches of Xorg-reliant test cases
Attachment 368833 [details] pushed as 522640a - xsettings: Expose monitored settings through D-Bus Attachment 368834 [details] pushed as 85e162f - xsettings: Use temporary variable for GTK+ modules dir Attachment 368835 [details] pushed as 2b79fb8 - xsettings: Make it possible to override GTK+ modules path Attachment 368943 [details] pushed as e514810 - xsettings: Make it possible to ignore llvmpipe Attachment 368945 [details] pushed as 26e1e1e - tests: Rename work directory Attachment 369014 [details] pushed as 8ec7856 - xsettings: Add tests for D-Bus interface Attachment 369015 [details] pushed as 9dbb9e8 - tests: Fix concurrent launches of Xorg-reliant test cases