Package: emacs;
Reported by: Binbin YE <phantom2501 <at> gmail.com>
Date: Thu, 21 Aug 2025 15:16:02 UTC
Severity: normal
Tags: patch
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.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.