Re: [Emacs-diffs] master 50c5d56: --with-cairo is no longer experimental

Re: [Emacs-diffs] master 50c5d56: --with-cairo is no longer experimental

[Robert Pluim]

>>>>> On Wed, 19 Jun 2019 03:08:53 -0400 (EDT), eggert@cs.ucla.edu (Paul Eggert) said:

    Paul> branch: master
    Paul> commit 50c5d5621cb5e6d7c86829ac4b776d81e47b2189
    Paul> Author: Paul Eggert <eggert@cs.ucla.edu>
    Paul> Commit: Paul Eggert <eggert@cs.ucla.edu>

    Paul>     --with-cairo is no longer experimental
    
    Paul>     Suggested by YAMAMOTO Mitsuharu in:
    Paul>     https://lists.gnu.org/r/emacs-devel/2019-06/msg00686.html
    Paul>     * configure.ac: Do not say --with-cairo is experimental.
    Paul>     * etc/NEWS: Say that --with-cairo is no longer experimental.

Hmm, how far are we from turning cairo on by default? Or from
*requiring* it, I should say? The reason I ask is that the existing
Xft support suffers from too many easily triggered bugs, and one way
to get away from Xft is to use cairo.

Robert

Re: [Emacs-diffs] master 50c5d56: --with-cairo is no longer experimental

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Date: Tue, 05 Nov 2019 11:33:44 +0100
> Cc: Paul Eggert <eggert@cs.ucla.edu>
> 
> Hmm, how far are we from turning cairo on by default? Or from
> *requiring* it, I should say? The reason I ask is that the existing
> Xft support suffers from too many easily triggered bugs, and one way
> to get away from Xft is to use cairo.

Do we know the Cairo build to be more reliable than the xft build?

Re: [Emacs-diffs] master 50c5d56: --with-cairo is no longer experimental

[Robert Pluim]

>>>>> On Tue, 05 Nov 2019 20:25:46 +0200, Eli Zaretskii <eliz@gnu.org> said:

    >> From: Robert Pluim <rpluim@gmail.com>
    >> Date: Tue, 05 Nov 2019 11:33:44 +0100
    >> Cc: Paul Eggert <eggert@cs.ucla.edu>
    >> 
    >> Hmm, how far are we from turning cairo on by default? Or from
    >> *requiring* it, I should say? The reason I ask is that the existing
    >> Xft support suffers from too many easily triggered bugs, and one way
    >> to get away from Xft is to use cairo.

    Eli> Do we know the Cairo build to be more reliable than the xft build?

I can crash the Xft one in about 2 seconds, even with my recent
patch. Iʼve not run the Cairo build in anger, but it seems stable.

Robert

Re: [Emacs-diffs] master 50c5d56: --with-cairo is no longer experimental

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Date: Tue, 05 Nov 2019 21:07:20 +0100
> Cc: eggert@cs.ucla.edu, emacs-devel@gnu.org
> 
> I can crash the Xft one in about 2 seconds, even with my recent
> patch. Iʼve not run the Cairo build in anger, but it seems stable.

Can't we provide some band-aid to prevent Xft from crashing Emacs?

Re: [Emacs-diffs] master 50c5d56: --with-cairo is no longer experimental

[Robert Pluim]

>>>>> On Wed, 06 Nov 2019 05:34:43 +0200, Eli Zaretskii <eliz@gnu.org> said:

    >> From: Robert Pluim <rpluim@gmail.com>
    >> Date: Tue, 05 Nov 2019 21:07:20 +0100
    >> Cc: eggert@cs.ucla.edu, emacs-devel@gnu.org
    >> 
    >> I can crash the Xft one in about 2 seconds, even with my recent
    >> patch. Iʼve not run the Cairo build in anger, but it seems stable.

    Eli> Can't we provide some band-aid to prevent Xft from crashing Emacs?

We could filter out 'Noto Color Emoji' in gtkutil.c:xg_get_font, but
the relevant GTK interface is marked as deprecated, so weʼd have to
match on font name or something. Itʼs not pretty:

diff --git a/src/gtkutil.c b/src/gtkutil.c
index 16d765533a..ccdba27ff2 100644
--- a/src/gtkutil.c
+++ b/src/gtkutil.c
@@ -2281,12 +2281,14 @@ xg_get_font (struct frame *f, const char *default_name)
 	  gint        size   = pango_font_description_get_size (desc);
 	  PangoWeight weight = pango_font_description_get_weight (desc);
 	  PangoStyle  style  = pango_font_description_get_style (desc);
