bug#44100: 26.3; ERC: Make IBuffer custom display work again

bug#44100: 26.3; ERC: Make IBuffer custom display work again

[Olivier Certner]

Applies to 26.3, but also all more recent versions as well.

Bug trigger:
1. Load ERC (e.g., open some IRC connection).
2. Launch ibuffer (e.g., type "M-x ibuffer").
3. Switch formats three times (e.g., type "`" three times).
4. You get this error: (wrong-type-argument listp erc-notice-face) and
the buffer is only partly filled, and then cannot be updated.

Root cause:
erc-ibuffer assumes (in (define-ibuffer-column erc-modified ...) form)
that erc-modified-channels-alist is an alist whose values are proper
lists, whereas they are dotted lists.

Patch:
To be attached shortly after bug creation.

-- 
Olivier Certner

bug#44100: Patch

[Olivier Certner]

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

	In attachment.

-- 
Olivier Certner

[-- Attachment #2: 0001-ERC-Make-IBuffer-ERC-s-custom-displays-work-again.patch --]
[-- Type: text/x-patch, Size: 1495 bytes --]

From 33586f449fdf646fbfb1aa6bd5ef6ca4c4848761 Mon Sep 17 00:00:00 2001
From: Olivier Certner <ocert.dev@free.fr>
Date: Tue, 20 Oct 2020 18:36:25 +0200
Subject: [PATCH] ERC: Make IBuffer ERC's custom displays work again

* lisp/erc/erc-ibuffer.el (erc-modified): Don't use `length' on
dotted lists (and not even to test if there is more than one
element, for that matter). Use `cdr' instead.
---
 ChangeLog.3             | 8 ++++++++
 lisp/erc/erc-ibuffer.el | 2 +-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/ChangeLog.3 b/ChangeLog.3
index ec2d3f8d46..86579b3913 100644
--- a/ChangeLog.3
+++ b/ChangeLog.3
@@ -1,3 +1,11 @@
+2020-10-20  Olivier Certner  <ocert.dev@free.fr>
+
+	ERC: Make IBuffer ERC's custom displays work again
+
+	* lisp/erc/erc-ibuffer.el (erc-modified): Don't use `length' on
+	dotted lists (and not even to test if there is more than one
+	element, for that matter). Use `cdr' instead.
+
 2020-08-03  Phil Sainty  <psainty@orcon.net.nz>
 
 	lisp/so-long.el: Improve support for major mode hooks
diff --git a/lisp/erc/erc-ibuffer.el b/lisp/erc/erc-ibuffer.el
index 7aee7705ff..955b022177 100644
--- a/lisp/erc/erc-ibuffer.el
+++ b/lisp/erc/erc-ibuffer.el
@@ -77,7 +77,7 @@
 	   erc-track-mode)
       (let ((entry (assq (current-buffer) erc-modified-channels-alist)))
 	(if entry
-	    (if (> (length entry) 1)
+	    (if (cdr entry)
 		(cond ((eq 'pal (nth 1 entry))
 		       (string erc-ibuffer-pal-char))
 		      ((eq 'fool (nth 1 entry))
-- 
2.24.1

bug#44100: Patch

[Corwin Brust]

Thank you, Olivier!

On Wed, Oct 21, 2020 at 6:42 AM Olivier Certner <ocert.dev@free.fr> wrote:
>
>         In attachment.
>
> --
> Olivier Certner

This looks fine to me.  Adding Amin and CC to the erc mailing list.
TIA for review and comments!

Regards,
Corwin

bug#44100: Patch

[Amin Bandali]

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

Hello Olivier, Corwin,

Corwin Brust writes:

> Thank you, Olivier!
>
> On Wed, Oct 21, 2020 at 6:42 AM Olivier Certner <ocert.dev@free.fr> wrote:
>>
>>         In attachment.
>>
>> --
>> Olivier Certner
>
> This looks fine to me.  Adding Amin and CC to the erc mailing list.
> TIA for review and comments!
>
> Regards,
> Corwin

Thanks for your Patch, Olivier; and thanks for the ping, Corwin.

I tried, but could not reproduce the error on Emacs trunk (from latest
git `master').  I will try with earlier versions, but it may be another
couple of days before I get to it.

Corwin, were you able to reproduce this?

Also, according to CONTRIBUTE [0] the ChangeLog files should not be
edited manually, and instead changes should be announced in the etc/NEWS
file, under the relevant section.

[0]: https://git.savannah.gnu.org/cgit/emacs.git/tree/CONTRIBUTE

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]

