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

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

[Nacho Barrientos]

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

Hi,

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

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

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.

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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-ERC-Track-Handle-face-text-properties-that-are-lists.patch --]
[-- Type: text/x-patch, Size: 1606 bytes --]

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)))
 
 ;;; 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))) ))
 
 ;;; erc-track-tests.el ends here
-- 
2.38.1


[-- Attachment #3: Type: text/plain, Size: 11570 bytes --]



In GNU Emacs 28.2 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.34, cairo version 1.17.6)
 of 2022-09-12 built on frederik
Windowing system distributor 'The X.Org Foundation', version 11.0.12101004
System Description: Arch Linux

Configured using:
 'configure --with-x-toolkit=gtk3 --with-native-compilation
 --sysconfdir=/etc --prefix=/usr --libexecdir=/usr/lib
 --localstatedir=/var --with-cairo --with-harfbuzz --with-libsystemd
 --with-modules 'CFLAGS=-march=x86-64 -mtune=generic -O2 -pipe -fno-plt
 -fexceptions -Wp,-D_FORTIFY_SOURCE=2 -Wformat -Werror=format-security
 -fstack-clash-protection -fcf-protection -g
 -ffile-prefix-map=/build/emacs/src=/usr/src/debug -flto=auto'
 'LDFLAGS=-Wl,-O1,--sort-common,--as-needed,-z,relro,-z,now -flto=auto''

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

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  paredit-mode: t
  global-undo-tree-mode: t
  undo-tree-mode: t
  erc-list-mode: t
  erc-menu-mode: t
  erc-ring-mode: t
  erc-pcomplete-mode: t
  erc-netsplit-mode: t
  recentf-mode: t
  goto-address-prog-mode: t
  erc-services-mode: t
  erc-autojoin-mode: t
  erc-networks-mode: t
  erc-track-mode: t
  erc-track-minor-mode: t
  erc-match-mode: t
  erc-spelling-mode: t
  erc-hl-nicks-mode: t
  erc-button-mode: t
  erc-fill-mode: t
  erc-stamp-mode: t
  erc-irccontrols-mode: t
  erc-noncommands-mode: t
  erc-move-to-prompt-mode: t
  erc-readonly-mode: t
  flyspell-mode: t
  display-time-mode: t
  engine-mode: t
  mu4e-column-faces-mode: t
  beginend-global-mode: t
  beginend-prog-mode: t
  outline-minor-mode: t
  global-git-commit-mode: t
  magit-auto-revert-mode: t
  editorconfig-mode: t
  hexl-follow-ascii: t
  csv-field-index-mode: t
  doom-modeline-mode: t
  yas-global-mode: t
  yas-minor-mode: t
  all-the-icons-ivy-rich-mode: t
  ivy-rich-project-root-cache-mode: t
  ivy-rich-mode: t
  ivy-posframe-mode: t
  ivy-mode: t
  shell-dirtrack-mode: t
  whole-line-or-region-global-mode: t
  whole-line-or-region-local-mode: t
  delete-selection-mode: t
  global-whitespace-mode: t
  override-global-mode: t
  global-eldoc-mode: t
  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
  window-divider-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  auto-fill-function: yas--auto-fill
  transient-mark-mode: t
  hs-minor-mode: t

Load-path shadows:
/home/nacho/.emacs.d/elpa/transient-20221124.2341/transient hides /usr/share/emacs/28.2/lisp/transient