-
-	  font = CALLN (Ffont_spec,
-			QCfamily, build_string (family),
-			QCsize, make_float (pango_units_to_double (size)),
-			QCweight, XG_WEIGHT_TO_SYMBOL (weight),
-			QCslant, XG_STYLE_TO_SYMBOL (style));
+          if (strcmp (family, "Noto Color Emoji") == 0)
+            font = Qnil;
+          else
+            font = CALLN (Ffont_spec,
+                          QCfamily, build_string (family),
+                          QCsize, make_float (pango_units_to_double (size)),
+                          QCweight, XG_WEIGHT_TO_SYMBOL (weight),
+                          QCslant, XG_STYLE_TO_SYMBOL (style));
 
 	  pango_font_description_free (desc);
 	  dupstring (&x_last_font_name, family);

Re: [Emacs-diffs] master 50c5d56: --with-cairo is no longer experimental

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: eggert@cs.ucla.edu,  emacs-devel@gnu.org
> Date: Wed, 06 Nov 2019 08:39:54 +0100
> 
>     Eli> Can't we provide some band-aid to prevent Xft from crashing Emacs?
> 
> We could filter out 'Noto Color Emoji' in gtkutil.c:xg_get_font, but
> the relevant GTK interface is marked as deprecated, so weʼd have to
> match on font name or something. Itʼs not pretty:

If the problem is a single font (or a small number of known fonts), we
could disable them through face-ignored-fonts, conditioned on the
right symbols that identify the problematic build(s).  Does that make
sense?

Re: [Emacs-diffs] master 50c5d56: --with-cairo is no longer experimental

[Robert Pluim]

>>>>> On Wed, 06 Nov 2019 18:11:15 +0200, Eli Zaretskii <eliz@gnu.org> said:

    >> From: Robert Pluim <rpluim@gmail.com>
    >> Cc: eggert@cs.ucla.edu,  emacs-devel@gnu.org
    >> Date: Wed, 06 Nov 2019 08:39:54 +0100
    >> 
    Eli> Can't we provide some band-aid to prevent Xft from crashing Emacs?
    >> 
    >> We could filter out 'Noto Color Emoji' in gtkutil.c:xg_get_font, but
    >> the relevant GTK interface is marked as deprecated, so weʼd have to
    >> match on font name or something. Itʼs not pretty:

    Eli> If the problem is a single font (or a small number of known fonts), we
    Eli> could disable them through face-ignored-fonts, conditioned on the
    Eli> right symbols that identify the problematic build(s).  Does that make
    Eli> sense?

It makes sense, but unfortunately the GTK font widget code doesnʼt
check face-ignored-fonts, so weʼd have to hoist that code out of
font_list_entities.

Robert

Re: [Emacs-diffs] master 50c5d56: --with-cairo is no longer experimental

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: eggert@cs.ucla.edu,  emacs-devel@gnu.org
> Date: Thu, 07 Nov 2019 09:48:27 +0100
> 
>     Eli> If the problem is a single font (or a small number of known fonts), we
>     Eli> could disable them through face-ignored-fonts, conditioned on the
>     Eli> right symbols that identify the problematic build(s).  Does that make
>     Eli> sense?
> 
> It makes sense, but unfortunately the GTK font widget code doesnʼt
> check face-ignored-fonts, so weʼd have to hoist that code out of
> font_list_entities.

Just making GTK font selection take face-ignored-fonts into account
sounds like a nice improvement to me, almost a bugfix.  So if this is
not too tricky to implement, I think we should go this way.

Thanks.

Re: [Emacs-diffs] master 50c5d56: --with-cairo is no longer experimental

[Robert Pluim]

>>>>> On Thu, 07 Nov 2019 16:31:34 +0200, Eli Zaretskii <eliz@gnu.org> said:

    >> From: Robert Pluim <rpluim@gmail.com>
    >> Cc: eggert@cs.ucla.edu,  emacs-devel@gnu.org
    >> Date: Thu, 07 Nov 2019 09:48:27 +0100
    >> 
    Eli> If the problem is a single font (or a small number of known fonts), we
    Eli> could disable them through face-ignored-fonts, conditioned on the
    Eli> right symbols that identify the problematic build(s).  Does that make
    Eli> sense?
    >> 
    >> It makes sense, but unfortunately the GTK font widget code doesnʼt
    >> check face-ignored-fonts, so weʼd have to hoist that code out of
    >> font_list_entities.

    Eli> Just making GTK font selection take face-ignored-fonts into account
    Eli> sounds like a nice improvement to me, almost a bugfix.  So if this is
    Eli> not too tricky to implement, I think we should go this way.

Itʼs not too hard, I can get to it in the next week or so.

Robert

