After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 490374 - monitor font configuration
monitor font configuration
Status: RESOLVED FIXED
Product: gnome-settings-daemon
Classification: Core
Component: plugins
unspecified
Other Linux
: Normal enhancement
: ---
Assigned To: gnome-settings-daemon-maint
gnome-settings-daemon-maint
Depends on:
Blocks:
 
 
Reported: 2007-10-26 04:17 UTC by Behdad Esfahbod
Modified: 2008-06-07 18:17 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch (8.06 KB, patch)
2008-06-02 08:39 UTC, Behdad Esfahbod
none Details | Review
Updated patch (9.23 KB, patch)
2008-06-02 19:57 UTC, Behdad Esfahbod
needs-work Details | Review
Updated patch (9.02 KB, patch)
2008-06-02 20:40 UTC, Behdad Esfahbod
none Details | Review
Updated patch (11.96 KB, patch)
2008-06-02 20:57 UTC, Behdad Esfahbod
needs-work Details | Review
Updated patch (12.11 KB, patch)
2008-06-02 21:19 UTC, Behdad Esfahbod
needs-work Details | Review
Updated patch (12.18 KB, patch)
2008-06-02 21:37 UTC, Behdad Esfahbod
none Details | Review
Updated patch (12.21 KB, patch)
2008-06-05 04:01 UTC, Behdad Esfahbod
committed Details | Review

Description Behdad Esfahbod 2007-10-26 04:17:54 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()
Comment 1 Behdad Esfahbod 2007-10-26 04:18:28 UTC
I was hoping that gnome-settings-daemon be doing some file monitoring already, but apparently I was wrong.
Comment 2 Behdad Esfahbod 2007-12-17 08:36:05 UTC
This has become just so easier with gio merged in.  I'll look into it.
Comment 3 Behdad Esfahbod 2008-06-02 08:39:09 UTC
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.
Comment 4 Morten Welinder 2008-06-02 15:48:50 UTC
It looks like a few "static" additions would be good.  "timeout" and
"monitors" seem like candidates.
Comment 5 Behdad Esfahbod 2008-06-02 19:57:07 UTC
Created attachment 111991 [details] [review]
Updated patch

Got rid of globals completely.
Comment 6 Morten Welinder 2008-06-02 20:08:17 UTC
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?
Comment 7 Jens Granseuer 2008-06-02 20:24:43 UTC
Also, identation looks off in a few places, and the license header looks odd? And font-config checks, please.
Comment 8 Behdad Esfahbod 2008-06-02 20:35:14 UTC
(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.
Comment 9 Behdad Esfahbod 2008-06-02 20:39:47 UTC
(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?
Comment 10 Behdad Esfahbod 2008-06-02 20:40:47 UTC
Created attachment 111996 [details] [review]
Updated patch
Comment 11 Behdad Esfahbod 2008-06-02 20:57:40 UTC
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.
Comment 12 Morten Welinder 2008-06-02 21:00:33 UTC
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.
Comment 13 Jens Granseuer 2008-06-02 21:06:09 UTC
(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...
Comment 14 Behdad Esfahbod 2008-06-02 21:19:07 UTC
Created attachment 112002 [details] [review]
Updated patch

This is actually fun :).

Resolved all the listed issues, including indentation...
Comment 15 Jens Granseuer 2008-06-02 21:27:21 UTC
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...
Comment 16 Behdad Esfahbod 2008-06-02 21:35:04 UTC
(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.
Comment 17 Behdad Esfahbod 2008-06-02 21:37:49 UTC
Created attachment 112004 [details] [review]
Updated patch
Comment 18 Behdad Esfahbod 2008-06-02 21:43:07 UTC
/me is heading out for a couple hours...
Comment 19 Jens Granseuer 2008-06-02 21:50:54 UTC
(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.
Comment 20 Behdad Esfahbod 2008-06-03 01:15:06 UTC
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.

Comment 21 Behdad Esfahbod 2008-06-03 01:25:33 UTC
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.
Comment 22 Behdad Esfahbod 2008-06-03 01:47:56 UTC
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,
Comment 23 Jens Granseuer 2008-06-03 07:26:27 UTC
I'd do it in fonts, then, given that we probably won't change that for the cursors.
Comment 24 Morten Welinder 2008-06-03 11:55:35 UTC
Is there anything in the font setup that actually requires a window?
And on what display?
Comment 25 Behdad Esfahbod 2008-06-03 15:24:25 UTC
(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.
Comment 26 Behdad Esfahbod 2008-06-05 04:01:50 UTC
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.
Comment 27 Behdad Esfahbod 2008-06-06 16:40:30 UTC
The GTK+ patch is committed now.  May I go ahead and commit this again?
Thanks.
Comment 28 Behdad Esfahbod 2008-06-07 18:17:48 UTC
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.