Features:
(shadow emacsbug cal-move transpose-frame paredit autoload pcmpl-unix
dired-subtree dired-hacks-utils mhtml-mode css-mode-expansions css-mode
html-mode-expansions sgml-mode facemenu org-duration misearch
multi-isearch cl-print sql mastodon-search calc-alg calc-ext calc-menu
calc calc-loaddefs rect calc-macs descr-text animate misc avy tabify man
shr-color projectile rg rg-info-hack rg-menu rg-ibuffer rg-result
wgrep-rg wgrep rg-history rg-header ibuf-ext ibuffer ibuffer-loaddefs
grep cus-edit cus-start dired-aux pkg-info epl pcase git-rebase
magit-patch magit-subtree magit-gitignore magit-ediff ediff ediff-merg
ediff-mult ediff-wind ediff-diff ediff-help ediff-init ediff-util
forge-list forge-commands forge-semi forge-bitbucket buck forge-gogs
gogs forge-gitea gtea forge-gitlab glab forge-github ghub-graphql treepy
gsexp ghub forge-notify forge-revnote forge-pullreq forge-issue
forge-topic yaml forge-post markdown-mode forge-repo forge forge-core
forge-db closql emacsql-sqlite emacsql emacsql-compiler expand-region
yaml-mode-expansions subword-mode-expansions text-mode-expansions
cc-mode-expansions the-org-mode-expansions ruby-mode-expansions
python-mode-expansions js-mode-expansions web-mode-expansions
er-basic-expansions expand-region-core expand-region-custom mailalias
view mastodon-notifications smiley gnus-cite qp mm-archive mail-extr
mastodon-profile mastodon-media mastodon-tl ts url-http url-gw url-cache
url-auth mastodon-auth mastodon-client plstore mastodon mastodon-toot
mastodon-iso persist mastodon-http magit-extras info-colors crux mwim
goto-chg undo-tree queue gnutls network-stream nsm secrets erc-list
erc-menu erc-ring erc-pcomplete erc-netsplit tramp-cache bug-reference
recentf tree-widget counsel swiper em-unix em-term term ehelp em-script
em-prompt em-hist em-pred em-glob em-cmpl em-basic em-banner em-alias
goto-addr erc-services erc-join erc-networks erc-track erc-match
erc-spelling erc-hl-nicks erc-button erc-fill erc-stamp erc-goodies erc
erc-backend erc-loaddefs cern-ldap cap-words superword subword ldap
net-utils quail flyspell ispell ol-eww ol-rmail ol-mhe ol-irc ol-info
ol-gnus nnselect gnus-search eieio-opt speedbar ezimage dframe
ol-docview doc-view jka-compr image-mode exif ol-bibtex ol-bbdb ol-w3m
ol-doi org-link-doi appt diary-lib diary-loaddefs notifications ox-gfm
ox-md ox-odt rng-loc rng-uri rng-parse rng-match rng-dt rng-util
rng-pttrn nxml-parse nxml-ns nxml-enc xmltok nxml-util ox-latex
ox-icalendar org-agenda ox-html table ox-ascii ox-publish ox org-element
org-tree-slide org-timer org-clock org-capture org-refile org-protocol
ielm bluetooth desktop-environment time exwm-systemtray xcb-systemtray
xcb-xembed exwm exwm-input xcb-keysyms xcb-xkb exwm-manage exwm-floating
xcb-cursor xcb-render exwm-layout exwm-workspace exwm-core xcb-ewmh
xcb-icccm xcb xcb-xproto xcb-types xcb-debug engine-mode eww xdg mm-url
pipewire pipewire-lib pipewire-cli pipewire-access eradio elfeed-show
elfeed-search vc-mtn vc-hg vc-git vc-bzr vc-src vc-sccs vc-svn vc-cvs
vc-rcs vc vc-dispatcher elfeed-csv elfeed elfeed-curl elfeed-log
elfeed-db elfeed-lib avl-tree url-queue xml-query mu4e-column-faces
inline mu4e mu4e-org mu4e-main mu4e-view gnus-art mm-uu mml2015 mm-view
mml-smime smime dig gnus-sum gnus-group gnus-undo gnus-start gnus-dbus
dbus gnus-cloud nnimap nnmail mail-source utf7 netrc nnoo gnus-spec
gnus-int gnus-range gnus-win gnus nnheader wid-edit mu4e-headers
mu4e-compose mu4e-draft mu4e-actions smtpmail sendmail mu4e-search
mu4e-lists mu4e-bookmarks mu4e-mark mu4e-message shr kinsoku svg xml
flow-fill mule-util hl-line mu4e-contacts mu4e-update mu4e-folders
mu4e-server mu4e-context mu4e-vars mu4e-helpers mu4e-config ido helpful
cc-langs trace edebug help-fns radix-tree elisp-refs beginend git-modes
gitignore-mode gitconfig-mode gitattributes-mode git-link orgit org ob
ob-tangle ob-ref ob-lob ob-table ob-exp org-macro org-footnote org-src
ob-comint org-pcomplete org-list org-faces org-entities
outline-minor-faces noutline outline org-version ob-emacs-lisp ob-core
ob-eval org-table oc-basic bibtex ol org-keys oc org-compat org-macs
cal-menu calendar cal-loaddefs magit-bookmark magit-submodule
magit-obsolete magit-blame magit-stash magit-reflog magit-bisect
magit-push magit-pull magit-fetch magit-clone magit-remote magit-commit
magit-sequence magit-notes magit-worktree magit-tag magit-merge
magit-branch magit-reset magit-files magit-refs magit-status magit
magit-repos magit-apply magit-wip magit-log magit-diff smerge-mode diff
diff-mode git-commit log-edit message rmc puny rfc822 mml mml-sec epa
derived epg rfc6068 epg-config gnus-util rmail rmail-loaddefs mm-decode
mm-bodies mm-encode mailabbrev gmm-utils pcvs-util add-log magit-core
magit-autorevert magit-margin magit-transient magit-process with-editor
magit-mode transient magit-git magit-base magit-section crm compat-27
compat-26 eshell-prompt-extras em-dirs esh-var esh-mode em-ls
eshell-bookmark bookmark eshell powerthesaurus request mailheader
autorevert filenotify mail-utils dom virtualenvwrapper
gitignore-templates editorconfig editorconfig-core
editorconfig-core-handle editorconfig-fnmatch nhexl-mode hexl sqlformat
reformatter jq-mode csv-mode sort web-mode disp-table sh-script
executable systemd conf-mode archive-rpm bindat archive-cpio arc-mode
archive-mode rpm-spec-mode yaml-mode json-mode json-snatcher js cc-mode
cc-fonts cc-guess cc-menus cc-styles cc-align go-dlv gud go-mode
find-file ffap etags fileloop generator xref rspec-mode python-mode
info-look advice org-loaddefs which-func hideshow hippie-exp
flymake-proc flymake project thingatpt ert pp ewoc debug backtrace
compile cc-cmds cc-engine cc-vars cc-defs ruby-mode smie
flycheck-package package-lint let-alist imenu finder lisp-mnt mail-parse
rfc2231 rfc2047 rfc2045 mm-util ietf-drums mail-prsvr flycheck find-func
rainbow-mode xterm-color doom-modeline doom-modeline-segments
doom-modeline-env doom-modeline-core shrink-path f f-shortdoc shortdoc
text-property-search dash compat compat-macs doom-themes-ext-visual-bell
face-remap doom-tokyo-night-theme doom-themes doom-themes-base
yasnippet-snippets yasnippet amx comp comp-cstr warnings s
all-the-icons-ivy-rich ivy-rich ivy-posframe posframe all-the-icons-ivy
ivy ivy-faces ivy-overlay colir color all-the-icons all-the-icons-faces
data-material data-weathericons data-octicons data-fileicons
data-faicons data-alltheicons disk-usage dired dired-loaddefs pinentry
tramp-cmds em-tramp esh-cmd esh-ext esh-opt esh-module esh-groups
esh-proc esh-io esh-arg esh-util tramp-sh tramp tramp-loaddefs trampver
tramp-integration files-x tramp-compat shell pcomplete comint ansi-color
ring parse-time iso8601 time-date ls-lisp format-spec
whole-line-or-region delsel cus-load whitespace cl-extra help-mode
use-package use-package-ensure use-package-delight use-package-diminish
use-package-bind-key bind-key easy-mmode use-package-core finder-inf
edmacro kmacro avoid server rx info package browse-url url url-proxy
url-privacy url-expand url-methods url-history url-cookie url-domsuf
url-util mailcap url-handlers url-parse auth-source cl-seq eieio
eieio-core cl-macs eieio-loaddefs password-cache json subr-x map
url-vars seq byte-opt gv bytecomp byte-compile cconv cl-loaddefs cl-lib
iso-transl tooltip 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 cl-generic
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 simple abbrev obarray
cl-preloaded nadvice button loaddefs faces cus-face macroexp files
window text-properties overlay sha1 md5 base64 format env code-pages
mule custom widget hashtable-print-readable backquote threads dbusbind
inotify lcms2 dynamic-setting system-font-setting font-render-setting
cairo move-toolbar gtk x-toolkit x multi-tty make-network-process
native-compile emacs)

