bug#69597: 29.2; ERC 5.6-git: Add a new customizable variable controlling how Erc displays spoilers

[J.P.]

Fadi Moukayed <smfadi@gmail.com> writes:

>> So, basically, I wonder if we shouldn't (instead?) just redefine the
>> face's role to be one of indicating _revealed_ text, which is currently
>> the job of `erc-inverse-face' (`erc-spoilers-face' could just :inherit
>> it). And FWIW, a change like this would be justifiable without much fuss
>> if we deemed it a bug fix, since this feature hasn't made it into any
>> releases yet.
>
> After pondering this issue for a day or two, I've come around to agree
> with this assessment. My gut feeling is that the KISS (as in "Keep It
> Simple") solution would be to go the :inherit route and to reveal any
> spoilers on user interaction.
> Aside from changing the definition of the face, this would entail a
> small modification/simplification in `erc-controls-propertize', where
> the already-existing `put-text-property' calls is changed to set
> 'mouse-face to 'erc-spoiler-face. An illustrative patch doing this
> change is included.

Makes sense to me.

> **However**, this is where I've seemingly hit another bug in Erc.
> While setting 'mouse-face should - in theory - work, and cause the
> propertized text to get revealed on mouse hover, in practice, it does
> not. Some part of Erc's formatting machinery seems to strip away the
> 'mouse-face property off the text, so it does seem like the
> `put-text-property' call in `erc-controls-propertize' has never really
> worked for quite some time.

Your suspicions are likely spot on. Sad as it is, I don't think this
"feature" has _ever_ worked, especially if the unit test is anything to
go by. Basically, if I remove a lazy contrivance from the test
environment so it better reflects reality, the thing fails with exactly
the behavior you describe. FWIW, I've attached an improved version that
no longer suffers from this problem.

> Or at least, this is what I observe on my
> own Emacs setup – would be helpful if others can confirm this
> behavior.
>
> Unfortunately, I haven't managed to find exactly where there the
> 'mouse-face property is removed, which is why I've termed the attached
> patch "illustrative", aka. it does not quite resolve the issue fully.
> Some help here would be appreciated.

Ugh, sorry to have put you through all that. I've gone ahead and
attached a preliminary proposal for addressing the situation. If it
seems rather roundabout, it definitely is. Basically, we can't really
responsibly move `erc-controls-highlight' after `erc-button-add-buttons'
in `erc-insert-modify-hook' without causing general mayhem. So, absent a
smarter way to reconcile various interests (many of them legacy)
contending for the same real estate (e.g., `mouse-face'), we'll likely
have little choice but to settle for something in the vicinity of where
I've ended up (although I'd love to be wrong about that).

>
> Cheers,
> FM.
[...]
>>
>
> From 06e008d1de8a85c9e6b9a5a83f5ec5aefeb446c3 Mon Sep 17 00:00:00 2001
> From: "F. Moukayed" <smfadi+emacs@gmail.com>
> Date: Fri, 8 Mar 2024 08:39:03 +0000
> Subject: [PATCH] * lisp/erc/erc-goodies.el: redefine & rework
>  `erc-spoilers-face' to indicate revealed text
>
> ---
>  lisp/erc/erc-goodies.el | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/lisp/erc/erc-goodies.el b/lisp/erc/erc-goodies.el
> index 7e30b10..12f7f3c 100644
> --- a/lisp/erc/erc-goodies.el
> +++ b/lisp/erc/erc-goodies.el
> @@ -665,9 +665,7 @@ The value `erc-interpret-controls-p' must also be t for 
> this to work."
>    "ERC inverse face."
>    :group 'erc-faces)
>  
> -(defface erc-spoiler-face
> -  '((((background light)) :foreground "DimGray" :background "DimGray")
> -    (((background dark)) :foreground "LightGray" :background "LightGray"))
> +(defface erc-spoiler-face '((t :inherit (erc-inverse-face)))
>    "ERC spoiler face."
>    :group 'erc-faces)
>  
> @@ -968,13 +966,10 @@ Also see `erc-interpret-controls-p' and 
> `erc-interpret-mirc-color'."
>    "Prepend properties from IRC control characters between FROM and TO.
>  If optional argument STR is provided, apply to STR, otherwise prepend 
> properties
>  to a region in the current buffer."
> -  (if (and fg bg (equal fg bg))
> -      (progn
> -        (setq fg 'erc-spoiler-face
> -              bg nil)
> -        (put-text-property from to 'mouse-face 'erc-inverse-face str))
> -    (when fg (setq fg (erc-get-fg-color-face fg)))
> -    (when bg (setq bg (erc-get-bg-color-face bg))))
> +  (when (and fg bg (equal fg bg))
> +    (put-text-property from to 'mouse-face 'erc-spoiler-face str))

Here's how I envision this working. So, in addition to your
`put-text-property' above, you'd have something like this:

  (erc--reserve-important-text-props from to
                                     '( mouse-face erc-spoiler-face
                                        cursor-face erc-spoiler-face))

If you want, you can add `cursor-face' as well, so people without mice
can optionally use the feature:

  (add-text-properties from to '( mouse-face erc-spoiler-face
                                  cursor-face erc-spoiler-face)))

Please take a look at and (if possible) try the changes when you have a
chance. Happy to explain whatever doesn't make sense. And, obviously, if
you have any improvements or a superior solution, please don't hesitate.

Many thanks, as always.

> +  (when fg (setq fg (erc-get-fg-color-face fg)))
> +  (when bg (setq bg (erc-get-bg-color-face bg)))
>    (font-lock-prepend-text-property
>     from
>     to

0001-5.6-Fix-misleading-test-in-erc-goodies.patch
Description: Text Data

0002-5.6-Make-important-text-props-more-resilient-in-ERC.patch
Description: Text Data