bug#44100: Patch

[Amin Bandali]

[-- Attachment #1.1: Type: text/plain, Size: 578 bytes --]

Hello again,

I did some more investigation, and was able to reproduce the issue.

However, I came up with a potentially more proper different fix: I think
the presence of the dotted pair with a non-nil cdr is the main problem,
since it means `erc-modified-channels-alist' does not fully adhere to
the documented structure in its docstring.  So I addressed the issue in
`erc-track-modified-channels' by wrapping up single faces in lists too,
ensuring that `erc-modified-channels-alist' never ends in a non-nil cdr.

Something along the lines of the following patch.  Thoughts?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-Maintain-the-documented-structure-for-erc-modified-c.patch --]
[-- Type: text/x-diff, Size: 3376 bytes --]

From 9cccb6b00fea837c9d28305db30b21353134b990 Mon Sep 17 00:00:00 2001
From: Amin Bandali <bandali@gnu.org>
Date: Sat, 31 Oct 2020 21:41:38 -0400
Subject: [PATCH] Maintain the documented structure for
 'erc-modified-channels-alist'

* lisp/erc/erc-track.el (erc-track-modified-channels): Wrap the single
face in a list to avoid a non-nil final cdr in
`erc-modified-channels-alist', making its value adhere more closely to
its stated structure in its docstring.  Also unbreaks erc-ibuffer.
* lisp/erc/erc-status-sidebar.el (erc-status-sidebar-default-chan-format):
Account for the change in structure of `erc-modified-channels-alist'
due to this fix.
---
 lisp/erc/erc-status-sidebar.el |  2 +-
 lisp/erc/erc-track.el          | 18 ++++++++++++------
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/lisp/erc/erc-status-sidebar.el b/lisp/erc/erc-status-sidebar.el
index 08dc8d6015..4fc9b4fc30 100644
--- a/lisp/erc/erc-status-sidebar.el
+++ b/lisp/erc/erc-status-sidebar.el
@@ -191,7 +191,7 @@ erc-status-sidebar-default-chan-format
     (setq channame (format "%s [%d]" channame num-messages)))
   (when erc-face
     (put-text-property 0 (length channame) 'face erc-face channame)
-    (when (eq erc-face 'erc-default-face)
+    (when (eq (car erc-face) 'erc-default-face)
       (add-face-text-property 0 (length channame) 'bold t channame)))
   channame)
 
diff --git a/lisp/erc/erc-track.el b/lisp/erc/erc-track.el
index 60f0cfa942..3618df6568 100644
--- a/lisp/erc/erc-track.el
+++ b/lisp/erc/erc-track.el
@@ -815,10 +815,13 @@ erc-track-modified-channels
 			      (throw 'found t))))))
 	    (if (not (assq (current-buffer) erc-modified-channels-alist))
 		;; Add buffer, faces and counts
-		(setq erc-modified-channels-alist
-		      (cons (cons (current-buffer)
-				  (cons 1 (erc-track-find-face faces)))
-			    erc-modified-channels-alist))
+                (let ((new-face (erc-track-find-face faces)))
+                  (setq erc-modified-channels-alist
+                        (cons (cons (current-buffer)
+                                    (cons 1 (if (listp new-face)
+                                                new-face
+                                              (list new-face))))
+                              erc-modified-channels-alist)))
 	      ;; Else modify the face for the buffer, if necessary.
 	      (when faces
 		(let* ((cell (assq (current-buffer)
@@ -828,7 +831,10 @@ erc-track-modified-channels
 				  (if old-face
 				      (cons old-face faces)
 				    faces))))
-		  (setcdr cell (cons (1+ (cadr cell)) new-face)))))
+		  (setcdr cell (cons (1+ (cadr cell))
+                                     (if (listp new-face)
+                                         new-face
+                                       (list new-face)))))))
 	    ;; And display it
 	    (erc-modified-channels-display)))
       ;; Else if the active buffer is the current buffer, remove it
@@ -863,7 +869,7 @@ erc-track-last-non-erc-buffer
 `erc-track-switch-buffer'.")
 
 (defun erc-track-sort-by-activest ()
-  "Sort erc-modified-channels-alist by activity.
+  "Sort `erc-modified-channels-alist' by activity.
 That means the number of unseen messages in a channel."
   (setq erc-modified-channels-alist
 	(sort erc-modified-channels-alist
-- 
2.17.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]

bug#44100: Patch

[Lars Ingebrigtsen]

Amin Bandali <bandali@gnu.org> writes:

> However, I came up with a potentially more proper different fix: I think
> the presence of the dotted pair with a non-nil cdr is the main problem,
> since it means `erc-modified-channels-alist' does not fully adhere to
> the documented structure in its docstring.  So I addressed the issue in
> `erc-track-modified-channels' by wrapping up single faces in lists too,
> ensuring that `erc-modified-channels-alist' never ends in a non-nil cdr.
>
> Something along the lines of the following patch.  Thoughts?

[...]

>    (when erc-face
>      (put-text-property 0 (length channame) 'face erc-face channame)
> -    (when (eq erc-face 'erc-default-face)
> +    (when (eq (car erc-face) 'erc-default-face)
>        (add-face-text-property 0 (length channame) 'bold t channame)))
>    channame)

Normalising to having a list of faces here sounds like a good idea, if
I'm skimming the code correctly, but it would probably help
comprehension a bit here if you renamed the parameter from `erc-face' to
something like `face-list'.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#44100: Patch

[Amin Bandali]

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

Lars Ingebrigtsen writes:

[...]
>
> Normalising to having a list of faces here sounds like a good idea, if
> I'm skimming the code correctly, but it would probably help
> comprehension a bit here if you renamed the parameter from `erc-face' to
> something like `face-list'.

Thanks.  However, after staring at C-h f erc-track-find-face RET for a
good while and experimenting in Edebug a fair bit, I have noticed that
my change to C-h f erc-track-modified-channels RET for normalizing to
always have a list breaks erc-track-find-face's algorithm in a
non-trivial way, due its dependence on the structure of the two
C-h v erc-track-faces-priority-list RET and
C-h v erc-track-faces-normal-list RET lists.

Since for the time being I don't have the time or energy to rethink
those two functions' algorithms to work with a normalized list of lists
for those two variables, I have decided to keep things as they are, and
do the less evasive change in erc-ibuffer as suggested by Olivier to fix
erc-ibuffer.  I will commit that change over the few days, and will also
try to clarify the docstring for C-h v erc-modified-channels-alist RET
some more to more accurately describe its structure.

I welcome anyone who would like to volunteer to send a patch for
updating those functions to work with a normalized list of lists for
those two variables, granted they can provide a convincing argument that
their change is correct and does not break the current behaviour, does
not dramatically further increase the complexity of the algorithms, and
is ideally backward compatible with the current non-normalized structure
of those two variables as well.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]

bug#44100: Patch

[Olivier Certner]

Hello Amin,

Sorry for the late reply, I missed these mails in a flood of other mails.

And sorry as well for the ChangeLog entry, I had read CONTRIBUTE, but mixed 
things up (there is a section explaining how to generate change log entries in 
Emacs afterwards, and I forgot the earlier advice).

> [...]
>
> I welcome anyone who would like to volunteer to send a patch for
> updating those functions to work with a normalized list of lists for
> those two variables, granted they can provide a convincing argument that
> their change is correct and does not break the current behaviour, does
> not dramatically further increase the complexity of the algorithms, and
> is ideally backward compatible with the current non-normalized structure
> of those two variables as well.

I'll have a look at this next week.

Thanks and regards.

-- 
Olivier Certner

Re: bug#44100: Patch

[Amin Bandali]

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

Hi Olivier,

Olivier Certner writes:

> Hello Amin,
>
> Sorry for the late reply, I missed these mails in a flood of other mails.
>

No problem!

>
> And sorry as well for the ChangeLog entry, I had read CONTRIBUTE, but
> mixed things up (there is a section explaining how to generate change
> log entries in Emacs afterwards, and I forgot the earlier advice).
>

Ah :-).  No worries; I thought I'd point out CONTRIBUTE just in case
you'd missed it.

>
>> [...]
>>
>> I welcome anyone who would like to volunteer to send a patch for
>> updating those functions to work with a normalized list of lists for
>> those two variables, granted they can provide a convincing argument that
>> their change is correct and does not break the current behaviour, does
>> not dramatically further increase the complexity of the algorithms, and
>> is ideally backward compatible with the current non-normalized structure
>> of those two variables as well.
>
> I'll have a look at this next week.
>
> Thanks and regards.

Sounds good, thanks!  Please keep us updated on how it goes.

Also, since implementing this will likely cross the ~15 line triviality
threshold, I sent you a copy of the copyright assignment request form
off-list, which you would fill out and send to the copyright clerk to
start the process of assigning the copyright for your changes to Emacs
to the FSF.

Thanks,
amin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]

bug#44100: 26.3; ERC: Make IBuffer custom display work again

[Lars Ingebrigtsen]

Amin Bandali <bandali@gnu.org> writes:

> Also, since implementing this will likely cross the ~15 line triviality
> threshold, I sent you a copy of the copyright assignment request form
> off-list, which you would fill out and send to the copyright clerk to
> start the process of assigning the copyright for your changes to Emacs
> to the FSF.

Amin, Olivier's paperwork is now complete, so could you look at the
following four patches from Olivier and review (and possibly apply)?

46777 normal,pat Olivier Certner    28.0.50; ERC: NickServ identification: Prompt for password after other sources, overall simplifications
46775 normal,pat Olivier Certner    27.1; ERC: Track: Modified channels doc and `erc-track-find-face' fixes
44140 normal,pat Olivier Certner    26.3; ERC stamps: Really use latest buffer's window's width prior to `fill-column'
44100 normal,pat Olivier Certner    26.3; ERC: Make IBuffer custom display work again


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#44100: 26.3; ERC: Make IBuffer custom display work again

[bandali]

Lars Ingebrigtsen writes:

> Amin Bandali <bandali@gnu.org> writes:
>
>> Also, since implementing this will likely cross the ~15 line triviality
>> threshold, I sent you a copy of the copyright assignment request form
>> off-list, which you would fill out and send to the copyright clerk to
>> start the process of assigning the copyright for your changes to Emacs
>> to the FSF.
>
> Amin, Olivier's paperwork is now complete, so could you look at the
> following four patches from Olivier and review (and possibly apply)?
>
> 46777 normal,pat Olivier Certner    28.0.50; ERC: NickServ identification: Prompt for password after other sources, overall simplifications
> 46775 normal,pat Olivier Certner    27.1; ERC: Track: Modified channels doc and `erc-track-find-face' fixes
> 44140 normal,pat Olivier Certner    26.3; ERC stamps: Really use latest buffer's window's width prior to `fill-column'
> 44100 normal,pat Olivier Certner    26.3; ERC: Make IBuffer custom display work again

Thanks, Lars.  I will indeed be reviewing/applying Olivier's patches
over the next couple of days.

bug#44100: 26.3; ERC: Make IBuffer custom display work again

[Amin Bandali]

Hi Olivier, all,

Olivier Certner writes:

> Hi,
>
> I've updated the patches for all these so that they cleanly apply on 28.0.50 
> (68276f6d30bbdc09). I've tested them again with a fresh build and everything 
> seems OK.
>
> Don't hesitate to poke me if you have additional questions or comments.
>
> Regards.

I hope you are all well, and my apologies for the long delay on this.

I went ahead and applied your updated patch to 'emacs-27' with a small
wording tweak and merged that into 'master' as well.  I'll also get to
your other patches over the next few days.

Thanks again,
amin

bug#44100: Updated patch

[Olivier Certner]

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

Patch slightly edited (commit message) and rebased on top of a recent 
'master'. Ready to apply. Easy to backport to 27 as well.

-- 
Olivier Certner

[-- Attachment #2: 0001-ERC-Unbreak-ERC-s-IBuffer-s-custom-displays-Bug-4410.patch --]
[-- Type: text/x-patch, Size: 957 bytes --]

From c21f772f35fad8f80d951048ff134387e30ca076 Mon Sep 17 00:00:00 2001
From: Olivier Certner <olce.emacs@certner.fr>
Date: Tue, 6 Jul 2021 10:51:57 +0200
Subject: [PATCH] ERC: Unbreak ERC's IBuffer's custom displays (Bug#44100)

* lisp/erc/erc-ibuffer.el (erc-modified): Don't use `length' on dotted
lists (and not even to test if there is more than one element, for
that matter).  Use `cdr' instead.
---
 lisp/erc/erc-ibuffer.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/erc/erc-ibuffer.el b/lisp/erc/erc-ibuffer.el
index 22336ede21..31e59a6d3e 100644
--- a/lisp/erc/erc-ibuffer.el
+++ b/lisp/erc/erc-ibuffer.el
@@ -73,7 +73,7 @@ erc-modified
 	   erc-track-mode)
       (let ((entry (assq (current-buffer) erc-modified-channels-alist)))
 	(if entry
-	    (if (> (length entry) 1)
+	    (if (cdr entry)
 		(cond ((eq 'pal (nth 1 entry))
 		       (string erc-ibuffer-pal-char))
 		      ((eq 'fool (nth 1 entry))
-- 
2.30.0

bug#44100: On Amin's proposed change

[Olivier Certner]

For the record, an extract of a private exchange with Amin on the change he 
proposed earlier in this bug (Bug#44100).

> > Sounds good, thanks!  Instrumenting the function(s),
> > putting breakpoints, and playing around with them should
> > hopefully help develop some intuition around them. 
> 
> As I was suspecting, your patch didn't work just because you did not convert
> the calls to `erc-track-find-face' correctly with respect to the change you
> wanted to make.
> 
> I'm not particularly keen on changing `erc-modified-channels-alist' format,
> I just find it OK as it is (more compact), and chose to change its
> documentation instead. Now, if you really insist on the other way, it's
> quick to do as well, and I'll send you the corresponding proposal.
> 
> I've also spent a while figuring out what `erc-track-find-face' does, and
> I've decided to rewrite it for clarification (without changing the
> behavior). It then appeared to me that its interface and call sites were
> doing something I also suspected to be wrong, for which I did apply another
> correction.
> 
> And I also updated the documentation of some vars/funs involved in tracking,
> to make things a bit easier to understand and cross-link better between
> them.
> 
> I'm currently testing the changes, and everything seems fine so far. I'll
> make the changes publicly available tomorrow for you to see, so we can
> start discussing them.

I made these changes public a while ago in Bug#46775.

-- 
Olivier Certner