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

From: Eli Zaretskii <[email-masked]>
Date: Sun, 31 Aug 2025 13:23:55 +0300

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