Memory information:
((conses 16 3838378 613339)
 (symbols 48 118745 6)
 (strings 32 646898 68799)
 (string-bytes 1 35930634)
 (vectors 16 249201)
 (vector-slots 8 5285026 676668)
 (floats 8 6777 8859)
 (intervals 56 188731 21866)
 (buffers 992 211))

--
 bye
 Nacho
 http://cern.ch/nacho

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

[J.P.]

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.

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

[Nacho Barrientos]

Hi J.P.,

On 06/12/22, J.P. said:

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

Thanks for taking the time to do this; your insights have been useful
for me to learn new stuff. As I mentioned I'm rather unfamiliar with
ERC's code *grins*.

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

I was totally unaware that "face composites" were a thing, so
perhaps my proposal to flatten the return value of `erc-faces-in' does
not make sense anymore.

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

This is indeed what I did initially in my configuration, before I
thought about reporting this bug. This approach works, however in my
opinion it's not ideal (for me as user), because:

1) I have to monkeypatch erc-hl-nicks.
2) I have to hardcode the nick-dependant face name to look for. There's
   already a face to identify mentions to the current nick
   (`erc-current-nick-face') so this should not be necessary.

1) could be addressed by submitting a patch for erc-hl-nicks so those
symbols are interned so they could be `equal''ed, as you suggested.

For 2) I don't have an elegant/generic solution to propose, but adding

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

to `erc-track-faces-priority-list' can definitely work for me. My
nickname is stable across networks, however I think that the rather
valid use case of being notified no matter what your nick name is cannot
be honoured elegantly when using erc-hl-nicks.

