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

Previous Next

Full log

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)]

Previous Next

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