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

[Fadi Moukayed]

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

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

Cheers,
FM.



Am Do., 7. März 2024 um 21:29 Uhr schrieb J.P. <jp@neverwas.me>:
>
> Hi Fadi,
>
> Fadi Moukayed <smfadi@gmail.com> writes:
>
> > Tags: patch
> > Severity: wishlist
> >
> > Hi all,
> >
> > I am submitting a trivial patch adding a simple customizable variable
> > (erc-format-spoilers) to Erc, allowing the user to control how Erc
> > displays spoiler text when mIRC formatting code interpretation is
> > enabled.
> > This idea for this patch was discussed with the Erc maintainers on
> > #erc, and was deemed to be useful enough to warrant a submission.
> >
> > Note that this is my first attempt to contribute to GNU Emacs.
>
> Welcome! I think this would make a helpful addition.
>
> However, I'm also starting to wonder if the way `erc-spoiler-face'
> currently operates doesn't constitute buggy behavior. Skimming the
> related commits and nonexistent discussion history, I see no clear
> indication as to motivation or reasoning, making me think it was just
> chucked in on a whim. Frankly, it calls into question the fitness of all
> involved (ahem). Really, though, the only legitimate use case that comes
> to mind is possibly emphasizing the presence of spoilers when an
> IRC-formatted fg/bg combo matches the buffer's own defaults, meaning the
> span might otherwise be mistaken for whitespace. But that doesn't seem
> like a common occurrence, and I doubt anyone would intentionally make
> `fg:erc-color-face0' or `fg:erc-color-face1' match `erc-default-face' or
> the `default' face's :background.
>
> 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.
>
> Another (competing) idea would be to instead have the option specify a
> regexp pattern along with color combinations that ERC could use to
> determine if a candidate is likely spoiler text, which would then be
> shown accordingly. Somehow, though, I'm rather dubious anyone would
> actually bother configuring such a thing.
>
> (Also, on a related note, we should probably add a `cursor-face'
> property to complement the `mouse-face' one currently added to spoilers
> and maybe also mention `cursor-face-highlight-mode' in the doc string.)
>
> Anyhow, I'm not suggesting you need to take on any of what I've just
> mentioned, especially with the fifteen-line limit in effect (unless of
> course your paperwork comes through in record time or you're feeling up
> for the challenge). That said, I'd still like your input on these
> matters if you don't mind. From the Transient project you shared and our
> wider discussion in the channel, it's clear you've thought more about
> this stuff than anyone else around, especially these days.
>
> J.P.
>
> > In GNU Emacs 29.2 (build 2, x86_64-pc-linux-gnu, GTK+ Version 3.24.20,
> >  cairo version 1.16.0) of 2024-02-27 built on lcy02-amd64-095
> [...]
> >
> > From 4d3b8fa17a975d6f04ba2a6ef4865d3938a76315 Mon Sep 17 00:00:00 2001
> > From: "F. Moukayed" <smfadi+emacs@gmail.com>
> > Date: Wed, 6 Mar 2024 18:33:46 +0000
> > Subject: [PATCH] * lisp/erc/erc.el: (erc-format-spoilers): Add a new
> >  customizable variable controling how Erc displays spoilers
>                                ^
> >
> > ---
> >  lisp/erc/erc-goodies.el | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/lisp/erc/erc-goodies.el b/lisp/erc/erc-goodies.el
> > index 7e30b10..211d704 100644
> > --- a/lisp/erc/erc-goodies.el
> > +++ b/lisp/erc/erc-goodies.el
> > @@ -645,6 +645,11 @@ emergency (message flood) it can be turned off to save 
> > processing time.  See
> >
> >  (defcustom erc-interpret-mirc-color nil
> >    "If non-nil, ERC will interpret mIRC color codes."
> > +  :type 'boolean
> > +  :group 'erc-control-characters)
> > +
> > +(defcustom erc-format-spoilers nil
> > +  "If non-nil, ERC will format spoilers with `erc-spoiler-face'."
> >    :type 'boolean)
> >
> >  (defcustom erc-beep-p nil
> > @@ -968,7 +973,7 @@ 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))
> > +  (if (and fg bg (equal fg bg) erc-format-spoilers)
> >        (progn
> >          (setq fg 'erc-spoiler-face
> >                bg nil)
>
> P.S. These changes look fine, I think.
>

0001-lisp-erc-erc-goodies-redefine-rework-erc-spoilers.patch
Description: Text Data