bug#64301: 30.0.50; ERC 5.6: Make speaker labels easier to work with

["J.P." <jp@neverwas.me>]

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

Some functionality I introduced for wrangling text properties in
erc-match.el was shoddily done and not very usable from the perspective
of another module building on erc-match as a foundation. The attached
patch attempts to correct that. It also ditches "erc-" namespacing for
invisibility spec members because the potential for collision seems
pretty low given that we're only talking ERC buffers. (Hopefully, I'm
not missing any downsides here.)



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-5.6-Don-t-bother-namespacing-ERC-s-own-invisibility-.patch --]
[-- Type: text/x-patch, Size: 16630 bytes --]

From 9fe0a765eba8105ef616280487882247834c299a Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
Date: Fri, 14 Jul 2023 21:08:31 -0700
Subject: [PATCH] [5.6] Don't bother namespacing ERC's own invisibility props

* lisp/erc/erc-fill.el (erc-fill-wrap-mode, erc-fill-wrap-enable):
Update name of internal flag for hiding messages.
* lisp/erc/erc-match.el (erc-match--hide-fools-offset-bounds,
erc-match--offset-invisible-bounds-p): Rename internal variable from
former to latter.
(erc-match--hide-fools-offset-bounds): Call `erc-match--hide-message'
with own `invisible' property value specifically for fools.
Specifically, use `match-fools' rather than `erc-match' or
`erc-match-fools' to save room when visually inspecting.  This retains
the module name as a prefix to hopefully minimize collisions with
invisibility spec members owned by non-ERC minor modes.
(erc-match--hide-message): Define property parameter instead of
hard-coding to `erc-match'.
(erc-match--modify-invisibility-spec): Use toggle command
non-interactively for adding and removing invisibility spec member.
(erc-match-toggle-hidden-fools): Add explicit override argument and
defer to general helper for actually modifying spec.
(erc-match--toggle-hidden): New helper for toggling invisibility
spec.
* lisp/erc/erc.el (erc--merge-prop): If new value is a list, prepend
onto existing.
* test/lisp/erc/erc-scenarios-match.el:
(erc-scenarios-match--invisible-stamp): Fix comment.
(erc-scenarios-match--find-eol): Fix bug affecting interactive use.
(erc-scenarios-match--stamp-left-fools-invisible,
erc-scenarios-match--stamp-right-fools-invisible,
erc-scenarios-match--stamp-right-invisible-fill-wrap,
erc-scenarios-match--stamp-both-invisible-fill-static): Update
`invisible' property from `erc-match' to `match-fools'.
* test/lisp/erc/erc-tests.el (erc-tests--equal-including-properties):
Relocate macro in same file.
(erc--merge-prop): New test.  (Bug#64301)
---
 lisp/erc/erc-fill.el                 |  4 +--
 lisp/erc/erc-match.el                | 43 +++++++++++++++--------
 lisp/erc/erc.el                      |  9 +++--
 test/lisp/erc/erc-scenarios-match.el | 51 ++++++++++++++++------------
 test/lisp/erc/erc-tests.el           | 50 +++++++++++++++++++++++----
 5 files changed, 110 insertions(+), 47 deletions(-)

diff --git a/lisp/erc/erc-fill.el b/lisp/erc/erc-fill.el
index a65c95f1d85..2dfae35b871 100644
--- a/lisp/erc/erc-fill.el
+++ b/lisp/erc/erc-fill.el
@@ -290,7 +290,7 @@ erc-fill-wrap-mode-map
 
 (defvar erc-match-mode)
 (defvar erc-button-mode)
-(defvar erc-match--hide-fools-offset-bounds)
+(defvar erc-match--offset-invisible-bounds-p)
 
 (defun erc-fill--wrap-ensure-dependencies ()
   (let (missing-deps)
@@ -340,7 +340,7 @@ fill-wrap
      (erc-stamp--display-margin-mode +1))
    (when (or (bound-and-true-p erc-match-mode) (memq 'match erc-modules))
      (require 'erc-match)
-     (setq erc-match--hide-fools-offset-bounds t))
+     (setq erc-match--offset-invisible-bounds-p t))
    (when erc-fill-wrap-merge
      (add-hook 'erc-button--prev-next-predicate-functions
                #'erc-fill--wrap-merged-button-p nil t))
diff --git a/lisp/erc/erc-match.el b/lisp/erc/erc-match.el
index a5b0af41b2a..3fba35536b3 100644
--- a/lisp/erc/erc-match.el
+++ b/lisp/erc/erc-match.el
@@ -655,24 +655,25 @@ erc-go-to-log-matches-buffer
 					(get-buffer (car buffer-cons))))))
     (switch-to-buffer buffer-name)))
 