--
 bye
 Nacho
 http://cern.ch/nacho

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

[Nacho Barrientos]

Hi again,

On 06/12/22, Nacho Barrientos said:
> This is indeed what I did initially in my configuration, before I
> thought about reporting this bug. This approach works, however in my
> opinion it's not ideal (for me as user), because:
>
> 1) I have to monkeypatch erc-hl-nicks.
> 2) I have to hardcode the nick-dependant face name to look for. There's
>    already a face to identify mentions to the current nick
>    (`erc-current-nick-face') so this should not be necessary.
>
> 1) could be addressed by submitting a patch for erc-hl-nicks so those
> symbols are interned so they could be `equal''ed, as you suggested.
>
> For 2) I don't have an elegant/generic solution to propose, but adding
>
>   (erc-hl-nicks-nick-nacho-face erc-current-nick-face)
>
> to `erc-track-faces-priority-list' can definitely work for me. My
> nickname is stable across networks, however I think that the rather
> valid use case of being notified no matter what your nick name is cannot
> be honoured elegantly when using erc-hl-nicks.

Actually there's something else that can be done from the erc-hl-nicks
side, which is:

  (setq erc-hl-nicks-skip-nicks '("nacho"))

Which comes again with the problematic of having to hardcode known nicks
but it does the job. If that variable is set as indicated above,
erc-hl-nicks will never create the composite face on the mention and
hence:

  λ> (erc-faces-in (buffer-substring (point) (+ 1 (point))))
  (erc-current-nick-face)

(in the example above, `point' is on a mention to the current nick)

Making erc-track's matching work. The current nickname is coloured
anyway by ERC so if erc-hl-nicks does not act on it should be okay.

This approach is good enough for me so feel free to close the bug if you
don't want to go down this rabbit hole further :)

Thanks again.

--
 bye
 Nacho
 http://cern.ch/nacho

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

[J.P.]

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

> Hi J.P.,

Hi Nacho,

>> 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.
>
> I was totally unaware that "face composites" were a thing, so
> perhaps my proposal to flatten the return value of `erc-faces-in' does
> not make sense anymore.

FWIW, "composite" was just some term I conjured up to refer to what's
described in the third `face' bullet point of "(elisp) Special
Properties":

  A list of faces.  Each list element should be either a face
  name or an anonymous face.  This specifies a face which is an
  aggregate of the attributes of each of the listed faces.
  Faces occurring earlier in the list have higher priority.