Re: [Emacs-diffs] master 50c5d56: --with-cairo is no longer experimental

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: eggert@cs.ucla.edu,  emacs-devel@gnu.org
> Date: Thu, 07 Nov 2019 17:11:55 +0100
> 
>     >> It makes sense, but unfortunately the GTK font widget code doesnʼt
>     >> check face-ignored-fonts, so weʼd have to hoist that code out of
>     >> font_list_entities.
> 
>     Eli> Just making GTK font selection take face-ignored-fonts into account
>     Eli> sounds like a nice improvement to me, almost a bugfix.  So if this is
>     Eli> not too tricky to implement, I think we should go this way.
> 
> Itʼs not too hard, I can get to it in the next week or so.

Thank you.

Re: [Emacs-diffs] master 50c5d56: --with-cairo is no longer experimental

[Robert Pluim]

[-- Attachment #1: Type: text/plain, Size: 441 bytes --]

>>>>> On Thu, 07 Nov 2019 19:24:16 +0200, Eli Zaretskii <eliz@gnu.org> said:

    Eli> Just making GTK font selection take face-ignored-fonts into account
    Eli> sounds like a nice improvement to me, almost a bugfix.  So if this is
    Eli> not too tricky to implement, I think we should go this way.
    >> 
    >> Itʼs not too hard, I can get to it in the next week or so.

    Eli> Thank you.

The following works for me:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Make-GTK-font-chooser-respect-face-ignored-fonts.patch --]
[-- Type: text/x-patch, Size: 3960 bytes --]

From bc38e74a504d0413f7a9c4a2395917601b260035 Mon Sep 17 00:00:00 2001
From: Robert Pluim <rpluim@gmail.com>
Date: Fri, 8 Nov 2019 21:07:10 +0100
Subject: [PATCH] Make GTK font chooser respect face-ignored-fonts
To: emacs-devel@gnu.org

* src/font.c (font_matches_ignored_fonts): New function, abstracts
out check for matching Vface_ignored_fonts.
(font_delete_unmatched): Use it.

* src/gtkutil.h: Add prototype for font_matches_ignored_fonts.
* src/gtkutil.c (xg_font_filter): New function, filters out fonts
from the GTK font chooser.  Uses font_matches_ignored_fonts.
(xg_get_font): Set the GTK chooser font filter to xg_font_filter.
---
 src/font.c    | 34 ++++++++++++++++++++--------------
 src/gtkutil.c | 18 ++++++++++++++++++
 src/gtkutil.h |  5 +++++
 3 files changed, 43 insertions(+), 14 deletions(-)

diff --git a/src/font.c b/src/font.c
index 8dfbfa0fac..9f92260d0c 100644
--- a/src/font.c
+++ b/src/font.c
@@ -2655,6 +2655,24 @@ font_clear_cache (struct frame *f, Lisp_Object cache,
 }
 \f
 
