bug#59805: 28.2; erc-track: handle faces modified with erc-button-add-face

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

Hi Nacho,

Nacho Barrientos <nacho.barrientos@cern.ch> writes:

> Hi,
>
> Some packages like erc-hl-nicks [0] use `erc-button-add-face' to add extra
> faces to some strings, notably nicknames.

Thanks for submitting this proposal. I have yet to form any opinions
about it but promise to eventually. In the meantime, I'll mention some
broadly related observations I happened to make while looking at it
briefly. Some are likely of less interest to you, but I'll state them
here anyway for lack of a better venue.

The first is that `erc-button-add-face' is similar in some respects to
`font-lock-prepend-text-property', which first appeared in Emacs 27. Its
behavior may provide some insights into how users and libraries expect
new faces to be composed with existing ones.

While the effects produced by both functions are visually similar, the
structures returned sometimes differ. The font-lock version also takes
some added pains regarding compatibility, but that's likely of little
consequence for our purposes. What does concern us is that the font-lock
version flattens as it goes at the cost of preserving a span's lineage,
which may not be a bad thing when it comes to searching (so long as
third-party code isn't too affected). For example, if an existing
message contains spans of property values like

  a (b c) ((d e) f)

adding a new value, x, produces

  (x a) (x b c) (x (d e) f)

with both functions. But the two differ when we add a composite face,
like (x y). Compare

  (x y a) (x y b c) (x y (d e) f)

from `font-lock-prepend-text-property', vs

  ((x y) a) ((x y) b c) ((x y) (d e) f)

from `erc-button-add-face'. Not sure of the implications of
"pre-flattening", but I think it's maybe worth a think.

As a side note, while looking at `erc-button-add-face', I noticed that a
utility function it calls, `erc-list', was added in Emacs 28 as
`ensure-list', which one of our dependencies, Compat 28, also provides.
So, I think it's probably safe to do something like this in ERC 5.6:

  diff --git a/lisp/erc/erc.el b/lisp/erc/erc.el
  index 268d83dc44..ca349685c2 100644
  --- a/lisp/erc/erc.el
  +++ b/lisp/erc/erc.el
  @@ -5631,11 +5631,7 @@ erc-put-text-property
   EmacsSpeak support."
     (put-text-property start end property value object))

  -(defun erc-list (thing)
  -  "Return THING if THING is a list, or a list with THING as its element."
  -  (if (listp thing)
  -      thing
  -    (list thing)))
  +(defalias 'erc-list 'ensure-list)

   (defun erc-parse-user (string)
     "Parse STRING as a user specification (nick!login@host).

> For instance, on a coloured current nickname mention for the nick
> 'nacho' (current nick), `erc-faces-in' would return:
>
> λ> (erc-faces-in (buffer-substring 348 349))
> ((erc-hl-nicks-nick-nacho-face erc-current-nick-face))
>
> instead of:
>
> λ> (erc-faces-in (buffer-substring 323 324))
> (erc-current-nick-face)
>
> when `erc-hl-nicks-mode` is not enabled. Note the nesting.
>
> This is problematic because `erc-track-modified-channels` does this
> comparison:
>
> ...
> (let ((faces (erc-faces-in (buffer-string))))
> ...
> 		   (not (catch 'found
> 			  (dolist (f faces)
> 			    (when (member f erc-track-faces-priority-list)
> 			      (throw 'found t))))))
> ...

Random observation: it seems `erc-faces-in' reverses the initial order
of property occurrences WRT `point'. Not sure what that means, if
anything, but it may be worth noting that the snippet above visits them
in reverse order as a result.

> failing to detect that `erc-current-nick-face' is indeed active and,
> assuming that `erc-current-nick-face' is member of
> `erc-track-faces-priority-list', therefore failing to add the
> buffer modified flag to the modeline.

Looks like the default value of `erc-track-faces-priority-list' contains
some composite faces, such as

  (erc-nick-default-face erc-current-nick-face)

So whatever happens, I think we'll have to preserve compatibility for
people already used to searching for such composites.

In fact, have you tried adding

  (erc-hl-nicks-nick-nacho-face erc-current-nick-face)

to `erc-track-faces-priority-list' while also overriding the function
`erc-hl-nicks-face-name' with something like this?

  (lambda (nick) (intern (concat "erc-hl-nicks-nick-" nick "-face")))

If that shows any promise, it could probably only ever manifest as a
user option for a select subset of declared nicks, so as not to inundate
the global obarray with ERC spam.

> I'm not an expert in this package so perhaps erc-hl-nicks shouldn't be
> doing this at all to add the extra faces to colour nicks. However, this
> situation can be easily addressed from the erc-track side by flattening
> the list that `erc-faces-in' returns as the attached patch (against
> current master) suggests. Hope that the modifications that I've done to
> the test help clarifying even more the issue.
>
> With the patch applied, the example call to `erc-faces-in' would return:
>
> λ> (erc-faces-in (buffer-substring 348 349))
> (erc-hl-nicks-nick-nacho-face erc-current-nick-face)
>
> and erc-track would work as expected when `erc-hl-nicks-mode` is
> enabled.
>
> Thanks.
>
> [0] https://github.com/leathekd/erc-hl-nicks
>
>> From d9573f9346e8af7be8d853503c0cbe10ec89d274 Mon Sep 17 00:00:00 2001
> From: Nacho Barrientos <nacho.barrientos@cern.ch>
> Date: Sat, 3 Dec 2022 13:35:00 +0100
> Subject: [PATCH] ERC: Track: Handle face text properties that are lists
>
> ---
>  lisp/erc/erc-track.el            | 2 +-
>  test/lisp/erc/erc-track-tests.el | 8 ++++++--
>  2 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/lisp/erc/erc-track.el b/lisp/erc/erc-track.el
> index ef9a8c243e9..4832926c4b2 100644
> --- a/lisp/erc/erc-track.el
> +++ b/lisp/erc/erc-track.el
> @@ -847,7 +847,7 @@ erc-faces-in
>        (and (setq cur (get-text-property i 'face str))
>  	   (not (member cur faces))
>  	   (push cur faces)))
> -    faces))
> +    (flatten-list faces)))

I'm not sure what to think about this quite yet. Perhaps Olivier Certner
(Cc'd), who has hacked on related areas of this file relatively
recently, has some thoughts. BTW, this would also flatten anonymous
faces (property lists), I think. Probably not a deal breaker, but anons
referencing named faces, like (:inherit erc-notice-face ...), might
influence search results and so should probably be culled. FWIW, it
looks like `font-lock--add-text-property' uses (keywordp (car value)) to
detect these.

