[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#59347: 29.0.50; `:family` face setting ignored
From: |
Eli Zaretskii |
Subject: |
bug#59347: 29.0.50; `:family` face setting ignored |
Date: |
Mon, 12 Dec 2022 17:07:07 +0200 |
> 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.
- bug#59347: 29.0.50; `:family` face setting ignored, (continued)
- bug#59347: 29.0.50; `:family` face setting ignored, Gregory Heytings, 2022/12/11
- bug#59347: 29.0.50; `:family` face setting ignored, Po Lu, 2022/12/11
- bug#59347: 29.0.50; `:family` face setting ignored, Gregory Heytings, 2022/12/12
- bug#59347: 29.0.50; `:family` face setting ignored, Po Lu, 2022/12/12
- bug#59347: 29.0.50; `:family` face setting ignored, Gregory Heytings, 2022/12/12
- bug#59347: 29.0.50; `:family` face setting ignored, Po Lu, 2022/12/12
- bug#59347: 29.0.50; `:family` face setting ignored, Gregory Heytings, 2022/12/12
- bug#59347: 29.0.50; `:family` face setting ignored, Po Lu, 2022/12/12
- bug#59347: 29.0.50; `:family` face setting ignored, Stefan Monnier, 2022/12/12
- bug#59347: 29.0.50; `:family` face setting ignored, Eli Zaretskii, 2022/12/12
- bug#59347: 29.0.50; `:family` face setting ignored,
Eli Zaretskii <=
- bug#59347: 29.0.50; `:family` face setting ignored, Gregory Heytings, 2022/12/12
- bug#59347: 29.0.50; `:family` face setting ignored, Eli Zaretskii, 2022/12/12
- bug#59347: 29.0.50; `:family` face setting ignored, Gregory Heytings, 2022/12/12
- bug#59347: 29.0.50; `:family` face setting ignored, Eli Zaretskii, 2022/12/13
- bug#59347: 29.0.50; `:family` face setting ignored, Po Lu, 2022/12/12
- bug#59347: 29.0.50; `:family` face setting ignored, Eli Zaretskii, 2022/12/13
bug#59347: 29.0.50; `:family` face setting ignored, Eli Zaretskii, 2022/12/04