GNU bug report logs - #79285
[Patch] support :font-features in face

Previous Next

To reply to this bug, email your comments to 79285 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Binbin YE <phantom2501 <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: [Patch] support :font-features in face
Date: Thu, 21 Aug 2025 23:28:53 +0900

[Message part 1 (text/plain, inline)]

Greetings,

It is my first time trying to contribute to Emacs source code.

The change is adding support for enabling stylistic set (font features)
when using harfbuzz font backend. It is a commonly supported feature in the
editors. See following links

https://code.visualstudio.com/docs/terminal/appearance#_font-feature-settings

https://developer.mozilla.org/en-US/docs/Web/CSS/font-feature-settings

The change adds :font-feature configuration like following

(set-face-attribute 'default nil :font "JetBrains Mono" :height 80
  :font-features '((zero . 1) (ss19 . 0) (calt . 1)))

The render result can be confirmed by adding "0" to composition table

(set-char-table-range composition-function-table
  ?0
  '(["." 0 font-shape-gstring]))

I have read the CONTRIBUTE file and tried to make the commit message as
clear as possible. Please let me know the code review process to get the
patch accepted.


Best,

Binbin

[Message part 2 (text/html, inline)]

[0001-support-configuring-font-features-in-face.patch (text/x-patch, attachment)]

Message #8 received at 79285 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Binbin YE <phantom2501 <at> gmail.com>
Cc: 79285 <at> debbugs.gnu.org
Subject: Re: bug#79285: [Patch] support :font-features in face
Date: Sat, 23 Aug 2025 15:51:16 +0300

> From: Binbin YE <phantom2501 <at> gmail.com>
> Date: Thu, 21 Aug 2025 23:28:53 +0900
> 
> It is my first time trying to contribute to Emacs source code.
> 
> The change is adding support for enabling stylistic set (font features) when using harfbuzz font backend. It is
> a commonly supported feature in the editors. See following links
> 
> https://code.visualstudio.com/docs/terminal/appearance#_font-feature-settings
> 
> https://developer.mozilla.org/en-US/docs/Web/CSS/font-feature-settings
> 
> The change adds :font-feature configuration like following
> 
> (set-face-attribute 'default nil :font "JetBrains Mono" :height 80
>   :font-features '((zero . 1) (ss19 . 0) (calt . 1)))
> 
> The render result can be confirmed by adding "0" to composition table 
> 
> (set-char-table-range composition-function-table
>   ?0
>   '(["." 0 font-shape-gstring]))

Thanks, this is an important feature to have in Emacs.

If implemented as a face property, as you have done in your patch, we
should also modify the face-merging code to be able to merge two lists
of features int a single list that includes all of the features,
right?  This is what should happen when two faces with non-nil
:font-features attribute are merged.

> I have read the CONTRIBUTE file and tried to make the commit message as clear as possible. Please let me
> know the code review process to get the patch accepted.

See a few comments below.

In addition, this will need s NEWS item and a suitable addition to the
ELisp manual, where the face attributes are described.

Last, but not least, to accept a contribution of this size, we will
need you to sign a copyright-assignment agreement with the FSF.  If
you are prepared to do that, I will send you the form to fill and the
instructions to go with it.

> +      /* Store font features from face attributes for use during shaping */
                                                                          ^^
Our style is to end the commend with a period and two spaces, before
"*/".

> +      if (CONSP (font_features))
> +          font_put_extra (entity, QCfont_features, font_features);
         ^^^^
We use indentation offset of 2 columns in C sources.

> +/* Convert Lisp font features to HarfBuzz features.
> +   FEATURES is a list of (feature . value) pairs.
> +   Returns the number of features converted, fills HBFEATURES array.
> +   Caller must ensure HBFEATURES has enough space. */
                                                    ^^
Two spaces there.

About "enough space" -- how many features could a font have?  And what
to do if it has more than 32?  GNU coding conventions frown on
arbitrary limits, and 32 sounds like it's quite arbitrary.  Can we do
better?

> +          Lisp_Object feature_sym = XCAR (feature_spec);
> +          Lisp_Object feature_val = XCDR (feature_spec);
> +
> +          if (SYMBOLP (feature_sym) && FIXNUMP (feature_val))

Are we sure the value cannot be larger than most-positive-fixnum?
Emacs can handle larger values if needed.

> @@ -491,7 +526,20 @@ hbfont_shape (Lisp_Object lgstring, Lisp_Object direction)
>    if (!hb_font)
>      return make_fixnum (0);
>  
> -  hb_bool_t success = hb_shape_full (hb_font, hb_buffer, NULL, 0, NULL);
> +
> +  /* Get font features from the font object */
> +  Lisp_Object font_features = Ffont_get (LGSTRING_FONT (lgstring), QCfont_features);
> +  static hb_feature_t features[32];  /* Cache features array, reasonable max size */
> +  int num_features = 0;
> +
> +  if (!NILP (font_features))
> +    {
> +      num_features = hb_features_from_lisp (font_features, features, 32);
> +    }
> +
> +  hb_bool_t success = hb_shape_full (hb_font, hb_buffer, features, num_features, NULL);

I think it is safer to pass NULL instead of 'features' if we know
there are no features.

Also, our style is not to use braces when the block has only one
statement.

> @@ -2917,6 +2923,13 @@ merge_face_ref (struct window *w,
>  		  else
>  		    err = true;
>  		}
> +	      else if (EQ (keyword, QCfont_features))
> +		{
> +		  if (NILP (value) || CONSP (value))
> +		    to[LFACE_FONT_FEATURES_INDEX] = value;
> +		  else
> +		    err = true;
> +		}

As mentioned above, I think we should be smarter here: instead of
overwriting the value of :font-features of the target face, we should
merge the two lists so that it includes the features from both faces.
Font features from FROM should override the same features from TO, but
font features which exist in TO but not in FROM should be left in the
resulting list.

Thanks again for working on this, and for your interest in Emacs.

Message #11 received at 79285 <at> debbugs.gnu.org (full text, mbox):

From: Binbin YE <phantom2501 <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 79285 <at> debbugs.gnu.org
Subject: Re: bug#79285: [Patch] support :font-features in face
Date: Sun, 24 Aug 2025 09:17:10 +0900

[Message part 1 (text/plain, inline)]

Hi Eli,

Thank you for the review of the patch. I'll update the changes in short,
together with NEWS and the ELisp manual.

> Last, but not least, to accept a contribution of this size, we will
> need you to sign a copyright-assignment agreement with the FSF.  If
> you are prepared to do that, I will send you the form to fill and the
> instructions to go with it.

I am aware of the existence of the agreement, and I am ready to sign it.
While I'm working on the patch, we can proceed the paperwork in parallel

Best,

Binbin

[Message part 2 (text/html, inline)]

Message #14 received at 79285 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Binbin YE <phantom2501 <at> gmail.com>
Cc: 79285 <at> debbugs.gnu.org
Subject: Re: bug#79285: [Patch] support :font-features in face
Date: Sun, 24 Aug 2025 08:53:06 +0300

> From: Binbin YE <phantom2501 <at> gmail.com>
> Date: Sun, 24 Aug 2025 09:17:10 +0900
> Cc: 79285 <at> debbugs.gnu.org
> 
> Thank you for the review of the patch. I'll update the changes in short, together with NEWS and the ELisp
> manual.

Thanks.

> > Last, but not least, to accept a contribution of this size, we will
> > need you to sign a copyright-assignment agreement with the FSF.  If
> > you are prepared to do that, I will send you the form to fill and the
> > instructions to go with it.
> 
> I am aware of the existence of the agreement, and I am ready to sign it. While I'm working on the patch, we
> can proceed the paperwork in parallel

Form sent off-list.

Message #17 received at 79285 <at> debbugs.gnu.org (full text, mbox):

From: Binbin YE <phantom2501 <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 79285 <at> debbugs.gnu.org
Subject: Re: bug#79285: [Patch] support :font-features in face
Date: Thu, 28 Aug 2025 10:36:45 +0900

[Message part 1 (text/plain, inline)]

Hi Eli,

I found I needed extra handling to support alist merging for the font
features, which took a bit longer than expected.

I am currently wrapping up the code before sending the updated patch. Can
you help me understand what indentation guidelines are?

> > +      if (CONSP (font_features))
> > +          font_put_extra (entity, QCfont_features, font_features);
         ^^^^
> We use indentation offset of 2 columns in C sources.

I tried clang-format with .clang-format configuration in the repository and
it produces indents mixed with tab and space. Like <tab><space><space>

And I spotted the existing code base in xfaces.c sometimes uses all spaces
and sometimes uses mixture of tab and space for indentation.

Is it OK to treat 1 indent level = 2 spaces?

[Message part 2 (text/html, inline)]

Message #20 received at 79285 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Binbin YE <phantom2501 <at> gmail.com>
Cc: 79285 <at> debbugs.gnu.org
Subject: Re: bug#79285: [Patch] support :font-features in face
Date: Thu, 28 Aug 2025 09:04:59 +0300

> From: Binbin YE <phantom2501 <at> gmail.com>
> Date: Thu, 28 Aug 2025 10:36:45 +0900
> Cc: 79285 <at> debbugs.gnu.org
> 
> I found I needed extra handling to support alist merging for the font features, which took a bit longer than
> expected.

Thanks, and take your time.  There's no rush.

> I am currently wrapping up the code before sending the updated patch. Can you help me understand what
> indentation guidelines are?

Sure.

> > > +      if (CONSP (font_features))
> > > +          font_put_extra (entity, QCfont_features, font_features);
>          ^^^^
> > We use indentation offset of 2 columns in C sources.
> 
> I tried clang-format with .clang-format configuration in the repository and it produces indents mixed with tab
> and space. Like <tab><space><space>

We use mixed TABs+SPCes for indentation, so this is okay.

> And I spotted the existing code base in xfaces.c sometimes uses all spaces and sometimes uses mixture of
> tab and space for indentation.

That is in general a mistake, it should use TABs where possible.  But
we don't make whitespace-only changes, we only fix these issues as
part of a larger changeset that touches the relevant places in the
code.  So sometimes such mistakes are left alone for some time.

> Is it OK to treat 1 indent level = 2 spaces?

Yes.

If you edit with Emacs, then the .dir-locals.el file in our repository
should set up the indentation for you, and then marking the region and
typing C-M-\ will reindent the region according to our rules.

Message #23 received at 79285 <at> debbugs.gnu.org (full text, mbox):

From: Binbin YE <phantom2501 <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 79285 <at> debbugs.gnu.org
Subject: Re: bug#79285: [Patch] support :font-features in face
Date: Sun, 31 Aug 2025 11:42:50 +0900

[Message part 1 (text/plain, inline)]

Hi Eli,

Thanks for the review and explanation, I addressed the issues you brought
up and it should be ready for the next round.

> If you edit with Emacs, then the .dir-locals.el file in our repository
> should set up the indentation for you, and then marking the region and
> typing C-M-\ will reindent the region according to our rules.

Somehow my Emacs does not respect the .dir-locals.el file, but I used
clang-format to format the changed lines

> Also, our style is not to use braces when the block has only one
> statement.

I have to say I had a hard time with this :P I should have removed the
braces from the one statement blocks.

> About "enough space" -- how many features could a font have?  And what
> to do if it has more than 32?  GNU coding conventions frown on
> arbitrary limits, and 32 sounds like it's quite arbitrary.  Can we do
> better?

I changed it to dynamically increase the size using existing xpalloc pattern

>  +          Lisp_Object feature_sym = XCAR (feature_spec);
>  +          Lisp_Object feature_val = XCDR (feature_spec);
>  +
>  +          if (SYMBOLP (feature_sym) && FIXNUMP (feature_val))
>
> Are we sure the value cannot be larger than most-positive-fixnum?
> Emacs can handle larger values if needed.

According to the harfbuzz document and the fonts I've seen, mostly the
values are either 0 or 1, occasionally you see 3
for different variations. This should be fine.

> As mentioned above, I think we should be smarter here: instead of
> overwriting the value of :font-features of the target face, we should
> merge the two lists so that it includes the features from both faces.
> Font features from FROM should override the same features from TO, but
> font features which exist in TO but not in FROM should be left in the
> resulting list.

I added support for attribute merging according to the review comment.

> In addition, this will need s NEWS item and a suitable addition to the
> ELisp manual, where the face attributes are described.

That's right, I've added both.

> Last, but not least, to accept a contribution of this size, we will
> need you to sign a copyright-assignment agreement with the FSF.  If
> you are prepared to do that, I will send you the form to fill and the
> instructions to go with it.

I have signed and sent the scanned version and I am currently waiting for
the digital copy from FSF.

[Message part 2 (text/html, inline)]

[0001-support-configuring-font-features-in-face.patch (text/x-patch, attachment)]

Message #26 received at 79285 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Binbin YE <phantom2501 <at> gmail.com>
Cc: 79285 <at> debbugs.gnu.org
Subject: Re: bug#79285: [Patch] support :font-features in face
Date: Sun, 31 Aug 2025 12:08:18 +0300

> From: Binbin YE <phantom2501 <at> gmail.com>
> Date: Sun, 31 Aug 2025 11:42:50 +0900
> Cc: 79285 <at> debbugs.gnu.org
> 
> Thanks for the review and explanation, I addressed the issues you brought up and it should be ready for the
> next round.

Thanks, see further comments below.

> From 0c9dbaff62669cca54c23df053f655dc47f91ab7 Mon Sep 17 00:00:00 2001
> From: Binbin Ye <phantom2501 <at> gmail.com>
> Date: Sun, 31 Aug 2025 11:35:33 +0900
> Subject: [PATCH] support configuring font features in face
> 
> support configuring font features in face when using harfbuzz backend
> using `:font-features'
> 
> * src/dispextern.h (lface_attribute_index): Add
> LFACE_FONT_FEATURES_INDEX
> 
> * src/font.c (font_load_for_lface): Store font featurs in extra info
> 
> * src/hbfont.c (hb_features_from_lisp, hbfont_shape): Convert face
> settings to harfbuzz features and pass to hb_shape_full
> 
> * src/xfaces.c (Finternal_set_lisp_face_attribute)
> (lface_fully_specified_p, check_lface_attrs, init_xfaces)
> (syms_of_xfaces): Add definition of font features attribute and update
> setter function
> (lface_hash, lface_same_font_attributes_p): Update face comparsion and
> hashing for font features
> (merge_face_font_features, merge_face_vectors, merge_face_ref): Support
> font features attribute merging
> 
> * etc/NEWS: Announce face attribute update
> 
> * doc/lispref/display.texi: Add documentation for font features face
> attribute

All the entries in the log message should begin with an upper-case
letter and end with a period.

> +@item :font-features
> +Font features to turn on or off.  Its value is an association list of
> +open type tag and the tag value pairs in the following form:
> +
> +@example
> +((@code{zero} . @var{1}) (@code{ss01} . @var{0}))
> +@end example
> +
> +The attribute is only effective when using @code{harfbuzz} font backend.

Several minor nits here:

  . the official spelling is "HarfBuzz", so please use that
  . 1 and 0 are literal constants, so @var is not appropriate
  . you say "the following form", but actually show an example, not
    the general shape of the form

So my suggestion is to rephrase as follows:

 @item :font-features
 Font features to turn on or off.  The value is an association list of
 open type tag and the tag value pairs in the form
 @w{@code{((@var{tag1} . @var{val1}) (@var{tag2} . @var{val2})@dots{})}}.
 The tag values are in most cases either 1 to turn the feature on or 0
 to turn it off.  For example:

 @example
 ((@code{zero} . 1) (@code{ss01} . 0))
 @end example

 @noindent
 This example turns on the @code{zero} feature and turns off the
 @code{ss01} feature.

 This attribute is only effective when using @code{HarfBuzz} font
 backend (@pxref{Low-Level Font}).

And I think we should have here a @uref pointer to the Web page that
documents the OpenType tags.

> +** New face option ':font-features'.
          ^^^^^^^^^^^
"face attribute"

> +Emacs now supports enable or disable font features when using harfbuzz
                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^            ^^^^^^^^
"enabling or disabling of font features"

Also, "HarfBuzz".

> +font backend with an alist.
                ^^^^^^^^^^^^^
I would say "by specifying this attribute whose value is an alist of
OpenType tags and tag values"

> +'(set-face-attribute 'default nil :font "Font name" :height 85
> +  :font-features '((zero . 1) (ss19 . 1) (cv01 . 0) ))'

This should have "For example:" before it.

> @@ -3482,6 +3482,12 @@ font_load_for_lface (struct frame *f, Lisp_Object *attrs, Lisp_Object spec)
>      {
>        name = Ffont_get (spec, QCuser_spec);
>        if (STRINGP (name)) font_put_extra (entity, QCuser_spec, name);
> +
> +      /* Store font features from face attributes for use during
> +       * shaping.  */

We don't use this style of comments, so please remove the leading "*"
in the second line.

> +	  if (SYMBOLP (feature_sym) && FIXNUMP (feature_val))
> +	    {
> +	      /* Convert symbol to HarfBuzz tag.  */
> +	      const char *feature_name
> +		= SSDATA (SYMBOL_NAME (feature_sym));
> +	      hb_tag_t tag = hb_tag_from_string (feature_name, -1);

We will need to DEF_DLL_FN and LOAD_DLL_FN for hb_tag_from_string, for
this to work on Windows.  But you can leave this to me, if you want.

> +	      (*hb_features)[count].start = 0;
> +	      (*hb_features)[count].end = (unsigned int) -1;

Shouldn't we use HB_FEATURE_GLOBAL_START and HB_FEATURE_GLOBAL_END
here?

> +  /* Cache features array to store enabled font features.  */
> +  static hb_feature_t *hb_features;
> +  static ptrdiff_t hb_features_size;
> +  unsigned int num_features = 0;
> +  if (!NILP (font_features))
> +    num_features = hb_features_from_lisp (font_features, &hb_features,
> +					  &hb_features_size);
> +  hb_bool_t success
> +    = hb_shape_full (hb_font, hb_buffer,
> +		     num_features == 0 ? NULL : hb_features,
> +		     num_features, NULL);

Hmm... not sure I understand what kind of caching is being used here.
AFAIU, hb_features are recomputed anew each time we call hbfont_shape,
so how does this caching help?

> +/* Merges the face font features FROM with the face font features TO,
> +   and returns the merged font features.  */

"Merge" and "return", according to our style of such commentary.

> +	      if (NILP (tail))
> +		{
> +		  /* Construct TAIL to a list and let TO point to the
> +		   * start of the list.  */

Style of comments again.

> +	      if (CONSP (exising_feature))
> +		/* If TO already contains the feature, update the
> +		 * value.  */

And here.

> +	  /* Validate font features list format: Should be a list of
> +	     (feature . value) pairs, without deplicate feature  */

Period missing at the end of this comment.

> +		  if (!CONSP (feature_spec)
> +		      || !SYMBOLP (XCAR (feature_spec))
> +		      || !FIXNUMP (XCDR (feature_spec)))
> +		    signal_error ("Invalid font features format",
> +				  value);
> +		  Lisp_Object feature_sym = XCAR (feature_spec);
> +		  if (!NILP (Fmemq (feature_sym, seen)))
> +		    signal_error ("Duplicate font feature tag",
> +				  feature_sym);
> +		  seen = Fcons (feature_sym, seen);
> +		}
> +	      if (!NILP (tail))
> +		signal_error ("Invalid font features format", value);

Can we show the offending feature and value as part of the error
message here?  IME, it makes debugging problematic Lisp code much
easier.

Thanks again for working on this.

Message #29 received at 79285 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: phantom2501 <at> gmail.com
Cc: 79285 <at> debbugs.gnu.org
Subject: Re: bug#79285: [Patch] support :font-features in face
Date: Sun, 31 Aug 2025 12:25:32 +0300

> Cc: 79285 <at> debbugs.gnu.org
> Date: Sun, 31 Aug 2025 12:08:18 +0300
> From: Eli Zaretskii <eliz <at> gnu.org>
> 
> > From: Binbin YE <phantom2501 <at> gmail.com>
> > Date: Sun, 31 Aug 2025 11:42:50 +0900
> > Cc: 79285 <at> debbugs.gnu.org
> > 
> > Thanks for the review and explanation, I addressed the issues you brought up and it should be ready for the
> > next round.
> 
> Thanks, see further comments below.

One more comment: did you try this code with text that we normally
don't pass to the shaping engine to display, like plain-ASCII text?
Emacs normally calls the shaping engine only for characters whose
slots in composition-function-table are non-nil.  Otherwise we display
the font glyphs directly without shaping.  (This is an optimization:
using the shaping engine slows down redisplay, because it is
implemented by calling to Lisp, which then calls back into C.)  So,
for example, if someone places a face with :font-features attribute on
plain-ASCII text, they will probably not see any effect.

If I'm right, then we will need to make changes in display-engine
functions like composition_compute_stop_pos, composition_reseat_it,
find_composition, and others, to force the text which has such a face
attribute to be handed to HarfBuzz for shaping.  An alternative is to
require that use of this face attribute needs special setup of
composition-function-table, but that is IMO worse because it will slow
down display of the relevant characters even if they don't have the
face with this attribute.

Message #32 received at 79285 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: phantom2501 <at> gmail.com
Cc: 79285 <at> debbugs.gnu.org
Subject: Re: bug#79285: [Patch] support :font-features in face
Date: Sun, 31 Aug 2025 12:35:05 +0300

> Cc: 79285 <at> debbugs.gnu.org
> Date: Sun, 31 Aug 2025 12:25:32 +0300
> From: Eli Zaretskii <eliz <at> gnu.org>
> 
> If I'm right, then we will need to make changes in display-engine
> functions like composition_compute_stop_pos, composition_reseat_it,
> find_composition, and others, to force the text which has such a face
> attribute to be handed to HarfBuzz for shaping.  An alternative is to
> require that use of this face attribute needs special setup of
> composition-function-table, but that is IMO worse because it will slow
> down display of the relevant characters even if they don't have the
> face with this attribute.

Or maybe it will be enough to make the change in
get_glyph_face_and_encoding, as the etc/TODO item suggests:

  instead of calling the 'encode_char' method of the font driver, we
  should invoke the 'shape' method

But this will only work if the effect of the relevant font features is
per-character, i.e. HarfBuzz doesn't need to see the entire word to
shape the characters according to the requested feature(s).  Do you
happen to know if all the features that are relevant for us can be
applied on a character-by-character basis?

Message #35 received at 79285 <at> debbugs.gnu.org (full text, mbox):

From: Binbin YE <phantom2501 <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 79285 <at> debbugs.gnu.org
Subject: Re: bug#79285: [Patch] support :font-features in face
Date: Sun, 31 Aug 2025 19:05:48 +0900

[Message part 1 (text/plain, inline)]

Hi Eli,

Thanks for the comments. Some quick responses:

>  +       if (SYMBOLP (feature_sym) && FIXNUMP (feature_val))
>  +         {
> > +           /* Convert symbol to HarfBuzz tag.  */
>  +           const char *feature_name
>  +             = SSDATA (SYMBOL_NAME (feature_sym));
>  +           hb_tag_t tag = hb_tag_from_string (feature_name, -1);
>
> We will need to DEF_DLL_FN and LOAD_DLL_FN for hb_tag_from_string, for
> this to work on Windows.  But you can leave this to me, if you want.

I don't have access to a Windows machine, it would be great if you can help
me with that.

>  +  /* Cache features array to store enabled font features.  */
>  +  static hb_feature_t *hb_features;
>  +  static ptrdiff_t hb_features_size;
>  +  unsigned int num_features = 0;
>  +  if (!NILP (font_features))
>  +    num_features = hb_features_from_lisp (font_features, &hb_features,
> > +                                       &hb_features_size);
>  +  hb_bool_t success
>  +    = hb_shape_full (hb_font, hb_buffer,
> > +                  num_features == 0 ? NULL : hb_features,
>  +                  num_features, NULL);
>
> Hmm... not sure I understand what kind of caching is being used here.
> AFAIU, hb_features are recomputed anew each time we call hbfont_shape,
> so how does this caching help?

I saw the comment a few lines below for the hb_buffer (static hb_buffer_t
*hb_buffer = NULL;) to have less allocation and
I followed that. It is true that the features are recomputed but the array
can be reused in the next call. If new
allocation is preferred I can change to that.


> One more comment: did you try this code with text that we normally
> don't pass to the shaping engine to display, like plain-ASCII text?
> Emacs normally calls the shaping engine only for characters whose
> slots in composition-function-table are non-nil.  Otherwise we display
> the font glyphs directly without shaping.  (This is an optimization:
> using the shaping engine slows down redisplay, because it is
> implemented by calling to Lisp, which then calls back into C.)  So,
> for example, if someone places a face with :font-features attribute on
> plain-ASCII text, they will probably not see any effect.
>

Yes you are right about it. I was confused why it did not have any effect
initially. And I figured I needed to do
something with composition-function-table. This is my test code

#+begin_src elisp
  (set-char-table-range composition-function-table
    ?0
    '(["." 0 font-shape-gstring]))

  (set-char-table-range composition-function-table
    ?!
    '(["\\(!==\\)" 0 font-shape-gstring]))
#+end_src

#+begin_src elisp
  (set-face-attribute 'default nil :font "JetBrains Mono" :height 100
    :font-features '((zero . 0) (ss19 . 1) (zero . 1)))
#+end_src

#+begin_src elisp
  (set-face-attribute 'default nil :font "JetBrains Mono" :height 100
    :font-features '((zero . 1)))
#+end_src


> If I'm right, then we will need to make changes in display-engine
> functions like composition_compute_stop_pos, composition_reseat_it,
> find_composition, and others, to force the text which has such a face
> attribute to be handed to HarfBuzz for shaping.  An alternative is to
> require that use of this face attribute needs special setup of
> composition-function-table, but that is IMO worse because it will slow
> down display of the relevant characters even if they don't have the
> face with this attribute.

Thanks for pointing that out, I can explore that part of the code. I have
not looked much into composite.c

[Message part 2 (text/html, inline)]

Message #38 received at 79285 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Binbin YE <phantom2501 <at> gmail.com>
Cc: 79285 <at> debbugs.gnu.org
Subject: Re: bug#79285: [Patch] support :font-features in face
Date: Sun, 31 Aug 2025 13:23:55 +0300

> From: Binbin YE <phantom2501 <at> gmail.com>
> Date: Sun, 31 Aug 2025 19:05:48 +0900
> Cc: 79285 <at> debbugs.gnu.org
> 
> >  +       if (SYMBOLP (feature_sym) && FIXNUMP (feature_val))
> >  +         {
> > > +           /* Convert symbol to HarfBuzz tag.  */
> >  +           const char *feature_name
> >  +             = SSDATA (SYMBOL_NAME (feature_sym));
> >  +           hb_tag_t tag = hb_tag_from_string (feature_name, -1);
> >
> > We will need to DEF_DLL_FN and LOAD_DLL_FN for hb_tag_from_string, for
> > this to work on Windows.  But you can leave this to me, if you want.
> 
> I don't have access to a Windows machine, it would be great if you can help me with that.

OK, so I will add that when the patch is installed.

> >  +  /* Cache features array to store enabled font features.  */
> >  +  static hb_feature_t *hb_features;
> >  +  static ptrdiff_t hb_features_size;
> >  +  unsigned int num_features = 0;
> >  +  if (!NILP (font_features))
> >  +    num_features = hb_features_from_lisp (font_features, &hb_features,
> > > +                                       &hb_features_size);
> >  +  hb_bool_t success
> >  +    = hb_shape_full (hb_font, hb_buffer,
> > > +                  num_features == 0 ? NULL : hb_features,
> >  +                  num_features, NULL);
> > 
> > Hmm... not sure I understand what kind of caching is being used here.
> > AFAIU, hb_features are recomputed anew each time we call hbfont_shape,
> > so how does this caching help?
> 
> I saw the comment a few lines below for the hb_buffer (static hb_buffer_t *hb_buffer = NULL;) to have less
> allocation and
> I followed that. It is true that the features are recomputed but the array can be reused in the next call. If new
> allocation is preferred I can change to that.

No, I guess it's okay to leave your code as it is.

> > One more comment: did you try this code with text that we normally
> > don't pass to the shaping engine to display, like plain-ASCII text?
> > Emacs normally calls the shaping engine only for characters whose
> > slots in composition-function-table are non-nil.  Otherwise we display
> > the font glyphs directly without shaping.  (This is an optimization:
> > using the shaping engine slows down redisplay, because it is
> > implemented by calling to Lisp, which then calls back into C.)  So,
> > for example, if someone places a face with :font-features attribute on
> > plain-ASCII text, they will probably not see any effect.
> > 
> 
> Yes you are right about it. I was confused why it did not have any effect initially. And I figured I needed to do
> something with composition-function-table. This is my test code
> 
> #+begin_src elisp
>   (set-char-table-range composition-function-table
>     ?0
>     '(["." 0 font-shape-gstring]))
> 
>   (set-char-table-range composition-function-table
>     ?!
>     '(["\\(!==\\)" 0 font-shape-gstring]))
> #+end_src
> 
> #+begin_src elisp
>   (set-face-attribute 'default nil :font "JetBrains Mono" :height 100
>     :font-features '((zero . 0) (ss19 . 1) (zero . 1)))
> #+end_src
> 
> #+begin_src elisp
>   (set-face-attribute 'default nil :font "JetBrains Mono" :height 100
>     :font-features '((zero . 1)))
> #+end_src
> 
> > If I'm right, then we will need to make changes in display-engine
> > functions like composition_compute_stop_pos, composition_reseat_it,
> > find_composition, and others, to force the text which has such a face
> > attribute to be handed to HarfBuzz for shaping.  An alternative is to
> > require that use of this face attribute needs special setup of
> > composition-function-table, but that is IMO worse because it will slow
> > down display of the relevant characters even if they don't have the
> > face with this attribute.
> 
> Thanks for pointing that out, I can explore that part of the code. I have not looked much into composite.c

See my other mail: perhaps using the shape method in
get_glyph_face_and_encoding will be enough.  If that works, it's a
much simpler change.

Message #41 received at 79285 <at> debbugs.gnu.org (full text, mbox):

From: Binbin YE <phantom2501 <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 79285 <at> debbugs.gnu.org
Subject: Re: bug#79285: [Patch] support :font-features in face
Date: Wed, 3 Sep 2025 08:40:38 +0900

[Message part 1 (text/plain, inline)]

On Sun, Aug 31, 2025 at 18:35 Eli Zaretskii <eliz <at> gnu.org> wrote:

> > Cc: 79285 <at> debbugs.gnu.org
> > Date: Sun, 31 Aug 2025 12:25:32 +0300
> > From: Eli Zaretskii <eliz <at> gnu.org>
> >
> > If I'm right, then we will need to make changes in display-engine
> > functions like composition_compute_stop_pos, composition_reseat_it,
> > find_composition, and others, to force the text which has such a face
> > attribute to be handed to HarfBuzz for shaping.  An alternative is to
> > require that use of this face attribute needs special setup of
> > composition-function-table, but that is IMO worse because it will slow
> > down display of the relevant characters even if they don't have the
> > face with this attribute.
>
> Or maybe it will be enough to make the change in
> get_glyph_face_and_encoding, as the etc/TODO item suggests:
>
>   instead of calling the 'encode_char' method of the font driver, we
>   should invoke the 'shape' method
>
> But this will only work if the effect of the relevant font features is
> per-character, i.e. HarfBuzz doesn't need to see the entire word to
> shape the characters according to the requested feature(s).  Do you
> happen to know if all the features that are relevant for us can be
> applied on a character-by-character basis?


I was diving into a bit more detail of how text is rendered in Emacs. If I
understand correctly the your suggestion is bypassing the composite.el and
constructing the glyph string directly in xdisp.c and pass it to all kinds
of shape function in different backend directly?

I’d need to wrap my head around the display engine first to have more
constructive discussion. A bit more direction from you on what to look for
will be very helpful.

[Message part 2 (text/html, inline)]

Message #44 received at 79285 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Binbin YE <phantom2501 <at> gmail.com>
Cc: 79285 <at> debbugs.gnu.org
Subject: Re: bug#79285: [Patch] support :font-features in face
Date: Wed, 03 Sep 2025 15:10:31 +0300

> From: Binbin YE <phantom2501 <at> gmail.com>
> Date: Wed, 3 Sep 2025 08:40:38 +0900
> Cc: 79285 <at> debbugs.gnu.org
> 
>  Or maybe it will be enough to make the change in
>  get_glyph_face_and_encoding, as the etc/TODO item suggests:
> 
>    instead of calling the 'encode_char' method of the font driver, we
>    should invoke the 'shape' method
> 
>  But this will only work if the effect of the relevant font features is
>  per-character, i.e. HarfBuzz doesn't need to see the entire word to
>  shape the characters according to the requested feature(s).  Do you
>  happen to know if all the features that are relevant for us can be
>  applied on a character-by-character basis?
> 
> I was diving into a bit more detail of how text is rendered in Emacs. If I understand correctly the your
> suggestion is bypassing the composite.el and constructing the glyph string directly in xdisp.c and pass it to
> all kinds of shape function in different backend directly?

That's the idea, yes.  It would mean to have a function in hbfont.c
that is the subset of hbfont_shape, and which accepts a single
character (not a Lisp string) and a font, and then constructs the
hb_buffer and submits that to hb_shape_full.

But please test if this should give good results by simulating it, as
follows:

 . make composition-function-table whose cells for several characters
   match only that one character, and see how a string of such
   characters is rendered using a font with relevant OpenType features
 . then compare that with rendering when composition-function-table
   has the same rule in the cell of each of those characters, matching
   any sequence of these characters (as in "[abcdefg]+")

If applying stylistic sets by rendering text one character at a time
produces different results from rendering them all as a single string,
then this idea is not workable, and we will need to use the (slower
and more complex) composite.c machinery instead.

If the idea does work, then presumably a change in
get_glyph_face_and_encoding for characters that have this special
face attribute will be all that's needed, perhaps together with some
flag in the 'struct it' to make that faster.  Details later, when we
know whether the idea works or not.

> I’d need to wrap my head around the display engine first to have more constructive discussion. A bit more
> direction from you on what to look for will be very helpful. 

See above.  If the idea of shaping one character at a time doesn't
work, I will then tell you how to study the code which handles
character compositions.

Thanks.

Message #47 received at 79285 <at> debbugs.gnu.org (full text, mbox):

From: Binbin YE <phantom2501 <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 79285 <at> debbugs.gnu.org
Subject: Re: bug#79285: [Patch] support :font-features in face
Date: Tue, 9 Sep 2025 00:26:11 +0900

[Message part 1 (text/plain, inline)]

On Wed, Sep 3, 2025 at 9:10 PM Eli Zaretskii <eliz <at> gnu.org> wrote:

>
> That's the idea, yes.  It would mean to have a function in hbfont.c
> that is the subset of hbfont_shape, and which accepts a single
> character (not a Lisp string) and a font, and then constructs the
> hb_buffer and submits that to hb_shape_full.
>
> But please test if this should give good results by simulating it, as
> follows:
>
>  . make composition-function-table whose cells for several characters
>    match only that one character, and see how a string of such
>    characters is rendered using a font with relevant OpenType features
>  . then compare that with rendering when composition-function-table
>    has the same rule in the cell of each of those characters, matching
>    any sequence of these characters (as in "[abcdefg]+")
>
> If applying stylistic sets by rendering text one character at a time
> produces different results from rendering them all as a single string,
> then this idea is not workable, and we will need to use the (slower
> and more complex) composite.c machinery instead.
>
> If the idea does work, then presumably a change in
> get_glyph_face_and_encoding for characters that have this special
> face attribute will be all that's needed, perhaps together with some
> flag in the 'struct it' to make that faster.  Details later, when we
> know whether the idea works or not.
>
>
I compared the result between

#+begin_src elisp
  (set-char-table-range composition-function-table
    ?!
    '(["\\(!==\\)" 0 font-shape-gstring]))
#+end_src

and

#+begin_src elisp
  (set-char-table-range composition-function-table
    ?!
    '(["\\(!\\)" 0 font-shape-gstring]))

  (set-char-table-range composition-function-table
    ?=
    '(["\\(=\\)" 0 font-shape-gstring]))
#+end_src

They are different, only matching the sequence produces the desired result
for multi-character ligatures.


I read the hbfont.c code and the hb buffer is cleared every time handling
the shaping. I think it makes sense that it
should not store the state of the Emacs buffer in hb buffer, and HarfBuzz
needs to know the whole sequence to shape
according to their document.

I did some research on how other programs make use of HarfBuzz. They
typically put an entire paragraph or put a line
into the shaping function. It is quite an interesting way for Emacs to
detect a sequence first and specifically shape
that sequence using HarfBuzz. It might be historical reason but it seems a
lot more work needs to be done in composite.c
or we need to figure out something better.

[Message part 2 (text/html, inline)]

Message #50 received at 79285 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Binbin YE <phantom2501 <at> gmail.com>
Cc: 79285 <at> debbugs.gnu.org
Subject: Re: bug#79285: [Patch] support :font-features in face
Date: Mon, 08 Sep 2025 19:33:53 +0300

> From: Binbin YE <phantom2501 <at> gmail.com>
> Date: Tue, 9 Sep 2025 00:26:11 +0900
> Cc: 79285 <at> debbugs.gnu.org
> 
> On Wed, Sep 3, 2025 at 9:10 PM Eli Zaretskii <eliz <at> gnu.org> wrote:
> 
>  That's the idea, yes.  It would mean to have a function in hbfont.c
>  that is the subset of hbfont_shape, and which accepts a single
>  character (not a Lisp string) and a font, and then constructs the
>  hb_buffer and submits that to hb_shape_full.
> 
>  But please test if this should give good results by simulating it, as
>  follows:
> 
>   . make composition-function-table whose cells for several characters
>     match only that one character, and see how a string of such
>     characters is rendered using a font with relevant OpenType features
>   . then compare that with rendering when composition-function-table
>     has the same rule in the cell of each of those characters, matching
>     any sequence of these characters (as in "[abcdefg]+")
> 
>  If applying stylistic sets by rendering text one character at a time
>  produces different results from rendering them all as a single string,
>  then this idea is not workable, and we will need to use the (slower
>  and more complex) composite.c machinery instead.
> 
>  If the idea does work, then presumably a change in
>  get_glyph_face_and_encoding for characters that have this special
>  face attribute will be all that's needed, perhaps together with some
>  flag in the 'struct it' to make that faster.  Details later, when we
>  know whether the idea works or not.
> 
> I compared the result between
> 
> #+begin_src elisp
>   (set-char-table-range composition-function-table
>     ?!
>     '(["\\(!==\\)" 0 font-shape-gstring]))
> #+end_src
> 
> and
> 
> #+begin_src elisp
>   (set-char-table-range composition-function-table
>     ?!
>     '(["\\(!\\)" 0 font-shape-gstring]))
> 
>   (set-char-table-range composition-function-table
>     ?=
>     '(["\\(=\\)" 0 font-shape-gstring]))
> #+end_src
> 
> They are different, only matching the sequence produces the desired result for multi-character ligatures.
> 
> I read the hbfont.c code and the hb buffer is cleared every time
> handling the shaping. I think it makes sense that it should not
> store the state of the Emacs buffer in hb buffer, and HarfBuzz needs
> to know the whole sequence to shape according to their document.

OK, thanks.  This is what I suspected.  Unfortunately, it means we
need to use the full machinery of character composition to support
this face attribute on arbitrary text: the entire chunk of text which
has this attribute, or at least its individual wortds, will need to be
passed through HarfBuzz en-masse.  It also means using this will
probably slow down redisplay of the relevant text parts, unless we
find a way of avoiding some of the slow code parts.

Let me think about the best way of doing this.  Meanwhile, I invite
you to read the large commentary at the beginning of xdisp.c, which
mentions character composition, and also take at least a cursory look
at the "automatic compositions" parts of composite.c, which is where
most of the code that deals with character compositions lives.

> I did some research on how other programs make use of HarfBuzz. They
> typically put an entire paragraph or put a line into the shaping
> function. It is quite an interesting way for Emacs to detect a
> sequence first and specifically shape that sequence using
> HarfBuzz. It might be historical reason but it seems a lot more work
> needs to be done in composite.c or we need to figure out something
> better.

It's not just a historical accident.  The Emacs display engine is
special, in that it examines text one character (or one grapheme
cluster, in case of compositions) at a time, and makes all the layout
decisions on the fly.  So passing large chunks of text to a shaping
engine, like other programs do, is out of the question, as long as we
keep this basic design of the Emacs display.

The reason why Emacs tries to avoid using the shaper, unless
composition-function-table tells us we must, is that the
implementation of shaping and composition in Emacs is exposed to Lisp
and uses Lisp code for some of its workings, and thus is slow.  Emacs
is unique in this: no other program allows the user to affect
character composition and shaping by a simple change of a
character-indexed table, while the session keeps running.  This gives
Lisp programs and users an unprecedented freedom of affecting how
stuff is displayed, but it comes at a price.

Message #53 received at 79285 <at> debbugs.gnu.org (full text, mbox):

From: Binbin YE <phantom2501 <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 79285 <at> debbugs.gnu.org
Subject: Re: bug#79285: [Patch] support :font-features in face
Date: Wed, 10 Sep 2025 00:51:42 +0900

[Message part 1 (text/plain, inline)]

On Tue, Sep 9, 2025 at 1:34 Eli Zaretskii <eliz <at> gnu.org> wrote:

>
>  It also means using this will
> probably slow down redisplay of the relevant text parts, unless we
> find a way of avoiding some of the slow code parts.
>
> Let me think about the best way of doing this.  Meanwhile, I invite
> you to read the large commentary at the beginning of xdisp.c, which
> mentions character composition, and also take at least a cursory look
> at the "automatic compositions" parts of composite.c, which is where
> most of the code that deals with character compositions lives.


Thanks for pointing out the direction, I might need to digest all that with
extra time. I’ll see if I can also help improving the commentary if there’s
a chance.



> The reason why Emacs tries to avoid using the shaper, unless
> composition-function-table tells us we must, is that the
> implementation of shaping and composition in Emacs is exposed to Lisp
> and uses Lisp code for some of its workings, and thus is slow.  Emacs
> is unique in this: no other program allows the user to affect
> character composition and shaping by a simple change of a
> character-indexed table, while the session keeps running.  This gives
> Lisp programs and users an unprecedented freedom of affecting how
> stuff is displayed, but it comes at a price.


It is extremely powerful design, maybe there is a chance that the shaping
engine can serve as additional source of information passing to the
composition — so that we can take advantage of both. I’m guessing, I
probably will realize how unpractical this idea is, later.

[Message part 2 (text/html, inline)]

Message #56 received at 79285 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Binbin YE <phantom2501 <at> gmail.com>
Cc: 79285 <at> debbugs.gnu.org
Subject: Re: bug#79285: [Patch] support :font-features in face
Date: Tue, 09 Sep 2025 19:20:33 +0300

> From: Binbin YE <phantom2501 <at> gmail.com>
> Date: Wed, 10 Sep 2025 00:51:42 +0900
> Cc: 79285 <at> debbugs.gnu.org
> 
>  The reason why Emacs tries to avoid using the shaper, unless
>  composition-function-table tells us we must, is that the
>  implementation of shaping and composition in Emacs is exposed to Lisp
>  and uses Lisp code for some of its workings, and thus is slow.  Emacs
>  is unique in this: no other program allows the user to affect
>  character composition and shaping by a simple change of a
>  character-indexed table, while the session keeps running.  This gives
>  Lisp programs and users an unprecedented freedom of affecting how
>  stuff is displayed, but it comes at a price.
> 
> It is extremely powerful design, maybe there is a chance that the shaping engine can serve as additional
> source of information passing to the composition — so that we can take advantage of both. I’m guessing, I
> probably will realize how unpractical this idea is, later. 

It should be possible to bypass the current flow of processing.  But
it will not be easy, and will require additional non-trivial effort.

Character composition currently works by passing to the shaper a chunk
of buffer text (one or more characters), receiving a series of font
glyphs for those characters from the shaper, and then laying out these
glyphs one by one.  The workhorse of the latter step is
next_element_from_composition; as long as we use that in the display
engine, restructuring the code that fills up 'struct composition_it'
which stores the information about the composed characters will
basically need to duplicate a lot of what we already do anyway.

So I think for starters we should reuse as much code of character
composition as possible, and leave optimizations (if needed) for
later.  The minimum that is needed is to replace the code which uses
composition rules stored in composition-function-table to determine
the sequence of characters to be passed to HarfBuzz by something
similar which determines that sequence by looking for stretches of
text that have the face property with this new attribute.  I'd suggest
to do this minimum first, and see how well/fast that works.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.