While perhaps less common than lone face names, such values do appear in
the wild, for example, when `whitespace-mode' is active.

> For 2) I don't have an elegant/generic solution to propose, but adding
>
>   (erc-hl-nicks-nick-nacho-face erc-current-nick-face)
>
> to `erc-track-faces-priority-list' can definitely work for me. My
> nickname is stable across networks, however I think that the rather
> valid use case of being notified no matter what your nick name is cannot
> be honoured elegantly when using erc-hl-nicks.

From what I can gather, you still believe this despite the more recent
findings in your followup just below.

> Hi again,
>
> [...]
>
> Actually there's something else that can be done from the erc-hl-nicks
> side, which is:
>
>   (setq erc-hl-nicks-skip-nicks '("nacho"))

Good find!

> Which comes again with the problematic of having to hardcode known nicks
> but it does the job. If that variable is set as indicated above,
> erc-hl-nicks will never create the composite face on the mention and
> hence:
>
>   λ> (erc-faces-in (buffer-substring (point) (+ 1 (point))))
>   (erc-current-nick-face)
>
> (in the example above, `point' is on a mention to the current nick)
>
> Making erc-track's matching work. The current nickname is coloured
> anyway by ERC so if erc-hl-nicks does not act on it should be okay.

Re the "hardcode problem": I'm not sure it's incumbent upon ERC to offer
a solution uniquely tailored to updating `erc-hl-nicks-skip-nicks'
specifically (not that you were suggesting such a thing). However, I do
think the current offering of `erc-nick-changed-functions' has proven
insufficient for servicing a variety of legitimate needs. It only runs
under really specific circumstances, such as when a server-mandated nick
change occurs or a confirmation arrives following a NickServ IDENTIFY
(or similar action). However, it does *not* run on confirmation of a
client-initiated NICK command, whether manually issued by a user or
executed as library code.

Unfortunately, for compatibility reasons, we can't just repurpose
`erc-nick-changed-functions' willy-nilly. Thus, I think introducing a
*new* abnormal hook to be run by `erc-set-current-nick' might be
justified, likely defined as compatible with functions taking a new nick
and giving nothing in return. Users and downstream packages could then
do whatever needs doing in reaction to nick changes, for example:

  (add-hook 'erc-set-current-nick-functions
            (lambda (n)
              (customize-set-variable
               'erc-hl-nicks-skip-nicks
               (cl-pushnew n erc-hl-nicks-skip-nicks :test #'equal))))

> This approach is good enough for me so feel free to close the bug if you
> don't want to go down this rabbit hole further :)

Regardless of this bug's original title, the mission of improving the
user experience remains paramount, wherever that may lead. If you're up
for it, I for one would welcome a patch adding a hook like the one
described above (whenever time allows). Otherwise, we could always just
kick the can down the road and rename this bug to something like "ERC
needs a smarter way to run user code on nick changes" in hopes some
enterprising future person will find it. Either way, I appreciate your
willingness to contribute to ERC.

J.P.

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

[J.P.]

"J.P." <jp@neverwas.me> writes:

> 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 ()
> [...]
>       (with-current-buffer (get-buffer-create "*erc-track--erc-faces-in*")
>         (font-lock-default-function 1) ; set buffer-local variables

Yeah, this is nonsense (sorry, people). Not sure what I was smoking, but
ERT runs its tests in a temporary buffer, so buffer-local stuff just
evaporates.

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

[J.P.]

This bug raised the issue of users needing to expressly customize an
option (in a third party package) to work around a perceived problem in
the way ERC combines new and existing faces. ERC has since gained its
own module for highlighting nicknames that refrains from doing so atop
`erc-current-nick-face' by default.

As for consing lists of `{,font-lock-}face' values onto one another
(rather than concatenating them), ERC now has a means of addressing this
as well as any headaches posed by traversing traditional trees. See the
new internal functions `erc--merge-prop' and `erc-nicks--skip-p'. And
while it's true that ERC, at present, only merges the `invisible' text
property, extending this to faces remains possible, perhaps during a
major release, like a 6.0.

Thanks and closing.