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: Sat, 28 Jan 2023 18:45:07 +0100
>> 
>> 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.
>
> I wouldn't be bothered by :stipple whose value is nil.  Why would
> someone do such a thing, except when the face is meant to be merged
> with other faces (which are expected to have non-nil :stipple
> attributes)?

The only place where this "matters" is in replace.el, AFAICT:

1. Customize list-matching-lines-prefix-face to 'default,

2. (face-differs-from-default-p list-matching-lines-prefix-face)
   ↦ :stipple

3. Luckily this does not impact replace-el:occur-1, since this check…

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

   … is redundant AFAIU: the goal is to _not_ apply
   list-matching-lines-prefix-face if it is close to 'default, and
   instead… leave the default face?  So why bother checking?

CC'ing Juri, who installed this check in bug#14017, for comment in case
I missed something.

(Hi Juri 👋  For context, the current report is about refining
indicate-copied-region to avoid spuriously considering the region as
"unhighlighted" and triggering the point-mark swap; see attached
patches.  I believe this aspect of the report might also be of interest
to you, given your participation in bug#42865)

>> > 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).
>
> I guess adding such an option would be a good precaution, indeed.

OK, two tentative patches attached, because I don't know which makes for
the better UX:

    ▼ Copy Region Inhibit Blink:
    Choice:
    (*) region-stands-out-p
            Whether the region can be distinguished visually. More
    ( ) always
            Always inhibit: never blink point and mark.
    ( ) ignore
            Never inhibit: always blink point and mark.
    ( ) Other predicate function.: ignore
        State : STANDARD.
       Whether we should refrain from blinking the cursor after a copy. ▼
       When this condition holds, ‘kill-ring-save’ will not blink the
       cursor between point and mark to denote the copied region.
    Groups: Killing

                                 ⁂ VS ⁂

    ▼ Copy Region Blink Predicate:
    Choice:
    (*) region-indistinguishable-p
            Whether the current region is not denoted visually. ▼
        This holds when the region is inactive, or when the ‘region’ face
        cannot be distinguished from the ‘default’ face.
    ( ) always
            Always blink point and mark.
    ( ) ignore
            Never blink point and mark.
    ( ) Other predicate function.: ignore
        State : STANDARD.
       Whether the cursor must be blinked after a copy. ▼
       When this condition holds, and the copied region fits in the
       current window, ‘kill-ring-save’ will blink the cursor between
       point and mark for ‘copy-region-blink-delay’ seconds.
    Groups: Killing


inhibit-vs-predicate.diff shows a comparison of both patches once
applied.

Boldly marked the new options with "---" in NEWS, because
copy-region-blink-delay is not documented in the manual either, but let
me know if that should be remedied.


Thank you for your time.

copy-region-blink-predicate.patch
Description: Text Data

copy-region-inhibit-blink.patch
Description: Text Data

inhibit-vs-predicate.diff
Description: Text Data