Eli Zaretskii writes: >> From: Kévin Le Gouguec >> Cc: gregory@heytings.org, 60841@debbugs.gnu.org >> Date: Wed, 18 Jan 2023 23:16:34 +0100 >> >> > * :extend nil for both: they display differently (region will not be >> > extended, default will be), >> > * :extend t for both: they display the same, >> > * default has nil, region has t: they display the same, >> > * default has t, region has nil: they display differently. > > The default face is always extended, so it should be treated as > implicitly having the :extend attribute set. > > I think face-differs-from-default-p should be fixed to either ignore > the :extend attribute like it ignores :inherit (since it could be > argued that :extend doesn't really control how the face affects > characters on display), or it should treat the default face as having > the :extend attribute with the value t. I have been slowly converging toward "ignore :extend" being TRT. I cannot find a situation where :extend matters. AFAICT "does this face stand out vs 'default?" always comes down to whether the face's :underline or :background _also_ differs, so :extend seems redundant. Breakdown in footnote[1]. (Have been going back and forth over this; apologies if I've missed something and that conclusion is wrong) > Can you see if any of these changes cause any trouble in our own use > of face-differs-from-default-p? AFAICT, it will actually fix a subtle > problem in diff-mode.el: if diff-changed face doesn't define > non-default colors, it will be still taken as different from > 'default', which I think is contrary to what diff-mode expects. Will do; that was on my checklist[2] whichever solution we eventually pick. > If we make the above change in face-differs-from-default-p, would > using it for the purpose of this issue fix the problem? I think so. Scribbled the attached patch; will report once I've tested it more thoroughly against (a) the :inverse-video use-case from my OP, (b) the "region with unspecified :background" use-case from emacs-devel, (c) other in-tree users of face-differs-from-default-p. In the meantime, let me know if it looks good in principle; there are still details I'd like to tweak FWIW. E.g. * face-differs-from-default-p probably deserves a comment explaining what's going on wrt these attributes, * that seq-difference call makes me inexplicably nervous; I thought I vaguely remembered debates on whether preloaded libraries {c,sh}ould use seq.el functions, but then I see that "emacs-lisp/seq" is already in preloaded-file-list, and e.g. rmc.el calls some of its functions. Am I misremembering? >> (Hm, and against my better judgement I went ahead and compared >> gui_supports_face_attributes_p vs tty_supports_face_attributes_p, and I >> see that they handle :extend differently and *mashes C-c C-c with >> forehead before fingers can type another wall of text*) > > TTY frames always extend the color, that's the reason for the > difference. (Not sure I get your meaning here; on the Linux TTY I have on hand, (set-face-extend 'region nil) does disable color extension) > But does this affect my proposal above? Not AFAICT. Good thing I hit message-send-and-exit in time 🤕 Bottomline: let me know if the attached seems to go in the right direction; meanwhile, will check that it does TRT for other in-tree users of face-differs-from-default-p. Thanks for your patience. [1] Face under test has… | Does text with that face stand out? :background = default :extend nil | no :background = default :extend t | no :background ≠ default :extend nil | yes :background ≠ default :extend t | yes ⇒ face-differs-from-default-p can just check :background ≠ default, which it already does via display-supports-face-attributes-p. Face under test has… | Does text with that face stand out? :underline = default :extend nil | no :underline = default :extend t | no :underline ≠ default :extend nil | yes :underline ≠ default :extend t | yes ⇒ face-differs-from-default-p can just check :underline ≠ default, which it already does via display-supports-face-attributes-p. NB: as far as I tested, this is is true for :underline's boolean values as well as for its other forms. [2] Another thing on that checklist would be adding ERT testcases for face-differs-from-default-p, but I see the specter of --batch beaming with mischievous glee at the thought of me invoking display-related functions with a "headless" Emacs. Is it…? - yep, yep, it *is* putting popcorn in the oven. 😮‍💨 Maybe this time I can convince it to just eat the popcorn and not throw it at me.