GNU bug report logs - #44100
26.3; ERC: Make IBuffer custom display work again

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 44100 in the body.
You can then email your comments to 44100 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Olivier Certner <ocert.dev <at> free.fr>
To: bug-gnu-emacs <at> gnu.org
Subject: 26.3; ERC: Make IBuffer custom display work again
Date: Tue, 20 Oct 2020 18:39:15 +0200

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

Message #8 received at 44100 <at> debbugs.gnu.org (full text, mbox):

From: Olivier Certner <ocert.dev <at> free.fr>
To: 44100 <at> debbugs.gnu.org
Subject: Patch
Date: Wed, 21 Oct 2020 12:08:47 +0200

[Message part 1 (text/plain, inline)]

	In attachment.

-- 
Olivier Certner

[0001-ERC-Make-IBuffer-ERC-s-custom-displays-work-again.patch (text/x-patch, attachment)]

Message #11 received at 44100 <at> debbugs.gnu.org (full text, mbox):

From: Corwin Brust <corwin <at> bru.st>
To: Olivier Certner <ocert.dev <at> free.fr>, Amin Bandali <bandali <at> gnu.org>
Cc: emacs-erc <at> gnu.org, 44100 <at> debbugs.gnu.org
Subject: Re: bug#44100: Patch
Date: Wed, 21 Oct 2020 10:54:13 -0500

Thank you, Olivier!

On Wed, Oct 21, 2020 at 6:42 AM Olivier Certner <ocert.dev <at> 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

Message #16 received at 44100 <at> debbugs.gnu.org (full text, mbox):

From: Amin Bandali <bandali <at> gnu.org>
To: Corwin Brust <corwin <at> bru.st>
Cc: emacs-erc <at> gnu.org, 44100 <at> debbugs.gnu.org,
 Olivier Certner <ocert.dev <at> free.fr>
Subject: Re: bug#44100: Patch
Date: Sat, 31 Oct 2020 15:54:12 -0400

[Message part 1 (text/plain, inline)]

Hello Olivier, Corwin,

Corwin Brust writes:

> Thank you, Olivier!
>
> On Wed, Oct 21, 2020 at 6:42 AM Olivier Certner <ocert.dev <at> 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

[signature.asc (application/pgp-signature, inline)]

Message #19 received at 44100 <at> debbugs.gnu.org (full text, mbox):

From: Amin Bandali <bandali <at> gnu.org>
To: Corwin Brust <corwin <at> bru.st>
Cc: emacs-erc <at> gnu.org, 44100 <at> debbugs.gnu.org,
 Olivier Certner <ocert.dev <at> free.fr>
Subject: Re: bug#44100: Patch
Date: Sat, 31 Oct 2020 22:16:54 -0400

[Message part 1 (text/plain, inline)]

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?

[0001-Maintain-the-documented-structure-for-erc-modified-c.patch (text/x-diff, inline)]

From 9cccb6b00fea837c9d28305db30b21353134b990 Mon Sep 17 00:00:00 2001
From: Amin Bandali <bandali <at> 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

[signature.asc (application/pgp-signature, inline)]

Message #22 received at 44100 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Amin Bandali <bandali <at> gnu.org>
Cc: Corwin Brust <corwin <at> bru.st>, emacs-erc <at> gnu.org, 44100 <at> debbugs.gnu.org,
 Olivier Certner <ocert.dev <at> free.fr>
Subject: Re: bug#44100: Patch
Date: Sun, 01 Nov 2020 14:09:54 +0100

Amin Bandali <bandali <at> 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

Message #25 received at 44100 <at> debbugs.gnu.org (full text, mbox):

From: Amin Bandali <bandali <at> gnu.org>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: emacs-erc <at> gnu.org, 44100 <at> debbugs.gnu.org,
 Olivier Certner <ocert.dev <at> free.fr>
Subject: Re: bug#44100: Patch
Date: Mon, 02 Nov 2020 21:38:21 -0500

[Message part 1 (text/plain, inline)]

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.

[signature.asc (application/pgp-signature, inline)]

Message #28 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Olivier Certner <ocert.dev <at> free.fr>
To: Lars Ingebrigtsen <larsi <at> gnus.org>, Amin Bandali <bandali <at> gnu.org>
Cc: bug-gnu-emacs <at> gnu.org, emacs-erc <at> gnu.org, 44100 <at> debbugs.gnu.org
Subject: Re: bug#44100: Patch
Date: Thu, 05 Nov 2020 17:38:11 +0100

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

Message #34 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Amin Bandali <bandali <at> gnu.org>
To: Olivier Certner <ocert.dev <at> free.fr>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, emacs-erc <at> gnu.org,
 bug-gnu-emacs <at> gnu.org, 44100 <at> debbugs.gnu.org
Subject: Re: bug#44100: Patch
Date: Sat, 07 Nov 2020 09:54:13 -0500

[Message part 1 (text/plain, inline)]

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

[signature.asc (application/pgp-signature, inline)]

Message #40 received at 44100 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Amin Bandali <bandali <at> gnu.org>
Cc: emacs-erc <at> gnu.org, 44100 <at> debbugs.gnu.org,
 Olivier Certner <ocert.dev <at> free.fr>
Subject: Re: bug#44100: 26.3; ERC: Make IBuffer custom display work again
Date: Fri, 04 Jun 2021 12:07:51 +0200

Amin Bandali <bandali <at> 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

Message #43 received at 44100 <at> debbugs.gnu.org (full text, mbox):

From: bandali <at> gnu.org
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: emacs-erc <at> gnu.org, 44100 <at> debbugs.gnu.org,
 Olivier Certner <ocert.dev <at> free.fr>
Subject: Re: bug#44100: 26.3; ERC: Make IBuffer custom display work again
Date: Fri, 04 Jun 2021 15:58:48 -0400

Lars Ingebrigtsen writes:

> Amin Bandali <bandali <at> 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.

Message #46 received at 44100 <at> debbugs.gnu.org (full text, mbox):

From: Olivier Certner <olce.emacs <at> certner.fr>
To: 44100 <at> debbugs.gnu.org
Subject: Updated patch
Date: Tue, 06 Jul 2021 14:06:32 +0200

[Message part 1 (text/plain, inline)]

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

[0001-ERC-Unbreak-ERC-s-IBuffer-s-custom-displays-Bug-4410.patch (text/x-patch, attachment)]

Message #49 received at 44100 <at> debbugs.gnu.org (full text, mbox):

From: Olivier Certner <olce.emacs <at> certner.fr>
To: 44100 <at> debbugs.gnu.org
Cc: Amin Bandali <bandali <at> gnu.org>
Subject: On Amin's proposed change
Date: Tue, 06 Jul 2021 16:29:43 +0200

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

Message #54 received at 44100-done <at> debbugs.gnu.org (full text, mbox):

From: Amin Bandali <bandali <at> gnu.org>
To: Olivier Certner <olce.emacs <at> certner.fr>, 44100-done <at> debbugs.gnu.org
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, emacs-erc <at> gnu.org
Subject: Re: bug#44100: 26.3; ERC: Make IBuffer custom display work again
Date: Mon, 02 Aug 2021 22:52:29 -0400

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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.