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

Previous Next

Full log

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.

Previous Next

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