-(defvar-local erc-match--hide-fools-offset-bounds nil)
+(defvar-local erc-match--offset-invisible-bounds-p nil
+  "Whether to hide a message from its preceding newline.")
 
 (defun erc-hide-fools (match-type _nickuserhost _message)
   "Hide comments from designated fools."
   (when (eq match-type 'fool)
-    (erc-match--hide-message)))
+    (erc-match--hide-message 'match-fools)))
 
-(defun erc-match--hide-message ()
+(defun erc-match--hide-message (prop)
   (progn ; FIXME raise sexp
-    (if erc-match--hide-fools-offset-bounds
+    (if erc-match--offset-invisible-bounds-p
         (let ((beg (point-min))
               (end (point-max)))
           (save-restriction
             (widen)
-            (erc--merge-prop (1- beg) (1- end) 'invisible 'erc-match)))
+            (erc--merge-prop (1- beg) (1- end) 'invisible prop)))
       ;; Before ERC 5.6, this also used to add an `intangible'
       ;; property, but the docs say it's now obsolete.
-      (erc--merge-prop (point-min) (point-max) 'invisible 'erc-match))))
+      (erc--merge-prop (point-min) (point-max) 'invisible prop))))
 
 (defun erc-beep-on-match (match-type _nickuserhost _message)
   "Beep when text matches.
@@ -682,19 +683,31 @@ erc-beep-on-match
 
 (defun erc-match--modify-invisibility-spec ()
   "Add an `erc-match' property to the local spec."
+  ;; Hopefully, this will be extended to do the same for other
+  ;; invisible properties managed by this module.
   (if erc-match-mode
-      (add-to-invisibility-spec 'erc-match)
+      (erc-match-toggle-hidden-fools +1)
     (erc-with-all-buffers-of-server nil nil
-      (remove-from-invisibility-spec 'erc-match))))
+      (erc-match-toggle-hidden-fools -1))))
 
-(defun erc-match-toggle-hidden-fools ()
+(defun erc-match-toggle-hidden-fools (arg)
   "Toggle fool visibility.
-Expect `erc-hide-fools' or a function that does something similar
-to be in `erc-text-matched-hook'."
-  (interactive)
-  (if (memq 'erc-match (ensure-list buffer-invisibility-spec))
-      (remove-from-invisibility-spec 'erc-match)
-    (add-to-invisibility-spec 'erc-match)))
+Expect the function `erc-hide-fools' or similar to be present in
+`erc-text-matched-hook'."
+  (interactive "P")
+  (erc-match--toggle-hidden 'match-fools arg))
+
+(defun erc-match--toggle-hidden (prop arg)
+  "Toggle invisibility for spec member PROP.
+Treat ARG in a manner similar to mode toggles defined by
+`define-minor-mode'."
+  (when arg
+    (setq arg (prefix-numeric-value arg)))
+  (if (memq prop (ensure-list buffer-invisibility-spec))
+      (unless (natnump arg)
+        (remove-from-invisibility-spec prop))
+    (when (or (not arg) (natnump arg))
+      (add-to-invisibility-spec prop))))
 
 (provide 'erc-match)
 
diff --git a/lisp/erc/erc.el b/lisp/erc/erc.el
index 03c21059a92..6ed919c91c2 100644
--- a/lisp/erc/erc.el
+++ b/lisp/erc/erc.el
@@ -3011,13 +3011,18 @@ erc--merge-prop
   "Compose existing PROP values with VAL between FROM and TO in OBJECT.
 For spans where PROP is non-nil, cons VAL onto the existing
 value, ensuring a proper list.  Otherwise, just set PROP to VAL.
-See also `erc-button-add-face'."
+When VAL is itself a list, prepend its members onto an existing
+value.  See also `erc-button-add-face'."
   (let ((old (get-text-property from prop object))
         (pos from)
         (end (next-single-property-change from prop object to))
         new)
     (while (< pos to)
-      (setq new (if old (cons val (ensure-list old)) val))
+      (setq new (if old
+                    (if (listp val)
+                        (append val (ensure-list old))
+                      (cons val (ensure-list old)))
+                  val))
       (put-text-property pos end prop new object)
       (setq pos end
             old (get-text-property pos prop object)
diff --git a/test/lisp/erc/erc-scenarios-match.el b/test/lisp/erc/erc-scenarios-match.el
index 8a718962c55..a438635960e 100644
--- a/test/lisp/erc/erc-scenarios-match.el
+++ b/test/lisp/erc/erc-scenarios-match.el
@@ -62,11 +62,15 @@ erc-scenarios-match--stamp-left-current-nick
                     'erc-current-nick-face))))))
 
 ;; When hacking on tests that use this fixture, it's best to run it
-;; interactively, and check for wierdness before and after doing
-;; M-: (remove-from-invisibility-spec 'erc-match) RET.
+;; interactively, and visually inspect the output with various
+;; combinations of:
+;;
+;;   M-x erc-match-toggle-hidden-fools RET
+;;   M-x erc-toggle-timestamps RET
+;;
 (defun erc-scenarios-match--invisible-stamp (hiddenp visiblep)
   (unless noninteractive
-    (kill-new "(remove-from-invisibility-spec 'erc-match)"))
+    (kill-new "erc-match-toggle-hidden-fools"))
 
   (erc-scenarios-common-with-cleanup
       ((erc-scenarios-common-dialog "join/legacy")
@@ -128,11 +132,11 @@ erc-scenarios-match--stamp-left-fools-invisible
 
        ;; Leading stamp has combined `invisible' property value.
        (should (equal (get-text-property (pos-bol) 'invisible)
-                      '(timestamp erc-match)))
+                      '(timestamp match-fools)))
 
-       ;; Message proper has the `invisible' property `erc-match'.
+       ;; Message proper has the `invisible' property `match-fools'.
        (let ((msg-beg (next-single-property-change (pos-bol) 'invisible)))
-         (should (eq (get-text-property msg-beg 'invisible) 'erc-match))
+         (should (eq (get-text-property msg-beg 'invisible) 'match-fools))
          (should (>= (next-single-property-change msg-beg 'invisible nil)
                      (pos-eol)))))
 
@@ -149,7 +153,10 @@ erc-scenarios-match--stamp-left-fools-invisible
 
 (defun erc-scenarios-match--find-eol ()
   (save-excursion
-    (goto-char (next-single-property-change (point) 'erc-command))
+    (if-let ((next (next-single-property-change (point) 'erc-command)))
+        (goto-char next)
+      ;; We're already at the end of the message.
+      (should (get-text-property (1- (point)) 'erc-command)))
     (pos-eol)))
 
 ;; In most cases, `erc-hide-fools' makes line endings invisible.
@@ -168,19 +175,19 @@ erc-scenarios-match--stamp-right-fools-invisible
 
          ;; Stamps have a combined `invisible' property value.
          (should (equal (get-text-property (1- end) 'invisible)
-                        '(timestamp erc-match)))
+                        '(timestamp match-fools)))
 
          ;; The final newline is hidden by `match', not `stamps'
-         (should (equal (get-text-property end 'invisible) 'erc-match))
+         (should (equal (get-text-property end 'invisible) 'match-fools))
 
-         ;; The message proper has the `invisible' property `erc-match',
+         ;; The message proper has the `invisible' property `match-fools',
          ;; and it starts after the preceding newline.
-         (should (eq (get-text-property (pos-bol) 'invisible) 'erc-match))
+         (should (eq (get-text-property (pos-bol) 'invisible) 'match-fools))
 
          ;; It ends just before the timestamp.
          (let ((msg-end (next-single-property-change (pos-bol) 'invisible)))
            (should (equal (get-text-property msg-end 'invisible)
-                          '(timestamp erc-match)))
+                          '(timestamp match-fools)))
 
            ;; Stamp's `invisible' property extends throughout the stamp
            ;; and ends before the trailing newline.
@@ -215,16 +222,16 @@ erc-scenarios-match--stamp-right-invisible-fill-wrap
 
        ;; Stamps have a combined `invisible' property value.
        (should (equal (get-text-property (1- (pos-eol)) 'invisible)
-                      '(timestamp erc-match)))
+                      '(timestamp match-fools)))
 
-       ;; The message proper has the `invisible' property `erc-match',
+       ;; The message proper has the `invisible' property `match-fools',
        ;; which starts at the preceding newline...
-       (should (eq (get-text-property (1- (pos-bol)) 'invisible) 'erc-match))
+       (should (eq (get-text-property (1- (pos-bol)) 'invisible) 'match-fools))
 
        ;; ... and ends just before the timestamp.
        (let ((msgend (next-single-property-change (1- (pos-bol)) 'invisible)))
          (should (equal (get-text-property msgend 'invisible)
-                        '(timestamp erc-match)))
+                        '(timestamp match-fools)))
 
          ;; The newline before `erc-insert-marker' is still visible.
          (should-not (get-text-property (pos-eol) 'invisible))
@@ -265,8 +272,8 @@ erc-scenarios-match--stamp-both-invisible-fill-static
              (search-forward "[23:59]"))))
 
        (ert-info ("Line endings in Bob's messages are invisible")
-         ;; The message proper has the `invisible' property `erc-match'.
-         (should (eq (get-text-property (pos-bol) 'invisible) 'erc-match))
+         ;; The message proper has the `invisible' property `match-fools'.
+         (should (eq (get-text-property (pos-bol) 'invisible) 'match-fools))
          (let* ((mbeg (next-single-property-change (pos-bol) 'erc-command))
                 (mend (next-single-property-change mbeg 'erc-command)))
 
@@ -283,9 +290,9 @@ erc-scenarios-match--stamp-both-invisible-fill-static
            (should (= (next-single-property-change (pos-bol) 'erc-timestamp)
                       mend))
 
-           ;; Line ending has the `invisible' property `erc-match'.
+           ;; Line ending has the `invisible' property `match-fools'.
            (should (= (char-after mend) ?\n))
-           (should (eq (get-text-property mend'invisible) 'erc-match))))
+           (should (eq (get-text-property mend'invisible) 'match-fools))))
 
        ;; Only the message right after Alice speaks contains stamps.
        (when (= 1 bob-utterance-counter)
@@ -298,7 +305,7 @@ erc-scenarios-match--stamp-both-invisible-fill-static
              ;; Date stamp has a combined `invisible' property value
              ;; that extends until the start of the message proper.
              (should (equal (get-text-property (point) 'invisible)
-                            '(timestamp erc-match)))
+                            '(timestamp match-fools)))
              (should (= (next-single-property-change (point) 'invisible)
                         (1+ (pos-eol))))))
 
@@ -314,7 +321,7 @@ erc-scenarios-match--stamp-both-invisible-fill-static
            (let ((msgend (next-single-property-change (pos-bol) 'invisible)))
              ;; Stamp has a combined `invisible' property value.
              (should (equal (get-text-property msgend 'invisible)
-                            '(timestamp erc-match)))
+                            '(timestamp match-fools)))
 
              ;; Combined `invisible' property spans entire timestamp.
              (should (= (next-single-property-change msgend 'invisible)
diff --git a/test/lisp/erc/erc-tests.el b/test/lisp/erc/erc-tests.el
index b5db5fe8764..610fb149a94 100644
--- a/test/lisp/erc/erc-tests.el
+++ b/test/lisp/erc/erc-tests.el
@@ -1272,6 +1272,50 @@ erc-process-input-line
 
           (should-not calls))))))
 
+(defmacro erc-tests--equal-including-properties (a b)
+  (list (if (< emacs-major-version 29)
+            'ert-equal-including-properties
+          'equal-including-properties)
+        a b))
+
+(ert-deftest erc--merge-prop ()
+  (with-current-buffer (get-buffer-create "*erc-test*")
+    ;; Baseline.
+    (insert "abc\n")
+    (erc--merge-prop 1 3 'erc-test 'x)
+    (should (erc-tests--equal-including-properties
+             (buffer-substring 1 4) #("abc" 0 2 (erc-test x))))
+    (erc--merge-prop 1 3 'erc-test 'y)
+    (should (erc-tests--equal-including-properties
+             (buffer-substring 1 4) #("abc" 0 2 (erc-test (y x)))))
+
+    ;; Multiple intervals.
+    (goto-char (point-min))
+    (insert "def\n")
+    (erc--merge-prop 1 2 'erc-test 'x)
+    (erc--merge-prop 2 3 'erc-test 'y)
+    (should (erc-tests--equal-including-properties
+             (buffer-substring 1 4)
+             #("def" 0 1 (erc-test x) 1 2 (erc-test y))))
+    (erc--merge-prop 1 3 'erc-test 'z)
+    (should (erc-tests--equal-including-properties
+             (buffer-substring 1 4)
+             #("def" 0 1 (erc-test (z x)) 1 2 (erc-test (z y)))))
+
+    ;; New val as list.
+    (goto-char (point-min))
+    (insert "ghi\n")
+    (erc--merge-prop 2 3 'erc-test '(y z))
+    (should (erc-tests--equal-including-properties
+             (buffer-substring 1 4) #("ghi" 1 2 (erc-test (y z)))))
+    (erc--merge-prop 1 3 'erc-test '(w x))
+    (should (erc-tests--equal-including-properties
+             (buffer-substring 1 4)
+             #("ghi" 0 1 (erc-test (w x)) 1 2 (erc-test (w x y z)))))
+
+    (when noninteractive
+      (kill-buffer))))
+
 (ert-deftest erc--split-string-shell-cmd ()
 
   ;; Leading and trailing space
@@ -1488,12 +1532,6 @@ erc-message
     (kill-buffer "ExampleNet")
     (kill-buffer "#chan")))
 
-(defmacro erc-tests--equal-including-properties (a b)
-  (list (if (< emacs-major-version 29)
-            'ert-equal-including-properties
-          'equal-including-properties)
-        a b))
-
 (ert-deftest erc-format-privmessage ()
   ;; Basic PRIVMSG
   (should (erc-tests--equal-including-properties
-- 
2.41.0