bug#60841: 30.0.50; kill-ring-save pauses despite region being highlighted

[Kévin Le Gouguec]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Kévin Le Gouguec <kevin.legouguec@gmail.com>
>> 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.

0001-Avoid-spurious-pause-in-kill-ring-save.patch
Description: Text Data