>  ;;; Buffer switching
>  
> diff --git a/test/lisp/erc/erc-track-tests.el b/test/lisp/erc/erc-track-tests.el
> index 475a436bb1d..1e0409e9df2 100644
> --- a/test/lisp/erc/erc-track-tests.el
> +++ b/test/lisp/erc/erc-track-tests.el
> @@ -116,8 +116,12 @@ erc-track--erc-faces-in
>      (put-text-property 3 (length str0) 'font-lock-face
>                         '(bold erc-current-nick-face) str0)
>      (put-text-property 3 (length str1) 'face
> -                       '(bold erc-current-nick-face) str1)
> +                       'bold str1)
>      (should (erc-faces-in str0))
> -    (should (erc-faces-in str1)) ))
> +    (should (length= (erc-faces-in str0) 2))
> +    (should (equal (erc-faces-in str0) '(bold erc-current-nick-face)))
> +    (should (erc-faces-in str1))
> +    (should (length= (erc-faces-in str1) 1))
> +    (should (equal (erc-faces-in str1) '(bold))) ))

Just noticed a semi-flagrant bug here (not yours).

I'm seeing that `font-lock-default-function' (at least these days) adds
buffer-local variables to the current buffer, making this test a
polluter. To better respect the commons, we probably ought to do
something more like this:

  (ert-deftest erc-track--erc-faces-in ()
    "`erc-faces-in' should pick up both 'face and 'font-lock-face properties."
    (let ((str0 (concat "in " (propertize "bold" 'font-lock-face
                                          '(bold erc-current-nick-face))))
          (str1 (concat "in " (propertize "bold" 'font-lock-face
                                          'bold))))
      ;; Turn on Font Lock mode: this initialize `char-property-alias-alist'
      ;; to '((face font-lock-face)).  Note that `font-lock-mode' don't
      ;; turn on the mode if the test is run on batch mode or if the
      ;; buffer name starts with ?\s (Bug#23954).
      (with-current-buffer (get-buffer-create "*erc-track--erc-faces-in*")
        (font-lock-default-function 1) ; set buffer-local variables
        (insert str0 "\n" str1 "\n")   ; insert for debugging

        (should (equal (erc-faces-in str0) '(bold erc-current-nick-face)))
        (should (equal (erc-faces-in str1) '(bold))))

      (when noninteractive
        (kill-buffer))))

>  ;;; erc-track-tests.el ends here

Taking the long view for a sec, I think we may eventually need to invest
in a revamped "view component" that's been rewritten from the ground up.
Modern extensions, such as those introduced by IRCv3, depend on fast
access to message and user data. And I'm not sure storing everything in
text properties is suitable for that. However, that's ultimately
unrelated to this proposal, in practical terms, so I'll abstain from
mentioning it again in this thread.

Overall, it's nice to know people are looking out for this corner of
ERC. Thanks again for submitting this. I hope others will chime in as
well.

J.P.