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

[Kévin Le Gouguec]

Revised patch attached; intended as "good for master".


Answers to open discussion points:

> 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.

diff-mode.el (re. smerge-mode.el) can indeed be fooled into thinking
diff-changed (re. smerge-refined-changed) differs-from-default, if one
"shoots their own foot", for example, setting…

* :extend t:         fixed by this patch                    ✔️
* :stipple nil:      foot blown with or without the patch   🤷
* :inherit 'default: foot blown with or without the patch   🤷

Problem with :stipple nil and :inherit 'default explained in [1].
indicate-copied-region will become affected if the current patch goes
in.

replace.el:occur-1 befuddled me for a moment[2], but the tl;dr is that
it will be none the worse for wear.

> > >> (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)
> 
> I'm sorry, you will have to look up the discussion that led to the
> development of the :extend attribute; I cannot afford searching for
> it.  The differences between TTY and GUI frames were one of the main
> reasons why we introduced this attribute.

Playing around with more terminals did not yield more insights[3], so it
seems I'll need to dig into the archives, indeed.

Currently squinting at emacs-devel:<83r2fbg5bq.fsf@gnu.org>, which
singles out NS versus X, Windows & TTYs; could this be what you had in
mind?

> Alternatively, we could add a user option to make the swap
> unconditional, because maybe some users would prefer that to splitting
> hair in this case.  Then we could stop worrying about all those fine
> differences.

Should I cook up a user option to unconditionally do the swap before we
apply the attached?  Otherwise we may disgruntle trunk users who
actually liked the behaviour I reported in the OP (swapping regardless
of whether region stands out).


                                   ⁂
                               footnotes
                                   ⁂

[1]

  (face-differs-from-default-p 'default)
  ↦ :stipple
  (display-supports-face-attributes-p '(:stipple nil))
  ↦ t

So a face that explicitly inherits from 'default, or sets :stipple to
nil (rather than 'unspecified) differs-from-default-p.

This seems inappropriate, based on display-supports-face-attributes-p's
docstring:

> The definition of ‘supported’ is somewhat heuristic, but basically means
> that a face containing all the attributes in ATTRIBUTES, when merged
> with the default face for display, can be represented in a way that’s
> 
>  (1) different in appearance from the default face, and
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>  (2) ‘close in spirit’ to what the attributes specify, if not exact.

I can debug gui_supports_face_attributes_p if we agree that there is
something to investigate here?

[2]

  (if (face-differs-from-default-p list-matching-lines-prefix-face)
      list-matching-lines-prefix-face)

> If this face will display the same as the default face, the prefix
> column will not be highlighted specially.
— C-h v list-matching-lines-prefix-face

Why bother with this `if' then?  Isn't this gratuituous complexity?
Just passing the face regardless would have the same effect (the prefix
column will look like it has the default face).

[3]

>                                            Maybe what I remember
> happens only on some terminals.

AFAICT, :extend {nil,t} successfully make the background {stop at,extend
past} EOL on these terminals:

* Linux TTYs (openSUSE Tumbleweed: Linux 6.1.7-1-default x86_64,
              Debian 11:           Linux 5.10.0-19-amd64 x86_64)
* VTE-based pseudo terminals (terminator, xfce4-terminal)
* Plasma's konsole
* xterm

>                                  Or maybe I'm misremembering and it
> was because of underline and not the color.

* Linux TTYs don't support :underline AFAICT,
* all pseudo terminals listed above do, and :extend nil/t both do TRT.

>                                              But there is definitely a
> difference.

ACK.  Based on vc-region-history alone it feels like
tty_supports_face_attributes_p just got left out of the :extend party,
but I'll see what turns up in the lists.

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