Created attachment 338546 [details] [review]
fontconfig-monitor: do not recreate configuration every event

Currently, the fontconfig monitor will call FcInitReinitialize() for
every event triggered by the file monitor on the fontconfig
configuration directories.

For large font files this creates a situation where the configuration is
reinitialized before the file has been fully copied and available for
use in fontconfig. The result is that the cache mtime for that directory
will be updated to use the current modification time, with the new font
file not registered in the cache yet, preventing the new font to be
avaialble to applications until the fontconfig cache is forcefully
recreated.

We can fix this by ignoring CREATED and CHANGED events from the file
monitor; in both those cases we will receive a CHANGES_DONE_HINT event
when the file is completely copied in the directory.

https://phabricator.endlessm.com/T13909

Review of attachment 338546 [details] [review]:

::: plugins/xsettings/fontconfig-monitor.c
@@ +113,3 @@
+         */
+        if (event_type == G_FILE_MONITOR_EVENT_CHANGED ||
+            event_type == G_FILE_MONITOR_EVENT_CREATED)

This isn't good enough though. The G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT event is not sent for the FAM monitor, which is used when the home directory is on NFS. Instead, you should queue up a timeout (the local polling monitor uses 5 seconds, which is probably a decent default), which you'd push back for every event received.

(In reply to Bastien Nocera from comment #2)
> This isn't good enough though. The G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT
> event is not sent for the FAM monitor, which is used when the home directory
> is on NFS. 

I am not sure that's correct... the FAM file monitor uses GFileMonitorSource internally which will end up emitting the G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT.

See https://git.gnome.org/browse/glib/tree/gio/glocalfilemonitor.c#n272

(In reply to Cosimo Cecchi from comment #3)
> (In reply to Bastien Nocera from comment #2)
> > This isn't good enough though. The G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT
> > event is not sent for the FAM monitor, which is used when the home directory
> > is on NFS. 
> 
> I am not sure that's correct... the FAM file monitor uses GFileMonitorSource
> internally which will end up emitting the
> G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT.
> 
> See https://git.gnome.org/browse/glib/tree/gio/glocalfilemonitor.c#n272

Fine then. But please remove the reference to the internal phabricator instance, we can't read that bug (would be nice to know the origin story of that bug though).

Attachment 338546 [details] pushed as f1b8257 - fontconfig-monitor: do not recreate configuration every event

(In reply to Bastien Nocera from comment #4)
> Fine then. But please remove the reference to the internal phabricator
> instance, we can't read that bug (would be nice to know the origin story of
> that bug though).

Whoops, I forget that every time! Removed now.
The story is that clicking on the Install button of gnome-font-viewer was failing for a specific font file that was very large (around 20MB). That was caused by multiple issues -- see https://git.gnome.org/browse/gnome-font-viewer/commit/?id=15c98a962ae1fe5bea03c71943313c3abffbe6e6 for another piece of it.