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

Previous Next

Full log

Message #17 received at 69597 <at> debbugs.gnu.org (full text, mbox):

From: Fadi Moukayed <smfadi <at> gmail.com>
To: "J.P." <jp <at> neverwas.me>
Cc: emacs-erc <at> gnu.org, 69597 <at> debbugs.gnu.org
Subject: Re: bug#69597: 29.2; ERC 5.6-git: Add a new customizable variable
 controlling how Erc displays spoilers
Date: Fri, 8 Mar 2024 17:47:33 +0100

[Message part 1 (text/plain, inline)]

Thanks JP,

Good news: With both of your patches applied (and including the
revised patch for erc-goodies.el, attached to this message), things
seem to behave exactly as expected. Spoilers are displayed correctly,
and hovering the mouse on them causes them to get revealed as
`erc-spoiler-face'.

The only issue I noticed after applying the patches, is that the
following warning is emitted on the *Messages* buffer – (Note that I
have native compilation enabled):

> ⛔ Warning (comp): erc-button.el:532:4: Warning: the function ‘erc--restore-important-text-props’ is not known to be defined.

I assume this is a compilation order issue? Note that this only
happens with a clean ELN cache (The following Emacs loads are fine),
so not sure how significant it is.

>Happy to explain whatever doesn't make sense

One question regarding "FIXME use a region instead of point-min/max"
in patch #0002, is there a reason why (region-beginning) /
(region-end) is indeed not used instead? Just mentioning that because
IIRC, point-{min,max} is a range over the entire (narrowed) buffer,
including the (buttonized) nick, message text, possible timestamp (if
activated) as well. I checked the properties on the whole message line
while testing and it doesn't seem to have any negative side-effects,
aside from the fact that it operates on more text than it has to – I
believe it need only be applied to the message text.

Cheers,
FM

Am Fr., 8. März 2024 um 16:05 Uhr schrieb J.P. <jp <at> neverwas.me>:
>
> Fadi Moukayed <smfadi <at> 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 <at> 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-erc-redefine-rework-erc-spoilers.patch (text/x-patch, attachment)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.