Re: bug#73580: 29.4; ERC 5.6.1-git: erc-nicks does not respect pal and fool faces

[J.P.]

Trevor Arjeski <tmarjeski@gmail.com> writes:

> The erc-nicks module does not respect erc-pal-face and erc-fool-face
> when assigning a face for a nick, specifically when a user writes a
> message that includes a nick of a pal or fool, leading to the faces
> being different in the nick tag (<your_nick>) and the message
> (ex. "your_nick: hi").
>
> Reproduction config:
>
>    (use-package erc
>      :defer t
>      :config
>      (custom-set-faces
>       `(erc-pal-face ((t (:foreground "red")))))
>      (setopt erc-modules
>              (seq-union '(nicks) erc-modules))
>      :custom
>      (erc-nicks-colors '("yellow"))
>      (erc-pals '("trev")))
>    
> In the above config, you will notice that "<trev>" will be red, but when
> someone mentions "trev" in a message, the nick will be yellow (instead
> of red).
>
> Attached is a proposed patch. Feedback needed and welcome!

Thanks for the bug report. The good news is I can definitely reproduce
this. :)

But whether it's a bug and how to go about addressing it is a bit more
involved, I'm afraid. As luck would have it, this issue or something
similar actually comes up every now and again but not so much in the
context of `nicks' (or `erc-highlight-nicks' before it).

Anyway, as you may have noticed, the `nicks' module formally depends on
the `button' module and a new (5.6+) `button'-provided interface:
`erc-button--modify-nick-function'. IMO, this coupling is an acceptable
trade-off because `nicks' can piggyback on the token scanning that
`button' provides. The `button' module itself runs its code at depth 30
on `erc-insert-modify-hook', which is earlier than the `match' module's
50. This means it applies its faces _before_ `match' ever touches them.

What probably threw you off in perhaps thinking `match' had a say before
`nicks' was the presence of useless faces from `match' in the default
value of the option `erc-nicks-skip-faces'. That's indeed my bad: they
shouldn't be there at all (and a patch to fix this would be most
welcome). To that end, I'd much prefer we kept `nicks' and `match' as
loosely coupled as possible for the sake of long-term maintainability,
although I'm sure your current proposal is quite effective from a purely
pragmatic POV.

FWIW, the way `match' has always "worked" is that each category ("fool",
"keyword", etc.) has an accompanying "match type," like `keyword',
`nick', `message', etc., configured by an option like
`erc-current-nick-highlight-type'. However, unlike the "current-nick"
and "keyword" categories, "pal" and "fool" don't offer an equivalent of
`nick-or-keyword' or `keyword' for their corresponding highlight
options. This accounts for the undesirable behavior you observe (why
mentions aren't ever highlighted).

It likely won't surprise you to hear that expanding the match-type
selection of "fool" and "pal" to full parity with those of the other
categories has actually been suggested many times over in the past. In
fact, ISTR at least one patch/gist thingy floating around somewhere on
the wiki webs that purports to tackle this.

In terms of approaches, I think a more "modern" solution would address
this task by mimicking the way `nicks' modifies message text (but only
when `button' is loaded or slated to be via membership in
`erc-modules'). That is, it'd apply its faces indirectly by hooking into
`erc-button--modify-nick-function'. However, such an approach would also
likely involve more surgery (possibly a full refactor of
`erc-match-message'). And without adequate test coverage, I'd be
reticent to go that route. (BTW, the entire `match' module is severely
under-covered, so anything to remedy the situation would also be
welcome.)

As for a more traditional, non-`button' approach: I might be amenable to
a super minimal patch so long as it leaves a very light footprint. A
rough plan would be something like:

1. Add `keyword' and `nick-or-keyword' to highlight-type options for
   fools and pals.

2. Ensure the "-p" predicates for fools and pals consider the entire
   message. IIRC, only one of them does.

3. In, `erc-match-message', either modify the `keyword' case in the
   giant `cond' or add a new one for non-"current-nick" `keyword' and
   `nick-or-keyword' types to search and replace all pals and fools in
   the message body (possibly including the speaker tag, for the
   `nicks-or-keyword' variant).

(I'm probably missing something, but you get the idea.) If you're
willing to try this, I can help with benchmarking and adding tests. No
pressure though, obviously. Also, I very much appreciate all code
contributions, even those we don't end up using.


> I did a bit more testing and realized that the problem is also occuring for 
> erc-current-nick.
>
> Here is an updated patch:
>
> From 88b01b15219d86aac2c8670f86d6001368bb04d1 Mon Sep 17 00:00:00 2001
> From: Trevor Arjeski <tmarjeski@gmail.com>
> Date: Tue, 1 Oct 2024 20:48:04 +0300
> Subject: [PATCH] Make erc-nicks respect priority faces
>
[...]
>  
>  (defgroup erc-nicks nil
> @@ -464,6 +465,10 @@ Favor a custom erc-nicks-NICK@NETWORK-face when defined."
>                                                (erc-network-name) "-face")))
>                     ((or (and (facep face) face)
>                          (erc-nicks--revive face face nick (erc-network))))))
> +        (let ((face (cond ((erc-match-pal-p nick t) 'erc-pal-face)
> +                          ((erc-match-fool-p nick t) 'erc-fool-face)
> +                          ((equal nick (erc-current-nick)) 
> 'erc-my-nick-face))))
                                                               ~~~~~~~~~~~~~~~~

Actually, when I said I could reproduce the bug, I only meant WRT pals
and fools. This last face is used for so-called "input" messages, which
are outgoing messages taken from input at the prompt (which you probably
already knew). If you're saying `nicks' _should_ highlight your own
speaker tags (or should optionally do so), please explain.

Thanks.