Subject: Re: bug#79285: [Patch] support :font-features in face

From: Eli Zaretskii <[email-masked]>
Date: Sun, 31 Aug 2025 12:08:18 +0300

> From: Binbin YE <[email-masked]>
> Date: Sun, 31 Aug 2025 11:42:50 +0900
> Cc: [email-masked]
> 
> 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 <[email-masked]>
> 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.