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

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

[Fadi Moukayed]

[-- Attachment #1: Type: text/plain, Size: 7090 bytes --]

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.

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
Repository revision: ac89b1141a261b40ab5607f8d74209ede4c164cc
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12201001
System Description: Ubuntu 22.04.4 LTS

Configured using:
 'configure --prefix=/snap/emacs/current/usr --with-x-toolkit=gtk3
 --without-xaw3d --with-modules --with-cairo
 --with-native-compilation=aot --without-pgtk --with-xinput2
 --with-tree-sitter --with-json
 'CFLAGS=-isystem/build/emacs/parts/emacs/install/usr/include
 -isystem/build/emacs/parts/emacs/install/usr/include/x86_64-linux-gnu
 -isystem/build/emacs/stage/usr/include -O2'
 'CPPFLAGS=-isystem/build/emacs/parts/emacs/install/usr/include
 -isystem/build/emacs/parts/emacs/install/usr/include/x86_64-linux-gnu
 -isystem/build/emacs/stage/usr/include'
 'LDFLAGS=-L/build/emacs/parts/emacs/install/lib
 -L/build/emacs/parts/emacs/install/usr/lib
 -L/build/emacs/parts/emacs/install/lib/x86_64-linux-gnu
 -L/build/emacs/parts/emacs/install/usr/lib/x86_64-linux-gnu
 -L/build/emacs/stage/usr/lib''

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 M17N_FLT MODULES
NATIVE_COMP NOTIFY INOTIFY PDUMPER PNG RSVG SECCOMP SOUND SQLITE3
THREADS TIFF TOOLKIT_SCROLL_BARS TREE_SITTER X11 XDBE XIM XINPUT2 XPM
GTK3 ZLIB

Important settings:
  value of $LANG: de_DE.UTF-8
  value of $XMODIFIERS: @im=ibus
  locale-coding-system: utf-8-unix

Major mode: ERC

Minor modes in effect:
  erc-ring-mode: t
  erc-notifications-mode: t
  erc-nicks-mode: t
  erc-netsplit-mode: t
  erc-menu-mode: t
  erc-list-mode: t
  erc-irccontrols-mode: t
  erc-keep-place-mode: t
  erc-move-to-prompt-mode: t
  erc-readonly-mode: t
  erc-scrolltobottom-mode: t
  erc-imenu-mode: t
  erc-pcomplete-mode: t
  erc-button--phantom-users-mode: t
  erc-button-mode: t
  erc-fill-wrap-mode: t
  erc-fill-mode: t
  erc-stamp--date-mode: t
  erc-stamp--display-margin-mode: t
  erc-stamp-mode: t
  erc-bufbar-mode: t
  erc-track-mode: (t erc-nicks--setup-track-integration)
  erc-track-minor-mode: t
  erc-match-mode: t
  erc-autojoin-mode: t
  erc-autoaway-mode: t
  recentf-mode: t
  pixel-scroll-precision-mode: t
  minibuffer-depth-indicate-mode: t
  global-whitespace-mode: t
  global-goto-address-mode: t
  goto-address-mode: t
  global-auto-revert-mode: t
  fido-vertical-mode: t
  icomplete-vertical-mode: t
  icomplete-mode: t
  fido-mode: t
  erc-networks-mode: t
  desktop-save-mode: t
  windmove-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  column-number-mode: t
  line-number-mode: t
  visual-line-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Features:
(shadow sort mail-extr emacsbug mule-util hl-line network-stream nsm
transient edmacro kmacro display-line-numbers org-element org-persist
org-id org-refile avl-tree generator oc-basic ol-eww eww xdg url-queue
mm-url ol-rmail ol-mhe ol-irc ol-info ol-gnus nnselect gnus-art mm-uu
mml2015 mm-view mml-smime smime gnutls dig gnus-sum shr pixel-fill
kinsoku url-file svg dom gnus-group gnus-undo gnus-start gnus-dbus
gnus-cloud nnimap nnmail mail-source utf7 nnoo parse-time gnus-spec
gnus-int gnus-range message sendmail yank-media puny rfc822 mml mml-sec
epa derived epg rfc6068 epg-config mm-decode mm-bodies mm-encode
mail-parse rfc2231 rfc2047 rfc2045 ietf-drums mailabbrev gmm-utils
mailheader gnus-win gnus nnheader gnus-util text-property-search
mail-utils range mm-util mail-prsvr ol-docview doc-view jka-compr
image-mode exif dired dired-loaddefs ol-bibtex bibtex iso8601 ol-bbdb
ol-w3m ol-doi org-link-doi org ob ob-tangle ob-ref ob-lob ob-table
ob-exp org-macro org-src ob-comint org-pcomplete org-list org-footnote
org-faces org-entities noutline outline ob-emacs-lisp ob-core ob-eval
org-cycle org-table ol org-fold org-fold-core org-keys oc org-loaddefs
find-func cal-menu calendar cal-loaddefs org-version org-compat org-macs
disp-table erc-ring erc-desktop-notifications notifications dbus xml
erc-nicks color erc-netsplit erc-menu erc-list erc-goodies erc-imenu
imenu erc-pcomplete time-date pcomplete comint ansi-osc ansi-color
erc-button erc-fill erc-stamp erc-status-sidebar erc-track erc-match
erc-join erc-autoaway leuven-dark-theme recentf tree-widget wid-edit
pixel-scroll cua-base ring mb-depth whitespace goto-addr thingatpt
autorevert filenotify icomplete erc format-spec erc-backend erc-networks
erc-common erc-compat compat erc-loaddefs desktop frameset cus-load
windmove site-start comp comp-cstr warnings icons rx cl-extra help-mode
erc-autoloads info compat-autoloads markdown-mode-autoloads package
browse-url url url-proxy url-privacy url-expand url-methods url-history
url-cookie generate-lisp-file url-domsuf url-util mailcap url-handlers
url-parse auth-source cl-seq eieio eieio-core cl-macs password-cache
json subr-x map byte-opt gv bytecomp byte-compile url-vars cl-loaddefs
cl-lib rmc iso-transl tooltip cconv eldoc paren electric uniquify
ediff-hook vc-hooks lisp-float-type elisp-mode mwheel term/x-win x-win
term/common-win x-dnd tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode lisp-mode prog-mode register
page tab-bar menu-bar rfn-eshadow isearch easymenu timer select
scroll-bar mouse jit-lock font-lock syntax font-core term/tty-colors
frame minibuffer nadvice seq simple cl-generic indonesian philippine
cham georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese eucjp-ms cp51932 hebrew greek romanian slovak czech
european ethiopic indian cyrillic chinese composite emoji-zwj charscript
charprop case-table epa-hook jka-cmpr-hook help abbrev obarray oclosure
cl-preloaded button loaddefs theme-loaddefs faces cus-face macroexp
files window text-properties overlay sha1 md5 base64 format env
code-pages mule custom widget keymap hashtable-print-readable backquote
threads dbusbind inotify lcms2 dynamic-setting system-font-setting
font-render-setting cairo move-toolbar gtk x-toolkit xinput2 x multi-tty
make-network-process native-compile emacs)

Memory information:
((conses 16 397463 44016)
 (symbols 48 27808 0)
 (strings 32 98182 4397)
 (string-bytes 1 3015974)
 (vectors 16 52846)
 (vector-slots 8 919020 87741)
 (floats 8 532 941)
 (intervals 56 4698 372)
 (buffers 984 16))

[-- Attachment #2: 0001-erc-format-spoilers-Add-a-new-custom.patch --]
[-- Type: text/x-patch, Size: 1378 bytes --]

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)
-- 
2.34.1

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

[J.P.]

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.

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

[Fadi Moukayed]

[-- Attachment #1: Type: text/plain, Size: 6543 bytes --]

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

[-- Attachment #2: 0001-lisp-erc-erc-goodies-redefine-rework-erc-spoilers.patch --]
[-- Type: text/x-patch, Size: 1771 bytes --]

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))
+  (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
-- 
2.34.1

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

[J.P.]

[-- Attachment #1: Type: text/plain, Size: 5606 bytes --]

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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-5.6-Fix-misleading-test-in-erc-goodies.patch --]
[-- Type: text/x-patch, Size: 3780 bytes --]

From f5473bd8c8fba7c5685f1a4cadb6e6f3eb9c6f27 Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
Date: Thu, 7 Mar 2024 21:53:11 -0800
Subject: [PATCH 1/2] [5.6] Fix misleading test in erc-goodies

* test/lisp/erc/erc-goodies-tests.el
(erc-controls-highlight--inverse): Don't shadow hook with an
unrealistic subset of members, in this case a lone member:
`erc-controls-highlight'.  Adjust expected buffer state to reflect new
role of `erc-spoiler-face'.  (Bug#69597)
---
 test/lisp/erc/erc-goodies-tests.el | 60 +++++++++++++++---------------
 1 file changed, 29 insertions(+), 31 deletions(-)

diff --git a/test/lisp/erc/erc-goodies-tests.el b/test/lisp/erc/erc-goodies-tests.el
index 7013ce0c8fc..ddc29acff1e 100644
--- a/test/lisp/erc/erc-goodies-tests.el
+++ b/test/lisp/erc/erc-goodies-tests.el
@@ -131,37 +131,35 @@ erc-controls-highlight--examples
 
 (ert-deftest erc-controls-highlight--inverse ()
   (should (eq t erc-interpret-controls-p))
-  (let ((erc-insert-modify-hook '(erc-controls-highlight))
-        erc-kill-channel-hook erc-kill-server-hook erc-kill-buffer-hook)
-    (with-current-buffer (get-buffer-create "#chan")
-      (erc-mode)
-      (setq-local erc-interpret-mirc-color t)
-      (erc--initialize-markers (point) nil)
-
-      (let* ((m "Spoiler: \C-c0,0Hello\C-c1,1World!")
-             (msg (erc-format-privmessage "bob" m nil t)))
-        (erc-display-message nil nil (current-buffer) msg))
-      (forward-line -1)
-      (should (search-forward "<bob> " nil t))
-      (save-restriction
-        (narrow-to-region (point) (pos-eol))
-        (should (eq (get-text-property (+ 9 (point)) 'mouse-face)
-                    'erc-inverse-face))
-        (should (eq (get-text-property (1- (pos-eol)) 'mouse-face)
-                    'erc-inverse-face))
-        (erc-goodies-tests--assert-face
-         0 "Spoiler: " 'erc-default-face
-         '(fg:erc-color-face0 bg:erc-color-face0))
-        (erc-goodies-tests--assert-face
-         9 "Hello" '(erc-spoiler-face)
-         '( fg:erc-color-face0 bg:erc-color-face0
-            fg:erc-color-face1 bg:erc-color-face1))
-        (erc-goodies-tests--assert-face
-         18 " World" '(erc-spoiler-face)
-         '( fg:erc-color-face0 bg:erc-color-face0
-            fg:erc-color-face1 bg:erc-color-face1 )))
-      (when noninteractive
-        (kill-buffer)))))
+  (erc-tests-common-make-server-buf)
+  (with-current-buffer (erc--open-target "#chan")
+    (setq-local erc-interpret-mirc-color t)
+    (let* ((m "Spoiler: \C-c0,0Hello\C-c1,1World!")
+           (msg (erc-format-privmessage "bob" m nil t)))
+      (erc-display-message nil nil (current-buffer) msg))
+    (forward-line -1)
+    (should (search-forward "<bob> " nil t))
+    (save-restriction
+      ;; Narrow to EOL or start of right-side stamp.
+      (narrow-to-region (point) (line-end-position))
+      (should (eq (get-text-property (+ 9 (point)) 'mouse-face)
+                  'erc-spoiler-face))
+      (should (eq (get-text-property (1- (pos-eol)) 'mouse-face)
+                  'erc-spoiler-face))
+      ;; "Spoiler" appears in ERC default face.
+      (erc-goodies-tests--assert-face
+       0 "Spoiler: " 'erc-default-face
+       '(fg:erc-color-face0 bg:erc-color-face0))
+      ;; "Hello" is masked in all white.
+      (erc-goodies-tests--assert-face
+       9 "Hello" '(fg:erc-color-face0 bg:erc-color-face0)
+       '(fg:erc-color-face1 bg:erc-color-face1))
+      ;; "World" is masked in all black.
+      (erc-goodies-tests--assert-face
+       18 " World" '(fg:erc-color-face1 bg:erc-color-face1 )
+       '(fg:erc-color-face0 bg:erc-color-face0))))
+  (when noninteractive
+    (erc-tests-common-kill-buffers)))
 
 (defvar erc-goodies-tests--motd
   ;; This is from ergo's MOTD
-- 
2.44.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-5.6-Make-important-text-props-more-resilient-in-ERC.patch --]
[-- Type: text/x-patch, Size: 6246 bytes --]

From a66abc007c071f224b459cffdc2a451e36a80903 Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
Date: Thu, 7 Mar 2024 21:53:23 -0800
Subject: [PATCH 2/2] [5.6] Make important text props more resilient in ERC

* lisp/erc/erc-button.el (erc-button-remove-old-buttons): Restore
original `mouse-face' values in areas marked as important after
clobbering.
* lisp/erc/erc.el (erc--reserve-important-text-props): New function.
(erc--restore-important-text-props): New function.
* test/lisp/erc/erc-tests.el
(erc--restore-important-text-props): New test.
(Bug#69597)
---
 lisp/erc/erc-button.el     |  3 ++-
 lisp/erc/erc.el            | 32 +++++++++++++++++++++++
 test/lisp/erc/erc-tests.el | 52 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 86 insertions(+), 1 deletion(-)

diff --git a/lisp/erc/erc-button.el b/lisp/erc/erc-button.el
index 6b78e451b54..4b4930e5bff 100644
--- a/lisp/erc/erc-button.el
+++ b/lisp/erc/erc-button.el
@@ -528,7 +528,8 @@ erc-button-remove-old-buttons
    '(erc-callback nil
                   erc-data nil
                   mouse-face nil
-                  keymap nil)))
+                  keymap nil))
+  (erc--restore-important-text-props '(mouse-face)))
 
 (defun erc-button-add-button (from to fun nick-p &optional data regexp)
   "Create a button between FROM and TO with callback FUN and data DATA.
diff --git a/lisp/erc/erc.el b/lisp/erc/erc.el
index cce3b2508fb..49b51c5d74c 100644
--- a/lisp/erc/erc.el
+++ b/lisp/erc/erc.el
@@ -3532,6 +3532,38 @@ erc--remove-from-prop-value-list
             old (get-text-property pos prop object)
             end (next-single-property-change pos prop object to)))))
 
+(defun erc--reserve-important-text-props (beg end plist)
+  "Record text-property pairs in PLIST as important between BEG and END.
+Also mark the message being inserted as containing these important props
+so modules performing destructive modifications can later restore them.
+Expect to run in a narrowed buffer at message-insertion time."
+  (when erc--msg-props
+    (let ((existing (erc--check-msg-prop 'erc--important-prop-names)))
+      (puthash 'erc--important-prop-names
+               (seq-union existing (cl-loop for (key _) on plist by #'cddr
+                                            collect key))
+               erc--msg-props)))
+  (erc--merge-prop beg end 'erc--important-props plist))
+
+;; FIXME use a region instead of point-min/max.
+(defun erc--restore-important-text-props (props)
+  "Restore PROPS where recorded in the accessible portion of the buffer.
+Expect to run in a narrowed buffer at message-insertion time."
+  (when-let ((registered (erc--check-msg-prop 'erc--important-prop-names))
+             (present (seq-intersection props registered))
+             (p (point-min))
+             (end (point-max)))
+    (while-let (((setq p (text-property-not-all p end
+                                                'erc--important-props nil)))
+                (val (get-text-property p 'erc--important-props))
+                (q (next-single-property-change p 'erc--important-props
+                                                nil end)))
+      (while-let ((k (pop val))
+                  (v (pop val)))
+        (when (memq k present)
+          (put-text-property p q k v)))
+      (setq p q))))
+
 (defvar erc-legacy-invisible-bounds-p nil
   "Whether to hide trailing rather than preceding newlines.
 Beginning in ERC 5.6, invisibility extends from a message's
diff --git a/test/lisp/erc/erc-tests.el b/test/lisp/erc/erc-tests.el
index 085b063bdb2..6809d9db41d 100644
--- a/test/lisp/erc/erc-tests.el
+++ b/test/lisp/erc/erc-tests.el
@@ -2232,6 +2232,58 @@ erc--remove-from-prop-value-list/many
     (when noninteractive
       (kill-buffer))))
 
+(ert-deftest erc--restore-important-text-props ()
+  (erc-mode)
+  (let ((erc--msg-props (map-into '((erc--important-prop-names a))
+                                  'hash-table)))
+    (insert (propertize "foo" 'a 'A 'b 'B 'erc--important-props '(a A))
+            " "
+            (propertize "bar" 'c 'C 'a 'A 'b 'B
+                        'erc--important-props '(a A c C)))
+
+    ;; Attempt to restore a and c when only a is registered.
+    (remove-list-of-text-properties (point-min) (point-max) '(a c))
+    (erc--restore-important-text-props '(a c))
+    (should (erc-tests-common-equal-with-props
+             (buffer-string)
+             #("foo bar"
+               0 3 (a A b B erc--important-props (a A))
+               4 7 (a A b B erc--important-props (a A c C)))))
+
+    ;; Add d between 3 and 6.
+    (erc--reserve-important-text-props 3 6 '(d D))
+    (put-text-property 3 6 'd 'D)
+    (should (erc-tests-common-equal-with-props
+             (buffer-string)
+             #("foo bar" ; #1
+               0 2 (a A b B erc--important-props (a A))
+               2 3 (d D a A b B erc--important-props (d D a A))
+               3 4 (d D erc--important-props (d D))
+               4 5 (d D a A b B erc--important-props (d D a A c C))
+               5 7 (a A b B erc--important-props (a A c C)))))
+    ;; Remove a and d, and attempt to restore d.
+    (remove-list-of-text-properties (point-min) (point-max) '(a d))
+    (erc--restore-important-text-props '(d))
+    (should (erc-tests-common-equal-with-props
+             (buffer-string)
+             #("foo bar"
+               0 2 (b B erc--important-props (a A))
+               2 3 (d D b B erc--important-props (d D a A))
+               3 4 (d D erc--important-props (d D))
+               4 5 (d D b B erc--important-props (d D a A c C))
+               5 7 (b B erc--important-props (a A c C)))))
+
+    ;; Restore a only.
+    (erc--restore-important-text-props '(a))
+    (should (erc-tests-common-equal-with-props
+             (buffer-string)
+             #("foo bar" ; same as #1 above
+               0 2 (a A b B erc--important-props (a A))
+               2 3 (d D a A b B erc--important-props (d D a A))
+               3 4 (d D erc--important-props (d D))
+               4 5 (d D a A b B erc--important-props (d D a A c C))
+               5 7 (a A b B erc--important-props (a A c C)))))))
+
 (ert-deftest erc--split-string-shell-cmd ()
 
   ;; Leading and trailing space
-- 
2.44.0

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

[Fadi Moukayed]

[-- Attachment #1: Type: text/plain, Size: 7371 bytes --]

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@neverwas.me>:
>
> 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
>

[-- Attachment #2: 0001-erc-redefine-rework-erc-spoilers.patch --]
[-- Type: text/x-patch, Size: 2210 bytes --]

From ac0b308f6b300eb454ebdf9ba4526fac012f1a52 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 | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/lisp/erc/erc-goodies.el b/lisp/erc/erc-goodies.el
index 7e30b10..611fdbe 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,19 @@ 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))
+    (add-text-properties from to '(mouse-face
+                                   erc-spoiler-face
+                                   cursor-face
+                                   erc-spoiler-face)
+                         str))
+  (erc--reserve-important-text-props from to
+                                     '(mouse-face
+                                       erc-spoiler-face
+                                       cursor-face
+                                       erc-spoiler-face))
+  (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
-- 
2.34.1

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

[J.P.]

[-- Attachment #1: Type: text/plain, Size: 3343 bytes --]

Fadi Moukayed <smfadi@gmail.com> writes:

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

Hm, unless I messed something up (definitely possible), that shouldn't
happen . For ERC, I typically remove all the lisp/erc/*.elc files after
every change and before rerunning Make, regardless of whether native
comp is enabled (though removing native-lisp/30.0.50-deadbeef/*.eln
isn't usually necessary, AFAICT). Sometimes, though, I also have to
remove lisp/loaddefs.* and others. In case you weren't aware, there are
recipes for regenerating various autoloads and Custom business in
lisp/Makefile, but I usually just delete all stale assets completely.

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

Unfortunately, insertion-hook members lack a means for detecting message
boundaries unequivocally, although they can obviously keep track of
their own modifications and make assertions accordingly. So I agree that
allowing the caller to specify BEG and END in cases where they're
already known makes sense. For example, if they've already scanned for
the end of the speaker name to accomplish some other task or happen to
know the start of the right stamp, it's worth passing that knowledge
along. But computing a sub-region specially, beforehand, just to call
this function is likely less efficient (not that you were suggesting
that). And, of course, accepting BEG/END parameters make it easier to
protect areas of the exposed buffer from props restoration, if ever
there's a need.

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

This is news to me, but apparently there's a Git hook that complains
about overlong subject lines. After running git-am(1) to apply your
latest patch, I saw:

  Line longer than 78 characters in commit message
  Commit aborted; please see the file CONTRIBUTE

So I adjusted the message to conform to this requirement. If the
attached changes look all right to you, then I'll install them (or
something similar) in the coming days.

Thanks,
J.P.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0000-v1-v2.diff --]
[-- Type: text/x-patch, Size: 6254 bytes --]

From acc9d0c4c1394a6e52bf34c59318888d91d249a6 Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
Date: Fri, 8 Mar 2024 17:45:54 -0800
Subject: [PATCH 0/3] *** NOT A PATCH ***

. Rename the test.

. Add `object' parameter to `erc--reserve-important-text-props'.

. Subject `erc--reserve-important-text-props' call site to the same guard
  condition as `add-text-properties', so messages that don't include spoilers
  aren't marked as having important props (thus allowing healing logic to
  ignore those messages instead of scan them unnecessarily).

F. Jason Park (2):
  [5.6] Fix misleading test in erc-goodies
  [5.6] Make important text props more resilient in ERC

F. Moukayed (1):
  [5.6] Redefine erc-spoilers-face to indicate revealed text

 lisp/erc/erc-button.el             |  3 +-
 lisp/erc/erc-goodies.el            | 21 +++++-----
 lisp/erc/erc.el                    | 34 ++++++++++++++++
 test/lisp/erc/erc-goodies-tests.el | 62 +++++++++++++++---------------
 test/lisp/erc/erc-tests.el         | 52 +++++++++++++++++++++++++
 5 files changed, 129 insertions(+), 43 deletions(-)

Interdiff:
diff --git a/lisp/erc/erc-goodies.el b/lisp/erc/erc-goodies.el
index 611fdbedf7b..212cdbfa9ef 100644
--- a/lisp/erc/erc-goodies.el
+++ b/lisp/erc/erc-goodies.el
@@ -967,16 +967,13 @@ erc-controls-propertize
 If optional argument STR is provided, apply to STR, otherwise prepend properties
 to a region in the current buffer."
   (when (and fg bg (equal fg bg))
-    (add-text-properties from to '(mouse-face
-                                   erc-spoiler-face
-                                   cursor-face
-                                   erc-spoiler-face)
-                         str))
-  (erc--reserve-important-text-props from to
-                                     '(mouse-face
-                                       erc-spoiler-face
-                                       cursor-face
-                                       erc-spoiler-face))
+    (add-text-properties from to '( mouse-face erc-spoiler-face
+                                    cursor-face erc-spoiler-face)
+                         str)
+    (erc--reserve-important-text-props from to
+                                       '( mouse-face erc-spoiler-face
+                                          cursor-face erc-spoiler-face)
+                                       str))
   (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
diff --git a/lisp/erc/erc.el b/lisp/erc/erc.el
index 49b51c5d74c..08bc9939b9a 100644
--- a/lisp/erc/erc.el
+++ b/lisp/erc/erc.el
@@ -3532,37 +3532,39 @@ erc--remove-from-prop-value-list
             old (get-text-property pos prop object)
             end (next-single-property-change pos prop object to)))))
 
-(defun erc--reserve-important-text-props (beg end plist)
+(defun erc--reserve-important-text-props (beg end plist &optional object)
   "Record text-property pairs in PLIST as important between BEG and END.
 Also mark the message being inserted as containing these important props
 so modules performing destructive modifications can later restore them.
 Expect to run in a narrowed buffer at message-insertion time."
   (when erc--msg-props
     (let ((existing (erc--check-msg-prop 'erc--important-prop-names)))
-      (puthash 'erc--important-prop-names
-               (seq-union existing (cl-loop for (key _) on plist by #'cddr
-                                            collect key))
+      (puthash 'erc--important-prop-names (seq-union existing (map-keys plist))
                erc--msg-props)))
-  (erc--merge-prop beg end 'erc--important-props plist))
+  (erc--merge-prop beg end 'erc--important-props plist object))
 
-;; FIXME use a region instead of point-min/max.
-(defun erc--restore-important-text-props (props)
+(defun erc--restore-important-text-props (props &optional beg end)
   "Restore PROPS where recorded in the accessible portion of the buffer.
-Expect to run in a narrowed buffer at message-insertion time."
+Expect to run in a narrowed buffer at message-insertion time.  Limit the
+effect to the region between buffer positions BEG and END, when non-nil.
+
+Callers should be aware that this function fails if the property
+`erc--important-props' has an empty value almost anywhere along the
+affected region.  Use the function `erc--remove-from-prop-value-list' to
+ensure that props with empty values are excised completely."
   (when-let ((registered (erc--check-msg-prop 'erc--important-prop-names))
              (present (seq-intersection props registered))
-             (p (point-min))
-             (end (point-max)))
-    (while-let (((setq p (text-property-not-all p end
-                                                'erc--important-props nil)))
-                (val (get-text-property p 'erc--important-props))
-                (q (next-single-property-change p 'erc--important-props
-                                                nil end)))
+             (b (or beg (point-min)))
+             (e (or end (point-max))))
+    (while-let
+        (((setq b (text-property-not-all b e 'erc--important-props nil)))
+         (val (get-text-property b 'erc--important-props))
+         (q (next-single-property-change b 'erc--important-props nil e)))
       (while-let ((k (pop val))
                   (v (pop val)))
         (when (memq k present)
-          (put-text-property p q k v)))
-      (setq p q))))
+          (put-text-property b q k v)))
+      (setq b q))))
 
 (defvar erc-legacy-invisible-bounds-p nil
   "Whether to hide trailing rather than preceding newlines.
diff --git a/test/lisp/erc/erc-goodies-tests.el b/test/lisp/erc/erc-goodies-tests.el
index ddc29acff1e..0ab40808a4a 100644
--- a/test/lisp/erc/erc-goodies-tests.el
+++ b/test/lisp/erc/erc-goodies-tests.el
@@ -129,7 +129,7 @@ erc-controls-highlight--examples
 ;; Hovering over the redacted area should reveal its underlying text
 ;; in a high-contrast face.
 
-(ert-deftest erc-controls-highlight--inverse ()
+(ert-deftest erc-controls-highlight--spoilers ()
   (should (eq t erc-interpret-controls-p))
   (erc-tests-common-make-server-buf)
   (with-current-buffer (erc--open-target "#chan")
-- 
2.44.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0001-5.6-Fix-misleading-test-in-erc-goodies.patch --]
[-- Type: text/x-patch, Size: 4012 bytes --]

From 5ceebcb6718c8140ea58f659a3b295a12d9e9a4f Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
Date: Thu, 7 Mar 2024 21:53:11 -0800
Subject: [PATCH 1/3] [5.6] Fix misleading test in erc-goodies

* test/lisp/erc/erc-goodies-tests.el
(erc-controls-highlight--inverse, erc-controls-highlight--spoilers):
Rename former to latter, and don't shadow `erc-insert-modify-hook'
with an unrealistic subset of members, in this case a lone member:
`erc-controls-highlight'.  Adjust expected buffer state to reflect new
role of `erc-spoiler-face'.  (Bug#69597)
---
 test/lisp/erc/erc-goodies-tests.el | 62 +++++++++++++++---------------
 1 file changed, 30 insertions(+), 32 deletions(-)

diff --git a/test/lisp/erc/erc-goodies-tests.el b/test/lisp/erc/erc-goodies-tests.el
index 7013ce0c8fc..0ab40808a4a 100644
--- a/test/lisp/erc/erc-goodies-tests.el
+++ b/test/lisp/erc/erc-goodies-tests.el
@@ -129,39 +129,37 @@ erc-controls-highlight--examples
 ;; Hovering over the redacted area should reveal its underlying text
 ;; in a high-contrast face.
 
-(ert-deftest erc-controls-highlight--inverse ()
+(ert-deftest erc-controls-highlight--spoilers ()
   (should (eq t erc-interpret-controls-p))
-  (let ((erc-insert-modify-hook '(erc-controls-highlight))
-        erc-kill-channel-hook erc-kill-server-hook erc-kill-buffer-hook)
-    (with-current-buffer (get-buffer-create "#chan")
-      (erc-mode)
-      (setq-local erc-interpret-mirc-color t)
-      (erc--initialize-markers (point) nil)
-
-      (let* ((m "Spoiler: \C-c0,0Hello\C-c1,1World!")
-             (msg (erc-format-privmessage "bob" m nil t)))
-        (erc-display-message nil nil (current-buffer) msg))
-      (forward-line -1)
-      (should (search-forward "<bob> " nil t))
-      (save-restriction
-        (narrow-to-region (point) (pos-eol))
-        (should (eq (get-text-property (+ 9 (point)) 'mouse-face)
-                    'erc-inverse-face))
-        (should (eq (get-text-property (1- (pos-eol)) 'mouse-face)
-                    'erc-inverse-face))
-        (erc-goodies-tests--assert-face
-         0 "Spoiler: " 'erc-default-face
-         '(fg:erc-color-face0 bg:erc-color-face0))
-        (erc-goodies-tests--assert-face
-         9 "Hello" '(erc-spoiler-face)
-         '( fg:erc-color-face0 bg:erc-color-face0
-            fg:erc-color-face1 bg:erc-color-face1))
-        (erc-goodies-tests--assert-face
-         18 " World" '(erc-spoiler-face)
-         '( fg:erc-color-face0 bg:erc-color-face0
-            fg:erc-color-face1 bg:erc-color-face1 )))
-      (when noninteractive
-        (kill-buffer)))))
+  (erc-tests-common-make-server-buf)
+  (with-current-buffer (erc--open-target "#chan")
+    (setq-local erc-interpret-mirc-color t)
+    (let* ((m "Spoiler: \C-c0,0Hello\C-c1,1World!")
+           (msg (erc-format-privmessage "bob" m nil t)))
+      (erc-display-message nil nil (current-buffer) msg))
+    (forward-line -1)
+    (should (search-forward "<bob> " nil t))
+    (save-restriction
+      ;; Narrow to EOL or start of right-side stamp.
+      (narrow-to-region (point) (line-end-position))
+      (should (eq (get-text-property (+ 9 (point)) 'mouse-face)
+                  'erc-spoiler-face))
+      (should (eq (get-text-property (1- (pos-eol)) 'mouse-face)
+                  'erc-spoiler-face))
+      ;; "Spoiler" appears in ERC default face.
+      (erc-goodies-tests--assert-face
+       0 "Spoiler: " 'erc-default-face
+       '(fg:erc-color-face0 bg:erc-color-face0))
+      ;; "Hello" is masked in all white.
+      (erc-goodies-tests--assert-face
+       9 "Hello" '(fg:erc-color-face0 bg:erc-color-face0)
+       '(fg:erc-color-face1 bg:erc-color-face1))
+      ;; "World" is masked in all black.
+      (erc-goodies-tests--assert-face
+       18 " World" '(fg:erc-color-face1 bg:erc-color-face1 )
+       '(fg:erc-color-face0 bg:erc-color-face0))))
+  (when noninteractive
+    (erc-tests-common-kill-buffers)))
 
 (defvar erc-goodies-tests--motd
   ;; This is from ergo's MOTD
-- 
2.44.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0002-5.6-Make-important-text-props-more-resilient-in-ERC.patch --]
[-- Type: text/x-patch, Size: 6410 bytes --]

From 9b5f8e0a85866ae9245a2b480183f6a36d23a413 Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
Date: Thu, 7 Mar 2024 21:53:23 -0800
Subject: [PATCH 2/3] [5.6] Make important text props more resilient in ERC

* lisp/erc/erc-button.el (erc-button-remove-old-buttons): Restore
original `mouse-face' values in areas marked as important after
clobbering.
* lisp/erc/erc.el (erc--reserve-important-text-props): New function.
(erc--restore-important-text-props): New function.
* test/lisp/erc/erc-tests.el
(erc--restore-important-text-props): New test.
(Bug#69597)
---
 lisp/erc/erc-button.el     |  3 ++-
 lisp/erc/erc.el            | 34 +++++++++++++++++++++++++
 test/lisp/erc/erc-tests.el | 52 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 88 insertions(+), 1 deletion(-)

diff --git a/lisp/erc/erc-button.el b/lisp/erc/erc-button.el
index 6b78e451b54..4b4930e5bff 100644
--- a/lisp/erc/erc-button.el
+++ b/lisp/erc/erc-button.el
@@ -528,7 +528,8 @@ erc-button-remove-old-buttons
    '(erc-callback nil
                   erc-data nil
                   mouse-face nil
-                  keymap nil)))
+                  keymap nil))
+  (erc--restore-important-text-props '(mouse-face)))
 
 (defun erc-button-add-button (from to fun nick-p &optional data regexp)
   "Create a button between FROM and TO with callback FUN and data DATA.
diff --git a/lisp/erc/erc.el b/lisp/erc/erc.el
index cce3b2508fb..08bc9939b9a 100644
--- a/lisp/erc/erc.el
+++ b/lisp/erc/erc.el
@@ -3532,6 +3532,40 @@ erc--remove-from-prop-value-list
             old (get-text-property pos prop object)
             end (next-single-property-change pos prop object to)))))
 
+(defun erc--reserve-important-text-props (beg end plist &optional object)
+  "Record text-property pairs in PLIST as important between BEG and END.
+Also mark the message being inserted as containing these important props
+so modules performing destructive modifications can later restore them.
+Expect to run in a narrowed buffer at message-insertion time."
+  (when erc--msg-props
+    (let ((existing (erc--check-msg-prop 'erc--important-prop-names)))
+      (puthash 'erc--important-prop-names (seq-union existing (map-keys plist))
+               erc--msg-props)))
+  (erc--merge-prop beg end 'erc--important-props plist object))
+
+(defun erc--restore-important-text-props (props &optional beg end)
+  "Restore PROPS where recorded in the accessible portion of the buffer.
+Expect to run in a narrowed buffer at message-insertion time.  Limit the
+effect to the region between buffer positions BEG and END, when non-nil.
+
+Callers should be aware that this function fails if the property
+`erc--important-props' has an empty value almost anywhere along the
+affected region.  Use the function `erc--remove-from-prop-value-list' to
+ensure that props with empty values are excised completely."
+  (when-let ((registered (erc--check-msg-prop 'erc--important-prop-names))
+             (present (seq-intersection props registered))
+             (b (or beg (point-min)))
+             (e (or end (point-max))))
+    (while-let
+        (((setq b (text-property-not-all b e 'erc--important-props nil)))
+         (val (get-text-property b 'erc--important-props))
+         (q (next-single-property-change b 'erc--important-props nil e)))
+      (while-let ((k (pop val))
+                  (v (pop val)))
+        (when (memq k present)
+          (put-text-property b q k v)))
+      (setq b q))))
+
 (defvar erc-legacy-invisible-bounds-p nil
   "Whether to hide trailing rather than preceding newlines.
 Beginning in ERC 5.6, invisibility extends from a message's
diff --git a/test/lisp/erc/erc-tests.el b/test/lisp/erc/erc-tests.el
index 085b063bdb2..6809d9db41d 100644
--- a/test/lisp/erc/erc-tests.el
+++ b/test/lisp/erc/erc-tests.el
@@ -2232,6 +2232,58 @@ erc--remove-from-prop-value-list/many
     (when noninteractive
       (kill-buffer))))
 
+(ert-deftest erc--restore-important-text-props ()
+  (erc-mode)
+  (let ((erc--msg-props (map-into '((erc--important-prop-names a))
+                                  'hash-table)))
+    (insert (propertize "foo" 'a 'A 'b 'B 'erc--important-props '(a A))
+            " "
+            (propertize "bar" 'c 'C 'a 'A 'b 'B
+                        'erc--important-props '(a A c C)))
+
+    ;; Attempt to restore a and c when only a is registered.
+    (remove-list-of-text-properties (point-min) (point-max) '(a c))
+    (erc--restore-important-text-props '(a c))
+    (should (erc-tests-common-equal-with-props
+             (buffer-string)
+             #("foo bar"
+               0 3 (a A b B erc--important-props (a A))
+               4 7 (a A b B erc--important-props (a A c C)))))
+
+    ;; Add d between 3 and 6.
+    (erc--reserve-important-text-props 3 6 '(d D))
+    (put-text-property 3 6 'd 'D)
+    (should (erc-tests-common-equal-with-props
+             (buffer-string)
+             #("foo bar" ; #1
+               0 2 (a A b B erc--important-props (a A))
+               2 3 (d D a A b B erc--important-props (d D a A))
+               3 4 (d D erc--important-props (d D))
+               4 5 (d D a A b B erc--important-props (d D a A c C))
+               5 7 (a A b B erc--important-props (a A c C)))))
+    ;; Remove a and d, and attempt to restore d.
+    (remove-list-of-text-properties (point-min) (point-max) '(a d))
+    (erc--restore-important-text-props '(d))
+    (should (erc-tests-common-equal-with-props
+             (buffer-string)
+             #("foo bar"
+               0 2 (b B erc--important-props (a A))
+               2 3 (d D b B erc--important-props (d D a A))
+               3 4 (d D erc--important-props (d D))
+               4 5 (d D b B erc--important-props (d D a A c C))
+               5 7 (b B erc--important-props (a A c C)))))
+
+    ;; Restore a only.
+    (erc--restore-important-text-props '(a))
+    (should (erc-tests-common-equal-with-props
+             (buffer-string)
+             #("foo bar" ; same as #1 above
+               0 2 (a A b B erc--important-props (a A))
+               2 3 (d D a A b B erc--important-props (d D a A))
+               3 4 (d D erc--important-props (d D))
+               4 5 (d D a A b B erc--important-props (d D a A c C))
+               5 7 (a A b B erc--important-props (a A c C)))))))
+
 (ert-deftest erc--split-string-shell-cmd ()
 
   ;; Leading and trailing space
-- 
2.44.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: 0003-5.6-Redefine-erc-spoilers-face-to-indicate-revealed-.patch --]
[-- Type: text/x-patch, Size: 2154 bytes --]

From acc9d0c4c1394a6e52bf34c59318888d91d249a6 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 3/3] [5.6] Redefine erc-spoilers-face to indicate revealed
 text

* lisp/erc/erc-goodies.el (erc-spoilers-face): Redefine role and
rework definition to inherit from `erc-inverse-face'.
(Bug#69597)

Copyright-paperwork-exempt: yes
---
 lisp/erc/erc-goodies.el | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/lisp/erc/erc-goodies.el b/lisp/erc/erc-goodies.el
index 7e30b1060fd..212cdbfa9ef 100644
--- a/lisp/erc/erc-goodies.el
+++ b/lisp/erc/erc-goodies.el
@@ -665,9 +665,7 @@ erc-inverse-face
   "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,16 @@ erc-controls-propertize
   "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))
+    (add-text-properties from to '( mouse-face erc-spoiler-face
+                                    cursor-face erc-spoiler-face)
+                         str)
+    (erc--reserve-important-text-props from to
+                                       '( mouse-face erc-spoiler-face
+                                          cursor-face erc-spoiler-face)
+                                       str))
+  (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
-- 
2.44.0

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

[Fadi Moukayed]

>I typically remove all the lisp/erc/*.elc files

Thanks, this is what I missed. Deleting all *.elc files solves the
warning issue.

>So I adjusted the message to conform to this requirement. If the
>attached changes look all right to you, then I'll install them (or
>something similar) in the coming days.

Much appreciated. Didn't notice/expect a client-side hook running when
committing.

That said, I'd like to +1 the changeset and confirm that the proposed
changes apply cleanly, and yield the desired result. I tested inputs
of the form ^CX,X<text>^C on a temporary private channel – spoilers
are formatted and revealed as intended, and no regressions were
observed. Very nice. Would be a neat addition/fix for the next Erc
release.

Cheers,
FM

Am Sa., 9. März 2024 um 05:43 Uhr schrieb J.P. <jp@neverwas.me>:
>
> Fadi Moukayed <smfadi@gmail.com> writes:
>
> > 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.
>
> Hm, unless I messed something up (definitely possible), that shouldn't
> happen . For ERC, I typically remove all the lisp/erc/*.elc files after
> every change and before rerunning Make, regardless of whether native
> comp is enabled (though removing native-lisp/30.0.50-deadbeef/*.eln
> isn't usually necessary, AFAICT). Sometimes, though, I also have to
> remove lisp/loaddefs.* and others. In case you weren't aware, there are
> recipes for regenerating various autoloads and Custom business in
> lisp/Makefile, but I usually just delete all stale assets completely.
>
> >>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.
>
> Unfortunately, insertion-hook members lack a means for detecting message
> boundaries unequivocally, although they can obviously keep track of
> their own modifications and make assertions accordingly. So I agree that
> allowing the caller to specify BEG and END in cases where they're
> already known makes sense. For example, if they've already scanned for
> the end of the speaker name to accomplish some other task or happen to
> know the start of the right stamp, it's worth passing that knowledge
> along. But computing a sub-region specially, beforehand, just to call
> this function is likely less efficient (not that you were suggesting
> that). And, of course, accepting BEG/END parameters make it easier to
> protect areas of the exposed buffer from props restoration, if ever
> there's a need.
>
> >> > 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
>
> This is news to me, but apparently there's a Git hook that complains
> about overlong subject lines. After running git-am(1) to apply your
> latest patch, I saw:
>
>   Line longer than 78 characters in commit message
>   Commit aborted; please see the file CONTRIBUTE
>
> So I adjusted the message to conform to this requirement. If the
> attached changes look all right to you, then I'll install them (or
> something similar) in the coming days.
>
> Thanks,
> J.P.

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

[J.P.]

[-- Attachment #1: Type: text/plain, Size: 1270 bytes --]

Fadi Moukayed <smfadi@gmail.com> writes:

> That said, I'd like to +1 the changeset and confirm that the proposed
> changes apply cleanly, and yield the desired result. I tested inputs
> of the form ^CX,X<text>^C on a temporary private channel – spoilers
> are formatted and revealed as intended, and no regressions were
> observed. Very nice. Would be a neat addition/fix for the next Erc
> release.

Really appreciate the thorough testing -- and your patience even more so
because as much as I'd like to put a bow on this, it turns out (sigh)
there's one lingering matter yet unresolved.

Alas, looking more closely at how `erc-controls-propertize' treats
`erc-inverse-face' (crucially, as a modifying toggle [1]), I've quickly
come to rue the day I ever thought to suggest otherwise, especially in
drawing misguided associations with `erc-spoiler-face'. (Indeed, my
quasi-conflating the two was what led us astray to begin with.) So, if
not already clear, I now believe we should just treat `erc-spoiler-face'
as its own concern entirely and not have it inherit from
`erc-inverse-face'. All this to say: yet another revision attached.

Thanks, and apologies for the head fake.

[1] https://modern.ircdocs.horse/formatting#reverse-color



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0000-v2-v3.diff --]
[-- Type: text/x-patch, Size: 10427 bytes --]

From d2ad575e8935981c23846bd54a3eac29b9290f45 Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
Date: Sat, 9 Mar 2024 07:12:16 -0800
Subject: [PATCH 0/3] *** NOT A PATCH ***

F. Jason Park (2):

  [5.6] Leverage inverse-video for erc-inverse-face

  . Add new foreground and background face for color code 99

  . Don't apply hover props for ^C99,99 because, by definition, the fg and bg
    map to different, contrasting colors.

  . Use `:inverse-video' face attribute for `erc-inverse-face' to mimic
    effect prescribed by https://modern.ircdocs.horse/formatting#reverse-color.

  [5.6] Make important text props more resilient in ERC

F. Moukayed (1):

  [5.6] Redefine erc-spoiler-face to indicate revealed text

  . Have `erc-spoiler-face' inherit from `default' instead of
    `erc-inverse-face'.

 lisp/erc/erc-button.el             |   3 +-
 lisp/erc/erc-goodies.el            |  39 +++++---
 lisp/erc/erc.el                    |  34 +++++++
 test/lisp/erc/erc-goodies-tests.el | 153 ++++++++++++++++++++---------
 test/lisp/erc/erc-tests.el         |  52 ++++++++++
 5 files changed, 223 insertions(+), 58 deletions(-)

Interdiff:
diff --git a/lisp/erc/erc-goodies.el b/lisp/erc/erc-goodies.el
index 212cdbfa9ef..6e9e48e1b81 100644
--- a/lisp/erc/erc-goodies.el
+++ b/lisp/erc/erc-goodies.el
@@ -661,11 +661,13 @@ erc-italic-face
   :group 'erc-faces)
 
 (defface erc-inverse-face
-  '((t :foreground "White" :background "Black"))
+  '((((supports :inverse-video t))
+     :inverse-video t)
+    (t :foreground "White" :background "Black"))
   "ERC inverse face."
   :group 'erc-faces)
 
-(defface erc-spoiler-face '((t :inherit erc-inverse-face))
+(defface erc-spoiler-face '((t :inherit default))
   "ERC spoiler face."
   :group 'erc-faces)
 
@@ -673,6 +675,16 @@ erc-underline-face
   "ERC underline face."
   :group 'erc-faces)
 
+(defface erc-control-default-fg '((t :inherit default))
+  "ERC foreground face for the \"default\" color code."
+  :group 'erc-faces)
+
+(defface erc-control-default-bg '((t :inherit default))
+  "ERC background face for the \"default\" color code."
+  :group 'erc-faces)
+
+;; FIXME rename these to something like `erc-control-color-N-fg',
+;; and deprecate the old names via `define-obsolete-face-alias'.
 (defface fg:erc-color-face0 '((t :foreground "White"))
   "ERC face."
   :group 'erc-faces)
@@ -802,7 +814,7 @@ erc-get-bg-color-face
       (intern (concat "bg:erc-color-face" (number-to-string n))))
      ((< 15 n 99)
       (list :background (aref erc--controls-additional-colors (- n 16))))
-     (t (erc-log (format "   Wrong color: %s" n)) '(default)))))
+     (t (erc-log (format "   Wrong color: %s" n)) 'erc-control-default-fg))))
 
 (defun erc-get-fg-color-face (n)
   "Fetches the right face for foreground color N (0-15)."
@@ -818,7 +830,7 @@ erc-get-fg-color-face
       (intern (concat "fg:erc-color-face" (number-to-string n))))
      ((< 15 n 99)
       (list :foreground (aref erc--controls-additional-colors (- n 16))))
-     (t (erc-log (format "   Wrong color: %s" n)) '(default)))))
+     (t (erc-log (format "   Wrong color: %s" n)) 'erc-control-default-bg))))
 
 ;;;###autoload(autoload 'erc-irccontrols-mode "erc-goodies" nil t)
 (define-erc-module irccontrols nil
@@ -966,7 +978,7 @@ erc-controls-propertize
   "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."
-  (when (and fg bg (equal fg bg))
+  (when (and fg bg (equal fg bg) (not (equal fg "99")))
     (add-text-properties from to '( mouse-face erc-spoiler-face
                                     cursor-face erc-spoiler-face)
                          str)
diff --git a/test/lisp/erc/erc-goodies-tests.el b/test/lisp/erc/erc-goodies-tests.el
index 0ab40808a4a..c8fb0544a72 100644
--- a/test/lisp/erc/erc-goodies-tests.el
+++ b/test/lisp/erc/erc-goodies-tests.el
@@ -29,19 +29,23 @@
 (defun erc-goodies-tests--assert-face (beg end-str present &optional absent)
   (setq beg (+ beg (point-min)))
   (let ((end (+ beg (1- (length end-str)))))
-    (while (and beg (< beg end))
-      (let* ((val (get-text-property beg 'font-lock-face))
-             (ft (flatten-tree (ensure-list val))))
-        (dolist (p (ensure-list present))
-          (if (consp p)
-              (should (member p val))
-            (should (memq p ft))))
-        (dolist (a (ensure-list absent))
-          (if (consp a)
-              (should-not (member a val))
-            (should-not (memq a ft))))
-        (setq beg (text-property-not-all beg (point-max)
-                                         'font-lock-face val))))))
+    (ert-info ((format "beg: %S, end-str: %S" beg end-str))
+      (while (and beg (< beg end))
+        (let* ((val (get-text-property beg 'font-lock-face))
+               (ft (flatten-tree (ensure-list val))))
+          (ert-info ((format "looking-at: %S, val: %S"
+                             (buffer-substring-no-properties beg end)
+                             val))
+            (dolist (p (ensure-list present))
+              (if (consp p)
+                  (should (member p val))
+                (should (memq p ft))))
+            (dolist (a (ensure-list absent))
+              (if (consp a)
+                  (should-not (member a val))
+                (should-not (memq a ft)))))
+          (setq beg (text-property-not-all beg (point-max)
+                                           'font-lock-face val)))))))
 
 ;; These are from the "Examples" section of
 ;; https://modern.ircdocs.horse/formatting.html
@@ -134,30 +138,93 @@ erc-controls-highlight--spoilers
   (erc-tests-common-make-server-buf)
   (with-current-buffer (erc--open-target "#chan")
     (setq-local erc-interpret-mirc-color t)
-    (let* ((m "Spoiler: \C-c0,0Hello\C-c1,1World!")
-           (msg (erc-format-privmessage "bob" m nil t)))
+    (let* ((raw (concat "BEGIN "
+                        "\C-c0,0 WhiteOnWhite "
+                        "\C-c1,1 BlackOnBlack "
+                        "\C-c99,99 Default "
+                        "\C-o END"))
+           (msg (erc-format-privmessage "bob" raw nil t)))
       (erc-display-message nil nil (current-buffer) msg))
     (forward-line -1)
     (should (search-forward "<bob> " nil t))
     (save-restriction
       ;; Narrow to EOL or start of right-side stamp.
       (narrow-to-region (point) (line-end-position))
-      (should (eq (get-text-property (+ 9 (point)) 'mouse-face)
-                  'erc-spoiler-face))
-      (should (eq (get-text-property (1- (pos-eol)) 'mouse-face)
-                  'erc-spoiler-face))
-      ;; "Spoiler" appears in ERC default face.
+      (save-excursion
+        (search-forward "WhiteOn")
+        (should (eq (get-text-property (point) 'mouse-face)
+                    'erc-spoiler-face))
+        (search-forward "BlackOn")
+        (should (eq (get-text-property (point) 'mouse-face)
+                    'erc-spoiler-face)))
+      ;; Start wtih ERC default face.
       (erc-goodies-tests--assert-face
-       0 "Spoiler: " 'erc-default-face
+       0 "BEGIN " 'erc-default-face
        '(fg:erc-color-face0 bg:erc-color-face0))
-      ;; "Hello" is masked in all white.
+      ;; Masked in all white.
       (erc-goodies-tests--assert-face
-       9 "Hello" '(fg:erc-color-face0 bg:erc-color-face0)
+       6 "WhiteOnWhite" '(fg:erc-color-face0 bg:erc-color-face0)
        '(fg:erc-color-face1 bg:erc-color-face1))
-      ;; "World" is masked in all black.
+      ;; Masked in all black.
       (erc-goodies-tests--assert-face
-       18 " World" '(fg:erc-color-face1 bg:erc-color-face1 )
-       '(fg:erc-color-face0 bg:erc-color-face0))))
+       20 "BlackOnBlack" '(fg:erc-color-face1 bg:erc-color-face1)
+       '(erc-control-default-fg erc-control-default-bg))
+      ;; Explicit "default" code ignoerd.
+      (erc-goodies-tests--assert-face
+       34 "Default" '(erc-control-default-fg erc-control-default-bg)
+       '(fg:erc-color-face1 bg:erc-color-face1))
+      (erc-goodies-tests--assert-face
+       43 "END" 'erc-default-face
+       '(erc-control-default-bg erc-control-default-fg))))
+  (when noninteractive
+    (erc-tests-common-kill-buffers)))
+
+(ert-deftest erc-controls-highlight--inverse ()
+  (should (eq t erc-interpret-controls-p))
+  (erc-tests-common-make-server-buf)
+  (with-current-buffer (erc--open-target "#chan")
+    (setq-local erc-interpret-mirc-color t)
+    (defvar erc-fill-column)
+    (let* ((erc-fill-column 90)
+           (raw (concat "BEGIN "
+                        "\C-c3,13 GreenOnPink "
+                        "\C-v PinkOnGreen "
+                        "\C-c99,99 ReversedDefault "
+                        "\C-v NormalDefault "
+                        "\C-o END"))
+           (msg (erc-format-privmessage "bob" raw nil t)))
+      (erc-display-message nil nil (current-buffer) msg))
+    (forward-line -1)
+    (should (search-forward "<bob> " nil t))
+    (save-restriction
+      ;; Narrow to EOL or start of right-side stamp.
+      (narrow-to-region (point) (line-end-position))
+      ;; Baseline.
+      (erc-goodies-tests--assert-face
+       0 "BEGIN " 'erc-default-face
+       '(fg:erc-color-face0 bg:erc-color-face0))
+      ;; Normal fg/bg combo.
+      (erc-goodies-tests--assert-face
+       6 "GreenOnPink" '(fg:erc-color-face3 bg:erc-color-face13)
+       '(erc-inverse-face))
+      ;; Reverse of previous, so former-bg on former-fg.
+      (erc-goodies-tests--assert-face
+       19 "PinkOnGreen"
+       '(erc-inverse-face fg:erc-color-face3 bg:erc-color-face13)
+       nil)
+      ;; The inverse of `default' because reverse still in effect.
+      (erc-goodies-tests--assert-face
+       32 "ReversedDefault" '(erc-inverse-face erc-control-default-fg
+                                               erc-control-default-bg)
+       '(fg:erc-color-face3 bg:erc-color-face13))
+      (erc-goodies-tests--assert-face
+       49 "NormalDefault" '(erc-control-default-fg
+                            erc-control-default-bg)
+       '(erc-inverse-face fg:erc-color-face1 bg:erc-color-face1))
+      (erc-goodies-tests--assert-face
+       64 "END" 'erc-default-face
+       '( erc-control-default-fg erc-control-default-bg
+          fg:erc-color-face0 bg:erc-color-face0))))
   (when noninteractive
     (erc-tests-common-kill-buffers)))
 
-- 
2.44.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0001-5.6-Leverage-inverse-video-for-erc-inverse-face.patch --]
[-- Type: text/x-patch, Size: 11134 bytes --]

From 2c0b39529cfc9edd6178f4243145688b915d37c6 Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
Date: Thu, 7 Mar 2024 21:53:11 -0800
Subject: [PATCH 1/3] [5.6] Leverage inverse-video for erc-inverse-face

* lisp/erc/erc-goodies.el (erc-inverse-face): Prefer the "reversing"
effect of swapping foreground and background colors over affected
intervals.  See https://modern.ircdocs.horse/formatting#reverse-color.
(erc-control-default-fg erc-control-default-bg): New faces to allow for
customizing the look of IRC color-code number 99.  Ignore the ERC
convention of prefixing control-code-derived faces with "fg:" and
"bg:" because it doesn't comport with modern sensibilities, which
demand identifiers normally be namespaced.
(erc-get-bg-color-face, erc-get-fg-color-face): Don't wrap face in a
list, and use new, dedicated faces.
* test/lisp/erc/erc-goodies-tests.el
(erc-controls-highlight--inverse): Redo, asserting behavior described
in https://modern.ircdocs.horse/formatting#reverse-color.
(erc-controls-highlight--spoilers): New test based on the body of the
old `erc-controls-highlight--inverse', except without shadowing
`erc-insert-modify-hook' with an unrealistic, idealized value.  Adjust
expected buffer state to reflect the new role of
`erc-spoiler-face'.  (Bug#69597)
---
 lisp/erc/erc-goodies.el            |  18 +++-
 test/lisp/erc/erc-goodies-tests.el | 153 ++++++++++++++++++++---------
 2 files changed, 124 insertions(+), 47 deletions(-)

diff --git a/lisp/erc/erc-goodies.el b/lisp/erc/erc-goodies.el
index 7e30b1060fd..dbf869dafe6 100644
--- a/lisp/erc/erc-goodies.el
+++ b/lisp/erc/erc-goodies.el
@@ -661,7 +661,9 @@ erc-italic-face
   :group 'erc-faces)
 
 (defface erc-inverse-face
-  '((t :foreground "White" :background "Black"))
+  '((((supports :inverse-video t))
+     :inverse-video t)
+    (t :foreground "White" :background "Black"))
   "ERC inverse face."
   :group 'erc-faces)
 
@@ -675,6 +677,16 @@ erc-underline-face
   "ERC underline face."
   :group 'erc-faces)
 
+(defface erc-control-default-fg '((t :inherit default))
+  "ERC foreground face for the \"default\" color code."
+  :group 'erc-faces)
+
+(defface erc-control-default-bg '((t :inherit default))
+  "ERC background face for the \"default\" color code."
+  :group 'erc-faces)
+
+;; FIXME rename these to something like `erc-control-color-N-fg',
+;; and deprecate the old names via `define-obsolete-face-alias'.
 (defface fg:erc-color-face0 '((t :foreground "White"))
   "ERC face."
   :group 'erc-faces)
@@ -804,7 +816,7 @@ erc-get-bg-color-face
       (intern (concat "bg:erc-color-face" (number-to-string n))))
      ((< 15 n 99)
       (list :background (aref erc--controls-additional-colors (- n 16))))
-     (t (erc-log (format "   Wrong color: %s" n)) '(default)))))
+     (t (erc-log (format "   Wrong color: %s" n)) 'erc-control-default-fg))))
 
 (defun erc-get-fg-color-face (n)
   "Fetches the right face for foreground color N (0-15)."
@@ -820,7 +832,7 @@ erc-get-fg-color-face
       (intern (concat "fg:erc-color-face" (number-to-string n))))
      ((< 15 n 99)
       (list :foreground (aref erc--controls-additional-colors (- n 16))))
-     (t (erc-log (format "   Wrong color: %s" n)) '(default)))))
+     (t (erc-log (format "   Wrong color: %s" n)) 'erc-control-default-bg))))
 
 ;;;###autoload(autoload 'erc-irccontrols-mode "erc-goodies" nil t)
 (define-erc-module irccontrols nil
diff --git a/test/lisp/erc/erc-goodies-tests.el b/test/lisp/erc/erc-goodies-tests.el
index 7013ce0c8fc..c8fb0544a72 100644
--- a/test/lisp/erc/erc-goodies-tests.el
+++ b/test/lisp/erc/erc-goodies-tests.el
@@ -29,19 +29,23 @@
 (defun erc-goodies-tests--assert-face (beg end-str present &optional absent)
   (setq beg (+ beg (point-min)))
   (let ((end (+ beg (1- (length end-str)))))
-    (while (and beg (< beg end))
-      (let* ((val (get-text-property beg 'font-lock-face))
-             (ft (flatten-tree (ensure-list val))))
-        (dolist (p (ensure-list present))
-          (if (consp p)
-              (should (member p val))
-            (should (memq p ft))))
-        (dolist (a (ensure-list absent))
-          (if (consp a)
-              (should-not (member a val))
-            (should-not (memq a ft))))
-        (setq beg (text-property-not-all beg (point-max)
-                                         'font-lock-face val))))))
+    (ert-info ((format "beg: %S, end-str: %S" beg end-str))
+      (while (and beg (< beg end))
+        (let* ((val (get-text-property beg 'font-lock-face))
+               (ft (flatten-tree (ensure-list val))))
+          (ert-info ((format "looking-at: %S, val: %S"
+                             (buffer-substring-no-properties beg end)
+                             val))
+            (dolist (p (ensure-list present))
+              (if (consp p)
+                  (should (member p val))
+                (should (memq p ft))))
+            (dolist (a (ensure-list absent))
+              (if (consp a)
+                  (should-not (member a val))
+                (should-not (memq a ft)))))
+          (setq beg (text-property-not-all beg (point-max)
+                                           'font-lock-face val)))))))
 
 ;; These are from the "Examples" section of
 ;; https://modern.ircdocs.horse/formatting.html
@@ -129,39 +133,100 @@ erc-controls-highlight--examples
 ;; Hovering over the redacted area should reveal its underlying text
 ;; in a high-contrast face.
 
-(ert-deftest erc-controls-highlight--inverse ()
+(ert-deftest erc-controls-highlight--spoilers ()
   (should (eq t erc-interpret-controls-p))
-  (let ((erc-insert-modify-hook '(erc-controls-highlight))
-        erc-kill-channel-hook erc-kill-server-hook erc-kill-buffer-hook)
-    (with-current-buffer (get-buffer-create "#chan")
-      (erc-mode)
-      (setq-local erc-interpret-mirc-color t)
-      (erc--initialize-markers (point) nil)
+  (erc-tests-common-make-server-buf)
+  (with-current-buffer (erc--open-target "#chan")
+    (setq-local erc-interpret-mirc-color t)
+    (let* ((raw (concat "BEGIN "
+                        "\C-c0,0 WhiteOnWhite "
+                        "\C-c1,1 BlackOnBlack "
+                        "\C-c99,99 Default "
+                        "\C-o END"))
+           (msg (erc-format-privmessage "bob" raw nil t)))
+      (erc-display-message nil nil (current-buffer) msg))
+    (forward-line -1)
+    (should (search-forward "<bob> " nil t))
+    (save-restriction
+      ;; Narrow to EOL or start of right-side stamp.
+      (narrow-to-region (point) (line-end-position))
+      (save-excursion
+        (search-forward "WhiteOn")
+        (should (eq (get-text-property (point) 'mouse-face)
+                    'erc-spoiler-face))
+        (search-forward "BlackOn")
+        (should (eq (get-text-property (point) 'mouse-face)
+                    'erc-spoiler-face)))
+      ;; Start wtih ERC default face.
+      (erc-goodies-tests--assert-face
+       0 "BEGIN " 'erc-default-face
+       '(fg:erc-color-face0 bg:erc-color-face0))
+      ;; Masked in all white.
+      (erc-goodies-tests--assert-face
+       6 "WhiteOnWhite" '(fg:erc-color-face0 bg:erc-color-face0)
+       '(fg:erc-color-face1 bg:erc-color-face1))
+      ;; Masked in all black.
+      (erc-goodies-tests--assert-face
+       20 "BlackOnBlack" '(fg:erc-color-face1 bg:erc-color-face1)
+       '(erc-control-default-fg erc-control-default-bg))
+      ;; Explicit "default" code ignoerd.
+      (erc-goodies-tests--assert-face
+       34 "Default" '(erc-control-default-fg erc-control-default-bg)
+       '(fg:erc-color-face1 bg:erc-color-face1))
+      (erc-goodies-tests--assert-face
+       43 "END" 'erc-default-face
+       '(erc-control-default-bg erc-control-default-fg))))
+  (when noninteractive
+    (erc-tests-common-kill-buffers)))
 
-      (let* ((m "Spoiler: \C-c0,0Hello\C-c1,1World!")
-             (msg (erc-format-privmessage "bob" m nil t)))
-        (erc-display-message nil nil (current-buffer) msg))
-      (forward-line -1)
-      (should (search-forward "<bob> " nil t))
-      (save-restriction
-        (narrow-to-region (point) (pos-eol))
-        (should (eq (get-text-property (+ 9 (point)) 'mouse-face)
-                    'erc-inverse-face))
-        (should (eq (get-text-property (1- (pos-eol)) 'mouse-face)
-                    'erc-inverse-face))
-        (erc-goodies-tests--assert-face
-         0 "Spoiler: " 'erc-default-face
-         '(fg:erc-color-face0 bg:erc-color-face0))
-        (erc-goodies-tests--assert-face
-         9 "Hello" '(erc-spoiler-face)
-         '( fg:erc-color-face0 bg:erc-color-face0
-            fg:erc-color-face1 bg:erc-color-face1))
-        (erc-goodies-tests--assert-face
-         18 " World" '(erc-spoiler-face)
-         '( fg:erc-color-face0 bg:erc-color-face0
-            fg:erc-color-face1 bg:erc-color-face1 )))
-      (when noninteractive
-        (kill-buffer)))))
+(ert-deftest erc-controls-highlight--inverse ()
+  (should (eq t erc-interpret-controls-p))
+  (erc-tests-common-make-server-buf)
+  (with-current-buffer (erc--open-target "#chan")
+    (setq-local erc-interpret-mirc-color t)
+    (defvar erc-fill-column)
+    (let* ((erc-fill-column 90)
+           (raw (concat "BEGIN "
+                        "\C-c3,13 GreenOnPink "
+                        "\C-v PinkOnGreen "
+                        "\C-c99,99 ReversedDefault "
+                        "\C-v NormalDefault "
+                        "\C-o END"))
+           (msg (erc-format-privmessage "bob" raw nil t)))
+      (erc-display-message nil nil (current-buffer) msg))
+    (forward-line -1)
+    (should (search-forward "<bob> " nil t))
+    (save-restriction
+      ;; Narrow to EOL or start of right-side stamp.
+      (narrow-to-region (point) (line-end-position))
+      ;; Baseline.
+      (erc-goodies-tests--assert-face
+       0 "BEGIN " 'erc-default-face
+       '(fg:erc-color-face0 bg:erc-color-face0))
+      ;; Normal fg/bg combo.
+      (erc-goodies-tests--assert-face
+       6 "GreenOnPink" '(fg:erc-color-face3 bg:erc-color-face13)
+       '(erc-inverse-face))
+      ;; Reverse of previous, so former-bg on former-fg.
+      (erc-goodies-tests--assert-face
+       19 "PinkOnGreen"
+       '(erc-inverse-face fg:erc-color-face3 bg:erc-color-face13)
+       nil)
+      ;; The inverse of `default' because reverse still in effect.
+      (erc-goodies-tests--assert-face
+       32 "ReversedDefault" '(erc-inverse-face erc-control-default-fg
+                                               erc-control-default-bg)
+       '(fg:erc-color-face3 bg:erc-color-face13))
+      (erc-goodies-tests--assert-face
+       49 "NormalDefault" '(erc-control-default-fg
+                            erc-control-default-bg)
+       '(erc-inverse-face fg:erc-color-face1 bg:erc-color-face1))
+      (erc-goodies-tests--assert-face
+       64 "END" 'erc-default-face
+       '( erc-control-default-fg erc-control-default-bg
+          fg:erc-color-face0 bg:erc-color-face0))))
+  (when noninteractive
+    (erc-tests-common-kill-buffers)))
 
 (defvar erc-goodies-tests--motd
   ;; This is from ergo's MOTD
-- 
2.44.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0002-5.6-Make-important-text-props-more-resilient-in-ERC.patch --]
[-- Type: text/x-patch, Size: 6410 bytes --]

From 7b97bb8fad96fcc2fb48858018bf0e5509a6d741 Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
Date: Thu, 7 Mar 2024 21:53:23 -0800
Subject: [PATCH 2/3] [5.6] Make important text props more resilient in ERC

* lisp/erc/erc-button.el (erc-button-remove-old-buttons): Restore
original `mouse-face' values in areas marked as important after
clobbering.
* lisp/erc/erc.el (erc--reserve-important-text-props): New function.
(erc--restore-important-text-props): New function.
* test/lisp/erc/erc-tests.el
(erc--restore-important-text-props): New test.
(Bug#69597)
---
 lisp/erc/erc-button.el     |  3 ++-
 lisp/erc/erc.el            | 34 +++++++++++++++++++++++++
 test/lisp/erc/erc-tests.el | 52 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 88 insertions(+), 1 deletion(-)

diff --git a/lisp/erc/erc-button.el b/lisp/erc/erc-button.el
index 6b78e451b54..4b4930e5bff 100644
--- a/lisp/erc/erc-button.el
+++ b/lisp/erc/erc-button.el
@@ -528,7 +528,8 @@ erc-button-remove-old-buttons
    '(erc-callback nil
                   erc-data nil
                   mouse-face nil
-                  keymap nil)))
+                  keymap nil))
+  (erc--restore-important-text-props '(mouse-face)))
 
 (defun erc-button-add-button (from to fun nick-p &optional data regexp)
   "Create a button between FROM and TO with callback FUN and data DATA.
diff --git a/lisp/erc/erc.el b/lisp/erc/erc.el
index cce3b2508fb..08bc9939b9a 100644
--- a/lisp/erc/erc.el
+++ b/lisp/erc/erc.el
@@ -3532,6 +3532,40 @@ erc--remove-from-prop-value-list
             old (get-text-property pos prop object)
             end (next-single-property-change pos prop object to)))))
 
+(defun erc--reserve-important-text-props (beg end plist &optional object)
+  "Record text-property pairs in PLIST as important between BEG and END.
+Also mark the message being inserted as containing these important props
+so modules performing destructive modifications can later restore them.
+Expect to run in a narrowed buffer at message-insertion time."
+  (when erc--msg-props
+    (let ((existing (erc--check-msg-prop 'erc--important-prop-names)))
+      (puthash 'erc--important-prop-names (seq-union existing (map-keys plist))
+               erc--msg-props)))
+  (erc--merge-prop beg end 'erc--important-props plist object))
+
+(defun erc--restore-important-text-props (props &optional beg end)
+  "Restore PROPS where recorded in the accessible portion of the buffer.
+Expect to run in a narrowed buffer at message-insertion time.  Limit the
+effect to the region between buffer positions BEG and END, when non-nil.
+
+Callers should be aware that this function fails if the property
+`erc--important-props' has an empty value almost anywhere along the
+affected region.  Use the function `erc--remove-from-prop-value-list' to
+ensure that props with empty values are excised completely."
+  (when-let ((registered (erc--check-msg-prop 'erc--important-prop-names))
+             (present (seq-intersection props registered))
+             (b (or beg (point-min)))
+             (e (or end (point-max))))
+    (while-let
+        (((setq b (text-property-not-all b e 'erc--important-props nil)))
+         (val (get-text-property b 'erc--important-props))
+         (q (next-single-property-change b 'erc--important-props nil e)))
+      (while-let ((k (pop val))
+                  (v (pop val)))
+        (when (memq k present)
+          (put-text-property b q k v)))
+      (setq b q))))
+
 (defvar erc-legacy-invisible-bounds-p nil
   "Whether to hide trailing rather than preceding newlines.
 Beginning in ERC 5.6, invisibility extends from a message's
diff --git a/test/lisp/erc/erc-tests.el b/test/lisp/erc/erc-tests.el
index 085b063bdb2..6809d9db41d 100644
--- a/test/lisp/erc/erc-tests.el
+++ b/test/lisp/erc/erc-tests.el
@@ -2232,6 +2232,58 @@ erc--remove-from-prop-value-list/many
     (when noninteractive
       (kill-buffer))))
 
+(ert-deftest erc--restore-important-text-props ()
+  (erc-mode)
+  (let ((erc--msg-props (map-into '((erc--important-prop-names a))
+                                  'hash-table)))
+    (insert (propertize "foo" 'a 'A 'b 'B 'erc--important-props '(a A))
+            " "
+            (propertize "bar" 'c 'C 'a 'A 'b 'B
+                        'erc--important-props '(a A c C)))
+
+    ;; Attempt to restore a and c when only a is registered.
+    (remove-list-of-text-properties (point-min) (point-max) '(a c))
+    (erc--restore-important-text-props '(a c))
+    (should (erc-tests-common-equal-with-props
+             (buffer-string)
+             #("foo bar"
+               0 3 (a A b B erc--important-props (a A))
+               4 7 (a A b B erc--important-props (a A c C)))))
+
+    ;; Add d between 3 and 6.
+    (erc--reserve-important-text-props 3 6 '(d D))
+    (put-text-property 3 6 'd 'D)
+    (should (erc-tests-common-equal-with-props
+             (buffer-string)
+             #("foo bar" ; #1
+               0 2 (a A b B erc--important-props (a A))
+               2 3 (d D a A b B erc--important-props (d D a A))
+               3 4 (d D erc--important-props (d D))
+               4 5 (d D a A b B erc--important-props (d D a A c C))
+               5 7 (a A b B erc--important-props (a A c C)))))
+    ;; Remove a and d, and attempt to restore d.
+    (remove-list-of-text-properties (point-min) (point-max) '(a d))
+    (erc--restore-important-text-props '(d))
+    (should (erc-tests-common-equal-with-props
+             (buffer-string)
+             #("foo bar"
+               0 2 (b B erc--important-props (a A))
+               2 3 (d D b B erc--important-props (d D a A))
+               3 4 (d D erc--important-props (d D))
+               4 5 (d D b B erc--important-props (d D a A c C))
+               5 7 (b B erc--important-props (a A c C)))))
+
+    ;; Restore a only.
+    (erc--restore-important-text-props '(a))
+    (should (erc-tests-common-equal-with-props
+             (buffer-string)
+             #("foo bar" ; same as #1 above
+               0 2 (a A b B erc--important-props (a A))
+               2 3 (d D a A b B erc--important-props (d D a A))
+               3 4 (d D erc--important-props (d D))
+               4 5 (d D a A b B erc--important-props (d D a A c C))
+               5 7 (a A b B erc--important-props (a A c C)))))))
+
 (ert-deftest erc--split-string-shell-cmd ()
 
   ;; Leading and trailing space
-- 
2.44.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: 0003-5.6-Redefine-erc-spoiler-face-to-indicate-revealed-t.patch --]
[-- Type: text/x-patch, Size: 2344 bytes --]

From d2ad575e8935981c23846bd54a3eac29b9290f45 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 3/3] [5.6] Redefine erc-spoiler-face to indicate revealed text

* lisp/erc/erc-goodies.el (erc-spoiler-face): Redefine role and redo
definition to inherit from `erc-control-default-face'.
(erc-controls-propertize): Include `cursor-face' in the applied hover
properties for spoiler text, and ensure they aren't clobbered by other
built-in modules, like `button'.
(Bug#69597)

Copyright-paperwork-exempt: yes
---
 lisp/erc/erc-goodies.el | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/lisp/erc/erc-goodies.el b/lisp/erc/erc-goodies.el
index dbf869dafe6..6e9e48e1b81 100644
--- a/lisp/erc/erc-goodies.el
+++ b/lisp/erc/erc-goodies.el
@@ -667,9 +667,7 @@ erc-inverse-face
   "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 default))
   "ERC spoiler face."
   :group 'erc-faces)
 
@@ -980,13 +978,16 @@ erc-controls-propertize
   "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) (not (equal fg "99")))
+    (add-text-properties from to '( mouse-face erc-spoiler-face
+                                    cursor-face erc-spoiler-face)
+                         str)
+    (erc--reserve-important-text-props from to
+                                       '( mouse-face erc-spoiler-face
+                                          cursor-face erc-spoiler-face)
+                                       str))
+  (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
-- 
2.44.0

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

[Fadi Moukayed]

[-- Attachment #1: Type: text/plain, Size: 2624 bytes --]

>Really appreciate the thorough testing -- and your patience even more so
>because as much as I'd like to put a bow on this, it turns out (sigh)
>there's one lingering matter yet unresolved.

*Ahem* I apologize for opening this can of worms really. Didn't quite
expect a simple question regarding spoiler rendering to result in this
amount of Erc hackery :)

> [1] https://modern.ircdocs.horse/formatting#reverse-color

Good call. I am aware of this reference (as any IRC tinkerer should
be), and indeed, Erc should handle ^V and ^C99,99 [1], even though
both are "Not universally supported".

I gave the patches another look and did another clean re-apply for an
additional test. All OK (results seen in the attached screenshot).
Implementing ^V using :inverse-video should be okay, although I'm not
sure about the implications on platforms that do not support that (do
any exist? documentation here is sparse, unfortunately). I assume it
will fall back to the hard-coded white-on-black color specification in
that case, which is probably an acceptable compromise.

Cheers,
FM

[1] subtle edge case which shouldn't be formatted as a spoiler, even
though X=Y=99 in ^CX,Y (Again, good catch, thanks for that)

Am Sa., 9. März 2024 um 17:06 Uhr schrieb J.P. <jp@neverwas.me>:
>
> Fadi Moukayed <smfadi@gmail.com> writes:
>
> > That said, I'd like to +1 the changeset and confirm that the proposed
> > changes apply cleanly, and yield the desired result. I tested inputs
> > of the form ^CX,X<text>^C on a temporary private channel – spoilers
> > are formatted and revealed as intended, and no regressions were
> > observed. Very nice. Would be a neat addition/fix for the next Erc
> > release.
>
> Really appreciate the thorough testing -- and your patience even more so
> because as much as I'd like to put a bow on this, it turns out (sigh)
> there's one lingering matter yet unresolved.
>
> Alas, looking more closely at how `erc-controls-propertize' treats
> `erc-inverse-face' (crucially, as a modifying toggle [1]), I've quickly
> come to rue the day I ever thought to suggest otherwise, especially in
> drawing misguided associations with `erc-spoiler-face'. (Indeed, my
> quasi-conflating the two was what led us astray to begin with.) So, if
> not already clear, I now believe we should just treat `erc-spoiler-face'
> as its own concern entirely and not have it inherit from
> `erc-inverse-face'. All this to say: yet another revision attached.
>
> Thanks, and apologies for the head fake.
>
> [1] https://modern.ircdocs.horse/formatting#reverse-color
>
>

[-- Attachment #2: erc-spoilers.png --]
[-- Type: image/png, Size: 13029 bytes --]

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:

> I gave the patches another look and did another clean re-apply for an
> additional test. All OK (results seen in the attached screenshot).
> Implementing ^V using :inverse-video should be okay, although I'm not
> sure about the implications on platforms that do not support that (do
> any exist? documentation here is sparse, unfortunately). I assume it
> will fall back to the hard-coded white-on-black color specification in
> that case, which is probably an acceptable compromise.

This has been installed as

  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=166c8a98

Thanks and closing.

                              * * *

A tangentially related note regarding this area of the code base: As
mentioned at least once in code comments, we should probably eventually
begin phasing out the existing face names in favor of ones that abide by
Emacs naming conventions:

- Like all identifiers, they should be properly namespaced, so no
  leading "fg:" and "bg:"

- They should _not_ end in "-face" (info "(elisp) Defining Faces")

I don't really have the bandwidth for dealing with this anytime soon,
even though it's a relatively straightforward, mostly mechanical change.
Just thought I'd note it here for the record in case someone reading
this takes a fancy.