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

classic Classic list List threaded Threaded
20 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Robert Pluim
>>>>> On Wed, 19 Jun 2019 03:08:53 -0400 (EDT), [hidden email] (Paul Eggert) said:

    Paul> branch: master
    Paul> commit 50c5d5621cb5e6d7c86829ac4b776d81e47b2189
    Paul> Author: Paul Eggert <[hidden email]>
    Paul> Commit: Paul Eggert <[hidden email]>

    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

Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
> From: Robert Pluim <[hidden email]>
> Date: Tue, 05 Nov 2019 11:33:44 +0100
> Cc: Paul Eggert <[hidden email]>
>
> 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?

Reply | Threaded
Open this post in threaded view
|

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

Robert Pluim
>>>>> On Tue, 05 Nov 2019 20:25:46 +0200, Eli Zaretskii <[hidden email]> said:

    >> From: Robert Pluim <[hidden email]>
    >> Date: Tue, 05 Nov 2019 11:33:44 +0100
    >> Cc: Paul Eggert <[hidden email]>
    >>
    >> 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

Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
> From: Robert Pluim <[hidden email]>
> Date: Tue, 05 Nov 2019 21:07:20 +0100
> Cc: [hidden email], [hidden email]
>
> 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?

Reply | Threaded
Open this post in threaded view
|

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

Robert Pluim
>>>>> On Wed, 06 Nov 2019 05:34:43 +0200, Eli Zaretskii <[hidden email]> said:

    >> From: Robert Pluim <[hidden email]>
    >> Date: Tue, 05 Nov 2019 21:07:20 +0100
    >> Cc: [hidden email], [hidden email]
    >>
    >> 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);

Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
> From: Robert Pluim <[hidden email]>
> Cc: [hidden email],  [hidden email]
> 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?

Reply | Threaded
Open this post in threaded view
|

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

Robert Pluim
>>>>> On Wed, 06 Nov 2019 18:11:15 +0200, Eli Zaretskii <[hidden email]> said:

    >> From: Robert Pluim <[hidden email]>
    >> Cc: [hidden email],  [hidden email]
    >> 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

Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
> From: Robert Pluim <[hidden email]>
> Cc: [hidden email],  [hidden email]
> 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.

Reply | Threaded
Open this post in threaded view
|

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

Robert Pluim
>>>>> On Thu, 07 Nov 2019 16:31:34 +0200, Eli Zaretskii <[hidden email]> said:

    >> From: Robert Pluim <[hidden email]>
    >> Cc: [hidden email],  [hidden email]
    >> 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

Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
> From: Robert Pluim <[hidden email]>
> Cc: [hidden email],  [hidden email]
> 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.

Reply | Threaded
Open this post in threaded view
|

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

Robert Pluim
>>>>> On Thu, 07 Nov 2019 19:24:16 +0200, Eli Zaretskii <[hidden email]> 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:


From bc38e74a504d0413f7a9c4a2395917601b260035 Mon Sep 17 00:00:00 2001
From: Robert Pluim <[hidden email]>
Date: Fri, 8 Nov 2019 21:07:10 +0100
Subject: [PATCH] Make GTK font chooser respect face-ignored-fonts
To: [hidden email]

* 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,
 }
 
 
+/* 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

Reply | Threaded
Open this post in threaded view
|

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.

Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
In reply to this post by Robert Pluim
> From: Robert Pluim <[hidden email]>
> Cc: [hidden email]
> 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?

Reply | Threaded
Open this post in threaded view
|

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

Robert Pluim
In reply to this post by Paul Eggert
>>>>> On Fri, 8 Nov 2019 14:59:34 -0800, Paul Eggert <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

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

Robert Pluim
In reply to this post by Eli Zaretskii
>>>>> On Sat, 09 Nov 2019 08:44:56 +0200, Eli Zaretskii <[hidden email]> said:

    >> From: Robert Pluim <[hidden email]>
    >> Cc: [hidden email]
    >> 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.

Reply | Threaded
Open this post in threaded view
|

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

Stefan Monnier
In reply to this post by Robert Pluim
> 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


Reply | Threaded
Open this post in threaded view
|

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

Robert Pluim
In reply to this post by Robert Pluim
>>>>> On Sat, 09 Nov 2019 08:44:56 +0200, Eli Zaretskii <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
In reply to this post by Robert Pluim
> From: Robert Pluim <[hidden email]>
> Cc: [hidden email]
> 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.

Reply | Threaded
Open this post in threaded view
|

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

Robert Pluim
>>>>> On Thu, 14 Nov 2019 13:53:45 +0200, Eli Zaretskii <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
In reply to this post by Robert Pluim
> From: Robert Pluim <[hidden email]>
> Cc: [hidden email]
> Date: Thu, 14 Nov 2019 10:48:10 +0100
>
> I called it font_is_ignored. Pushed as ca44f33be2

Thanks.