+/* Check whether NAME should be ignored based on Vface_ignored_fonts.
+   This is reused by xg_font_filter to apply the same checks to the
+   GTK font chooser.  */
+
+bool
+font_matches_ignored_fonts (const char *name, ptrdiff_t namelen)
+{
+  Lisp_Object tail, regexp;
+  for (tail = Vface_ignored_fonts; CONSP (tail); tail = XCDR (tail))
+    {
+      regexp = XCAR (tail);
+      if (STRINGP (regexp)
+          && fast_c_string_match_ignore_case (regexp, name,
+                                              namelen) >= 0)
+        return true;
+    }
+  return false;
+}
 static Lisp_Object scratch_font_spec, scratch_font_prefer;
 
 /* Check each font-entity in VEC, and return a list of font-entities
@@ -2677,22 +2695,10 @@ font_delete_unmatched (Lisp_Object vec, Lisp_Object spec, int size)
 	{
 	  char name[256];
 	  ptrdiff_t namelen;
-	  Lisp_Object tail, regexp;
-
 	  namelen = font_unparse_xlfd (entity, 0, name, 256);
 	  if (namelen >= 0)
-	    {
-	      for (tail = Vface_ignored_fonts; CONSP (tail); tail = XCDR (tail))
-		{
-		  regexp = XCAR (tail);
-		  if (STRINGP (regexp)
-		      && fast_c_string_match_ignore_case (regexp, name,
-							  namelen) >= 0)
-		    break;
-		}
-	      if (CONSP (tail))
-		continue;
-	    }
+            if (font_matches_ignored_fonts (name, namelen))
+                continue;
 	}
       if (NILP (spec))
 	{
diff --git a/src/gtkutil.c b/src/gtkutil.c
index 16d765533a..0cda21cdd1 100644
--- a/src/gtkutil.c
+++ b/src/gtkutil.c
@@ -2228,6 +2228,21 @@ xg_get_file_name (struct frame *f,
 
 static char *x_last_font_name;
 
+#if GTK_CHECK_VERSION (3, 2, 0)
+static gboolean
+xg_font_filter (const PangoFontFamily *family,
+                const PangoFontFace *face,
+                gpointer data)
+{
+  const char *name = pango_font_family_get_name ((PangoFontFamily *)family);
+  ptrdiff_t namelen = strlen (name);
+
+  if (font_matches_ignored_fonts (name, namelen))
+    return FALSE;
+  return TRUE;
+}
+#endif
+
 /* Pop up a GTK font selector and return the name of the font the user
    selects, as a C string.  The returned font name follows GTK's own
    format:
@@ -2247,6 +2262,9 @@ xg_get_font (struct frame *f, const char *default_name)
   w = gtk_font_chooser_dialog_new
     ("Pick a font", GTK_WINDOW (FRAME_GTK_OUTER_WIDGET (f)));
 
+#if GTK_CHECK_VERSION (3, 2, 0)
+  gtk_font_chooser_set_filter_func (GTK_FONT_CHOOSER (w), xg_font_filter, NULL, NULL);
+#endif
   if (default_name)
     {
       /* Convert fontconfig names to Gtk names, i.e. remove - before
diff --git a/src/gtkutil.h b/src/gtkutil.h
index 229aa08f81..fea3271637 100644
--- a/src/gtkutil.h
+++ b/src/gtkutil.h
@@ -203,5 +203,10 @@ extern void xg_initialize (void);
 extern bool xg_ignore_gtk_scrollbar;
 
 extern bool xg_gtk_initialized;
+
+#if GTK_CHECK_VERSION (3, 2, 0)
+extern bool font_matches_ignored_fonts (const char *, ptrdiff_t);
+#endif
+
 #endif /* USE_GTK */
 #endif /* GTKUTIL_H */
-- 
2.23.0

Re: [Emacs-diffs] master 50c5d56: --with-cairo is no longer experimental

[Paul Eggert]

On 11/8/19 1:40 PM, Robert Pluim wrote:
> +  Lisp_Object tail, regexp;
> +  for (tail = Vface_ignored_fonts; CONSP (tail); tail = XCDR (tail))

Thanks for writing that. This part should use FOR_EACH_TAIL_SAFE to 
avoid worst-case hangs.

Re: [Emacs-diffs] master 50c5d56: --with-cairo is no longer experimental

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: emacs-devel@gnu.org
> Date: Fri, 08 Nov 2019 22:40:52 +0100
> 
> The following works for me:

LGTM, thanks.  I would just change the name of the new function to
something like font_is_ignored, which is slightly more general, and so
will still fit if we later modify that function to do something
instead of in addition to matching against face-ignored-fonts.  But
that's admittedly splitting hair.

Another thing that I miss in the patch is the actual setting of
face-ignored-fonts for GTK builds.  We want to do that by default,
yes?

Re: [Emacs-diffs] master 50c5d56: --with-cairo is no longer experimental

[Robert Pluim]

>>>>> On Fri, 8 Nov 2019 14:59:34 -0800, Paul Eggert <eggert@cs.ucla.edu> said:

    Paul> On 11/8/19 1:40 PM, Robert Pluim wrote:
    >> +  Lisp_Object tail, regexp;
    >> +  for (tail = Vface_ignored_fonts; CONSP (tail); tail = XCDR (tail))

    Paul> Thanks for writing that. This part should use FOR_EACH_TAIL_SAFE to
    Paul> avoid worst-case hangs.

OK. Although a part of me says that anyone who sets face-ignored-fonts
to a circular list knows what's up, and deserves the results :-)

Robert

Re: [Emacs-diffs] master 50c5d56: --with-cairo is no longer experimental

[Robert Pluim]

>>>>> On Sat, 09 Nov 2019 08:44:56 +0200, Eli Zaretskii <eliz@gnu.org> said:

    >> From: Robert Pluim <rpluim@gmail.com>
    >> Cc: emacs-devel@gnu.org
    >> Date: Fri, 08 Nov 2019 22:40:52 +0100
    >> 
    >> The following works for me:

    Eli> LGTM, thanks.  I would just change the name of the new function to
    Eli> something like font_is_ignored, which is slightly more general, and so
    Eli> will still fit if we later modify that function to do something
    Eli> instead of in addition to matching against face-ignored-fonts.  But
    Eli> that's admittedly splitting hair.

