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

From: "J.P." <[email-masked]>
Date: Fri, 08 Mar 2024 07:05:00 -0800

Fadi Moukayed <[email-masked]> 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" <[email-masked]>
> 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

>From f5473bd8c8fba7c5685f1a4cadb6e6f3eb9c6f27 Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <[email-masked]>
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

>From a66abc007c071f224b459cffdc2a451e36a80903 Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <[email-masked]>
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