bug#59347: 29.0.50; `:family` face setting ignored

[Eli Zaretskii <eliz@gnu.org>]

> From: Gregory Heytings <gregory@heytings.org>
> cc: monnier@iro.umontreal.ca, Po Lu <luangruo@yahoo.com>, 
>     59347@debbugs.gnu.org
> 
> 
> Amazing.  I thought this exhausting discussion was over, but no, Po Lu 
> came and "improved" the code that was agreed upon only a couple of hours 
> after it was pushed, disregarding this entire discussion, the docstring of 
> the variable that the commit introduced, the commit message, and without 
> asking any questions.  How can that be right, how can that be acceptable?

This discussion went overboard, IMO.  I indeed think that Po Lu should
have discussed his changes before doing them, but your reaction, in
particular the revert, is also overreaction.

> The name of the variable was changed, and the docstring was "improved", 
> and became completely wrong.  It is simply untrue that this is a list of 
> attributes that Emacs will "ignore when realizing a face", or in fact that 
> this changes anything fundamental in the way Emacs realizes faces.

If this is the doc string you saw, you were not looking at an
up-to-date tree.  I renamed the variable and rewrote the doc string
soon after Po Lu made his changes; the word "ignore" (which I agree is
inaccurate) is no longer there.  If you still have comments on the
current doc string, please speak up.

> Here is again, in every possible detail, but this time in a single post, 
> the analysis of this subtle bug, and the rationale of the patch that was 
> agreed upon.  I explain this bug with an concrete example, with the 
> 'variable-pitch' face and a font with a 'medium' weight.  That example is 
> only meant to illustrate the bug: the same reasoning applies for other 
> faces and other font attributes (slant or width).

Thanks.  This is a good description, and it is good to have it in the
bug discussion, for posterity.

> From: Po Lu <luangruo@yahoo.com>
> Cc: Gregory Heytings <gregory@heytings.org>
> 
>   - unsetting the "extra" attribute is not safe on the Haiku port.

If this is so, why not disable that only for Haiku?

>   - the bitmask variable is a real nusiance for anyone trying to
>     debug Emacs or change the layout of the font attribute index
>     enumerator.

"Nuisance" is an exaggeration.  But I agree that using more
descriptive values is more convenient, if and when someone needs to
change the default value.  And I have a proposal for how to do this
without sacrificing performance; read on.

> Just because a bug has been closed does NOT mean the change in it is no
> longer subject to scrutiny.

But, after such a long and painful discussion, it would have been
prudent to talk first and cut the metal later.  And, btw, your change
had two copy/paste typos, which would have probably be avoided if you
posted the patch first.

> all that code takes somewhere between 2 to 4 microseconds to complete
> for me, in a build with optimizations enabled.  I don't know how many

People are reportedly running Emacs sessions with several thousands of
faces, in which case 2 to 4 usec per face could add up to a
significant number.  So it cannot do any harm to try to make the
"usual" case faster.

> From: Gregory Heytings <gregory@heytings.org>
> To: Po Lu <luangruo@yahoo.com>
> cc: emacs-devel@gnu.org
>
> >> Of course I did.  That you did not read it is another thing.
> >
> > You did not.  The only real technical argument you put forth was some 
> > nonsense about FOR_EACH_TAIL being slow.
> >
> 
> "Nonsense", again?  Thank you!

Yes, let's please avoid nasty unfriendly language.

> From: Po Lu <luangruo@yahoo.com>
> To: Gregory Heytings <gregory@heytings.org>
> Cc: Eli Zaretskii <eliz@gnu.org>,  monnier@iro.umontreal.ca,
>   59347@debbugs.gnu.org
> 
> The fundamental problem I have with the doc string is
> that it did not say what it does, or what it is supposed to debug, in a
> manner users can understand.

Does the current doc string have any such problems you can spot?

Anyway, here's my proposal:

 . we change the default value of the variable to be t, and document
   that this stands for (:weight :width :slant)
 . we change the code to reset only those 3 attributes when the
   value is t, and to reset nothing when the value is nil
 . the (slower) code which loops over the list will only run if the
   value of the variable is neither nil nor t
 . we avoid resetting the :extra attribute on Haiku

Is this okay with you both?  I volunteer to make these changes if you
agree.

Thanks.