Naming is difficult, and Iʼm bad at it. Strictly speaking you could
say it should be 'font_font_is_ignored', since the first 'font_' is
the subsystem prefix, but 'font_is_ignored' works for me.

    Eli> Another thing that I miss in the patch is the actual setting of
    Eli> face-ignored-fonts for GTK builds.  We want to do that by default,
    Eli> yes?

We do, but thatʼs a separate and independent patch (I should probably
put something in PROBLEMS as well).

diff --git i/src/xfaces.c w/src/xfaces.c
index 3806fa90e2..35feca7e4e 100644
--- i/src/xfaces.c
+++ w/src/xfaces.c
@@ -6770,12 +6770,15 @@ syms_of_xfaces (void)
               doc: /* List of ignored fonts.
 Each element is a regular expression that matches names of fonts to
 ignore.  */);
-#ifdef HAVE_OTF_KANNADA_BUG
-  /* https://debbugs.gnu.org/30193  */
-  Vface_ignored_fonts = list1 (build_string ("Noto Serif Kannada"));
+#ifdef HAVE_XFT
+  Vface_ignored_fonts = list1 (build_string ("Noto Color Emoji"));
 #else
   Vface_ignored_fonts = Qnil;
 #endif
+#ifdef HAVE_OTF_KANNADA_BUG
+  /* https://debbugs.gnu.org/30193  */
+  Vface_ignored_fonts = Fcons (build_string ("Noto Serif Kannada"), Vface_ignored_fonts);
+#endif

   DEFVAR_LISP ("face-remapping-alist", Vface_remapping_alist,
               doc: /* Alist of face remappings.

Re: [Emacs-diffs] master 50c5d56: --with-cairo is no longer experimental

[Stefan Monnier]

> OK. Although a part of me says that anyone who sets face-ignored-fonts
> to a circular list knows what's up, and deserves the results :-)

I tend to feel the same way, but I think the reality is diametrically
opposed: someone who sets face-ignored-fonts to a circular list is much
more likely to do that by accident (I'd guess something like using
`nconc` together with a quoted list) and would have no clue what's
going on.


        Stefan

Re: [Emacs-diffs] master 50c5d56: --with-cairo is no longer experimental

[Robert Pluim]

>>>>> On Sat, 09 Nov 2019 08:44:56 +0200, Eli Zaretskii <eliz@gnu.org> said:

    Eli> LGTM, thanks.  I would just change the name of the new function to
    Eli> something like font_is_ignored, which is slightly more general, and so
    Eli> will still fit if we later modify that function to do something
    Eli> instead of in addition to matching against face-ignored-fonts.  But
    Eli> that's admittedly splitting hair.

I called it font_is_ignored. Pushed as ca44f33be2

Robert

Re: [Emacs-diffs] master 50c5d56: --with-cairo is no longer experimental

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: emacs-devel@gnu.org
> Date: Tue, 12 Nov 2019 08:51:25 +0100
> 
> --- i/src/xfaces.c
> +++ w/src/xfaces.c
> @@ -6770,12 +6770,15 @@ syms_of_xfaces (void)
>                doc: /* List of ignored fonts.
>  Each element is a regular expression that matches names of fonts to
>  ignore.  */);
> -#ifdef HAVE_OTF_KANNADA_BUG
> -  /* https://debbugs.gnu.org/30193  */
> -  Vface_ignored_fonts = list1 (build_string ("Noto Serif Kannada"));
> +#ifdef HAVE_XFT
> +  Vface_ignored_fonts = list1 (build_string ("Noto Color Emoji"));
>  #else
>    Vface_ignored_fonts = Qnil;
>  #endif
> +#ifdef HAVE_OTF_KANNADA_BUG
> +  /* https://debbugs.gnu.org/30193  */
> +  Vface_ignored_fonts = Fcons (build_string ("Noto Serif Kannada"), Vface_ignored_fonts);
> +#endif

LGTM, but please also add a short description of the reason why these
fonts are excluded, so that the readers won't need to look up the full
bug discussion.

Re: [Emacs-diffs] master 50c5d56: --with-cairo is no longer experimental

[Robert Pluim]

>>>>> On Thu, 14 Nov 2019 13:53:45 +0200, Eli Zaretskii <eliz@gnu.org> said:

    Eli> LGTM, but please also add a short description of the reason why these
    Eli> fonts are excluded, so that the readers won't need to look up the full
    Eli> bug discussion.

OK, done.

Robert

Re: [Emacs-diffs] master 50c5d56: --with-cairo is no longer experimental

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: emacs-devel@gnu.org
> Date: Thu, 14 Nov 2019 10:48:10 +0100
> 
> I called it font_is_ignored. Pushed as ca44f33be2

Thanks.