GNOME Bugzilla – Bug 490374
monitor font configuration
Last modified: 2008-06-07 18:17:48 UTC
I want to make installed font show up in apps instantly. For this, we need: * gnome-settings-daemon do: - get list of files to monitor, using FcConfigGetConfigFiles, FcConfigGetConfigDirs and FcConfigGetFontDirs. - send notification if any changed, using what it sends now when one changes font properties * gtk+ catches that, and calls pango_fc_font_map_clear_caches() and redraws widgets. * pango does if (!FcConfigUptoDate (0)) FcInitReinitialize (); in pango_fc_font_map_clear_caches()
I was hoping that gnome-settings-daemon be doing some file monitoring already, but apparently I was wrong.
This has become just so easier with gio merged in. I'll look into it.
Created attachment 111936 [details] [review] Patch Attaching patch that uses gio to monitor fontconfig configurations and set the xsettings key Fontconfig/Timestamp upon change. The other part of this is bug 536185 that makes gtk listen to that and do the right thing. The code needs some conditionals on fontconfig being available. Any chance this can get into a release today? Want to see it tested.
It looks like a few "static" additions would be good. "timeout" and "monitors" seem like candidates.
Created attachment 111991 [details] [review] Updated patch Got rid of globals completely.
Looks good, but kill the pointless "g_ptr_array_set_size (monitors, 0);" More importantly, does this end up busy-waiting in any situations we should worry about? Ideally this should sleep until and unless someone actually changes the font configuration. What happens, for example, if a non-existent font directory is listed? Does running find/beagle/whatever on a watched directory trigger this?
Also, identation looks off in a few places, and the license header looks odd? And font-config checks, please.
(In reply to comment #6) > Looks good, but kill the pointless "g_ptr_array_set_size (monitors, 0);" Done. Will attach. > More importantly, does this end up busy-waiting in any situations we > should worry about? Ideally this should sleep until and unless someone > actually changes the font configuration. > > What happens, for example, if a non-existent font directory is listed? > Does running find/beagle/whatever on a watched directory trigger this? Good points indeed, and my main concern. I changed the code to, instead of a forced reinitializations, ask fontconfig if anything actually changed: if (!FcConfigUptoDate (NULL) && FcInitReinitialize ()) { notify = TRUE; monitors_free (handle->monitors); handle->monitors = monitors_create (data); } So it should be much more robust now. For example, it doesn't notify upon creating .#whatever.swap files in the conf directory because fontconfig ignores them.
(In reply to comment #7) > Also, identation looks off in a few places, Can you elaborate? Is it that it uses tabs? Converted to 8 spaces. Coming. > and the license header looks odd? How? Why? Copied from neighboring xsettings-common.c and updated date and author. > And font-config checks, please. Right. Lemme think about how the check should be done and come up with an updated one. Do you plan to push a release out today?
Created attachment 111996 [details] [review] Updated patch
Created attachment 111999 [details] [review] Updated patch Added checks for fontconfig. In fact, replaced the check for Xft with check for fontconfig. The code doesn't depend on Xft. It was just checking for Xft to avoid doing unnecessary work if no one is listening on those changes. But the Xft font settings are also used by cairo and other pangofc backends. So the proper conditional is really whether fontconfig (or pangofc) rather than Xft. Patch should be complete now.
And from the Department of Nits: monitor_files should not have a return value since it is always ignored. Re. indentation, gsd-xsettings-manager.c has 8-space indentation while fontconfig-monitor.c has 2-space indentation. That is inconsistent, but not necessarily your inconsistency.
(In reply to comment #9) > (In reply to comment #7) > > Also, identation looks off in a few places, > > Can you elaborate? Is it that it uses tabs? Converted to 8 spaces. Coming. Yeah, that looks better. > > and the license header looks odd? > > How? Why? It doesn't state any license, for example. > Copied from neighboring xsettings-common.c and updated date and > author. That's I guess some of the oldest code we have in there. Maybe you could just use the standard GPL header used in most other files? > Do you plan to push a release out today? Ideally, yes, though that looks to be a pretty tight schedule. +#ifdef __cplusplus +extern "C" { +#endif /* __cplusplus */ That looks like it should be G_BEGIN_DECLS. @@ -87,6 +89,7 @@ struct GnomeXSettingsManagerPrivate { XSettingsManager **managers; guint notify[4]; + fontconfig_monitor_handle_t *fontconfig_handle; }; This part seems to need HAVE_FONTCONFIG as well. dnl --------------------------------------------------------------------------- -dnl - XFT2 +dnl - Fontconfig dnl --------------------------------------------------------------------------- -if $PKG_CONFIG --exists xft; then - AC_DEFINE(HAVE_XFT2, 1, [Define if Xft functionality is available]) +have_fontconfig=no +if $PKG_CONFIG --exists fontconfig; then + have_fontconfig=yes + AC_DEFINE(HAVE_FONTCONFIG, 1, [Define if Fontconfig functionality is available]) fi Contrary to the Xft case where we didn't use any Xft code, we do so with fontconfig so we need a proper check with PKG_CHECK_MODULES and subst the CFLAGS and LIBS accordingly. (In reply to comment #12) > Re. indentation, gsd-xsettings-manager.c has 8-space indentation > while fontconfig-monitor.c has 2-space indentation. That is > inconsistent, but not necessarily your inconsistency. Hell will freeze over before we can standardize that betweeen different files/authors/modules...
Created attachment 112002 [details] [review] Updated patch This is actually fun :). Resolved all the listed issues, including indentation...
Not quite. You need to use FONTCONFIG_CFLAGS and FONTCONFIG_LIBS, too. Also, found another one ;-): + if (g_file_test (str, G_FILE_TEST_IS_DIR)) + monitor = g_file_monitor_directory (file, G_FILE_MONITOR_NONE, NULL, NULL); + else + monitor = g_file_monitor_file (file, G_FILE_MONITOR_NONE, NULL, NULL); + + g_signal_connect (monitor, "changed", G_CALLBACK (stuff_changed), data); + + g_ptr_array_add (monitors, monitor); At least in the _directory case (and I suspect the docs are wrong for the file case), monitor can be NULL...
(In reply to comment #15) > Not quite. You need to use FONTCONFIG_CFLAGS and FONTCONFIG_LIBS, too. I do that. In a style different from yours though: +if HAVE_FONTCONFIG +libxsettings_la_SOURCES += \ + fontconfig-monitor.h \ + fontconfig-monitor.c \ + $(NULL) +libxsettings_la_CFLAGS += \ + $(FONTCONFIG_CFLAGS) +libxsettings_la_LIBADD += \ + $(FONTCONFIG_LIBS) +endif This is cleaner for my taste. > Also, found another one ;-): > > + if (g_file_test (str, G_FILE_TEST_IS_DIR)) > + monitor = g_file_monitor_directory (file, > G_FILE_MONITOR_NONE, NULL, NULL); > + else > + monitor = g_file_monitor_file (file, > G_FILE_MONITOR_NONE, NULL, NULL); > + > + g_signal_connect (monitor, "changed", G_CALLBACK > (stuff_changed), data); > + > + g_ptr_array_add (monitors, monitor); > > At least in the _directory case (and I suspect the docs are wrong for the file > case), monitor can be NULL... Right. Fixing.
Created attachment 112004 [details] [review] Updated patch
/me is heading out for a couple hours...
(In reply to comment #16) > I do that. In a style different from yours though: Ah, yes, missed that. My bad, sorry. Feel free to commit now.
Committed. Thanks for all the review. 2008-06-02 Behdad Esfahbod <behdad@gnome.org> * configure.ac: Check for fontconfig instead of xft2. * plugins/xsettings/Makefile.am: * plugins/xsettings/gsd-xsettings-manager.c (fontconfig_callback), (gnome_xsettings_manager_start), (gnome_xsettings_manager_stop): Send a Fontconfig/Timestamp xsettings notification whenever fontconfig configurations change. (bug #490374) * plugins/xsettings/fontconfig-monitor.c: * plugins/xsettings/fontconfig-monitor.h: Monitor fontconfig configuration files using gio.
Err. Speaking to mclasen about a slightly different mechanism on the X side. Using root properties instead of xsettings. Lemme revert in the mean time. Sorry about that.
So, Matthias suggests I use a root window property instead, given that we just need a notification signal and no value. In that case, probably makes more sense to do this as a separate plugin. Is that what you like to see, or just do it in xsettings? Thanks,
I'd do it in fonts, then, given that we probably won't change that for the cursors.
Is there anything in the font setup that actually requires a window? And on what display?
(In reply to comment #24) > Is there anything in the font setup that actually requires a window? > And on what display? No, no window. This thing is mainly per-host. But since gtk+ doesn't depend on dbus, we are using X to notify. So, root window of default screen of default display is what I'm doing. The gtk+ side of it is a bit more complex than the xsettings approach, but I'm fixing it today.
Created attachment 112184 [details] [review] Updated patch Ok, after playing with other ideas for a couple days, we came back to decide that xsettings is just fine. So I like ask for commit permission again. The only change since last time is that now I call time(NULL) once and use the value for all managers. This is necessary to avoid race conditions with multi-screens. Otherwise if the time returns two different values we end up invalidating all windows twice on the gtk side.
The GTK+ patch is committed now. May I go ahead and commit this again? Thanks.
Thanks. 2008-06-07 Behdad Esfahbod <behdad@gnome.org> (Commit this again) * configure.ac: Check for fontconfig instead of xft2. * plugins/xsettings/Makefile.am: * plugins/xsettings/gsd-xsettings-manager.c (fontconfig_callback), (gnome_xsettings_manager_start), (gnome_xsettings_manager_stop): Send a Fontconfig/Timestamp xsettings notification whenever fontconfig configurations change. (bug #490374) * plugins/xsettings/fontconfig-monitor.c: * plugins/xsettings/fontconfig-monitor.h: Monitor fontconfig configuration files using gio.