* bug#19363: Acknowledgement (24.4.1; Notifications can make ERC unusable)
2014-12-31 8:59 ` Dima Kogan
@ 2014-12-31 15:47 ` Dima Kogan
2015-01-04 19:52 ` Stefan Monnier
2014-12-31 17:46 ` Dima Kogan
1 sibling, 1 reply; 14+ messages in thread
From: Dima Kogan @ 2014-12-31 15:47 UTC (permalink / raw)
To: 19363
[-- Attachment #1: Type: text/plain, Size: 1581 bytes --]
Here's a solution to THIS bug, for real this time. I'm attaching a
tested-working patch. I'm no longer sure this is a regression since it's
very sensitive to load order. In emacs 24.3 if ERC is byte-compiled then
the bug manifests, but if it isn't then it works ok. In any case, the
patch solves the issue regardless.
To reiterate, ERC works if you start emacs with no init file, and then
evaluate
(require 'erc)
(require 'erc-desktop-notifications)
(erc-notifications-enable)
However ERC does NOT work if you start with just this in the init file:
(eval-after-load 'erc
'(progn
(require 'erc-desktop-notifications)
(erc-notifications-enable)
))
The issue is that in the working case, the value of
erc-server-PRIVMSG-functions ends up as
(erc-notifications-PRIVMSG erc-auto-query erc-server-PRIVMSG)
and in the broken case as
(erc-auto-query erc-notifications-PRIVMSG)
erc-server-PRIVMSG is important, so ERC does not work correctly if it is
missing. This missing element is normally added in
erc-backend.el by
(define-erc-response-handler (PRIVMSG NOTICE) ...
which is a macro. In the macro, the significant line is
(defvar ,hook-name ',fn-name ,(format hook-doc name))
If we have some (eval-after-load 'erc ...) stuff then by the time this
(defvar) is evaluated, the list may already have a value, so the defvar
then does NOT add its value to the list. The patch explicitly changes
the (defvar list default) idiom to
(defvar list nil) (add-to-list 'list default) and thus the default value
always appears in the list.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-ERC-no-longer-gets-confused-by-eval-after-load-erc.patch --]
[-- Type: text/x-diff, Size: 1575 bytes --]
From 28bc9430ce6342d210e986586af7b6f12e103043 Mon Sep 17 00:00:00 2001
From: Dima Kogan <dima@secretsauce.net>
Date: Wed, 31 Dec 2014 08:13:57 -0800
Subject: [PATCH] ERC no longer gets confused by (eval-after-load 'erc ...)
ERC was initializing one of its lists with (defvar list default). If
the list already had a value due to (eval-after-load 'erc ...) for
instance, then (defvar) would see an initialized variable, and would NOT
add the default value to the list. This was breaking things. This
patch changes the above defvar idiom to
(defvar list nil)
(add-to-list 'list default)
This way the default value is added to the list unconditionally
Closes: #19363
---
lisp/erc/erc-backend.el | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/lisp/erc/erc-backend.el b/lisp/erc/erc-backend.el
index fa95d7e..43e56c0 100644
--- a/lisp/erc/erc-backend.el
+++ b/lisp/erc/erc-backend.el
@@ -1179,8 +1179,11 @@ add things to `%s' instead."
(cl-loop for alias in aliases
collect (intern (format "erc-server-%s-functions" alias)))))
`(prog2
- ;; Normal hook variable.
- (defvar ,hook-name ',fn-name ,(format hook-doc name))
+ ;; Normal hook variable. The variable may already have a
+ ;; value at this point, so I default to nil, and (add-hook)
+ ;; unconditionally
+ (defvar ,hook-name nil ,(format hook-doc name))
+ (add-to-list ',hook-name ',fn-name)
;; Handler function
(defun ,fn-name (proc parsed)
,fn-doc
--
2.1.3
^ permalink raw reply related [flat|nested] 14+ messages in thread* bug#19363: Acknowledgement (24.4.1; Notifications can make ERC unusable)
2014-12-31 15:47 ` Dima Kogan
@ 2015-01-04 19:52 ` Stefan Monnier
2015-01-04 21:26 ` Dima Kogan
0 siblings, 1 reply; 14+ messages in thread
From: Stefan Monnier @ 2015-01-04 19:52 UTC (permalink / raw)
To: Dima Kogan; +Cc: 19363
> If we have some (eval-after-load 'erc ...) stuff then by the time this
> (defvar) is evaluated, the list may already have a value, so the defvar
> then does NOT add its value to the list. The patch explicitly changes
> the (defvar list default) idiom to
> (defvar list nil) (add-to-list 'list default) and thus the default value
> always appears in the list.
Thanks. That's a good change.
The general rule is that a hook's initial/default value should be nil
(that's fairly strong "should", with relatively few exceptions.
This bug-report is a good example of why it should be that way).
It's also generally better if the hook's "normal" value is nil, tho
I think in this case it'd be probably inconvenient.
But I do wonder: if the function *has* to be on that hook for ERC to
work correctly, then maybe that function's place is not in the hook but
right at those places where the hook is run (i.e. hard-coded).
Could you (or someone who understands the code better than I do) take
a look to see if such a change would be even better?
Stefan
>> From 28bc9430ce6342d210e986586af7b6f12e103043 Mon Sep 17 00:00:00 2001
> From: Dima Kogan <dima@secretsauce.net>
> Date: Wed, 31 Dec 2014 08:13:57 -0800
> Subject: [PATCH] ERC no longer gets confused by (eval-after-load 'erc ...)
> ERC was initializing one of its lists with (defvar list default). If
> the list already had a value due to (eval-after-load 'erc ...) for
> instance, then (defvar) would see an initialized variable, and would NOT
> add the default value to the list. This was breaking things. This
> patch changes the above defvar idiom to
> (defvar list nil)
> (add-to-list 'list default)
> This way the default value is added to the list unconditionally
> Closes: #19363
> ---
> lisp/erc/erc-backend.el | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
> diff --git a/lisp/erc/erc-backend.el b/lisp/erc/erc-backend.el
> index fa95d7e..43e56c0 100644
> --- a/lisp/erc/erc-backend.el
> +++ b/lisp/erc/erc-backend.el
> @@ -1179,8 +1179,11 @@ add things to `%s' instead."
> (cl-loop for alias in aliases
> collect (intern (format "erc-server-%s-functions" alias)))))
> `(prog2
> - ;; Normal hook variable.
> - (defvar ,hook-name ',fn-name ,(format hook-doc name))
> + ;; Normal hook variable. The variable may already have a
> + ;; value at this point, so I default to nil, and (add-hook)
> + ;; unconditionally
> + (defvar ,hook-name nil ,(format hook-doc name))
> + (add-to-list ',hook-name ',fn-name)
> ;; Handler function
> (defun ,fn-name (proc parsed)
> ,fn-doc
> --
> 2.1.3
^ permalink raw reply [flat|nested] 14+ messages in thread* bug#19363: Acknowledgement (24.4.1; Notifications can make ERC unusable)
2015-01-04 19:52 ` Stefan Monnier
@ 2015-01-04 21:26 ` Dima Kogan
2015-01-05 1:54 ` Stefan Monnier
0 siblings, 1 reply; 14+ messages in thread
From: Dima Kogan @ 2015-01-04 21:26 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 19363
Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> If we have some (eval-after-load 'erc ...) stuff then by the time this
>> (defvar) is evaluated, the list may already have a value, so the defvar
>> then does NOT add its value to the list. The patch explicitly changes
>> the (defvar list default) idiom to
>> (defvar list nil) (add-to-list 'list default) and thus the default value
>> always appears in the list.
>
> But I do wonder: if the function *has* to be on that hook for ERC to
> work correctly, then maybe that function's place is not in the hook but
> right at those places where the hook is run (i.e. hard-coded).
> Could you (or someone who understands the code better than I do) take
> a look to see if such a change would be even better?
Hi. I suspect that hard-coding this would be a very big and unwelcome
change in this case because there's some indirection here. Each ERC
"response handler" has a hook such as this. The (defvar ...)
(add-to-list ...) happens in the macro that defines a response handler:
define-erc-response-handler, and the hooks are invoked in a general way
for all server responses in erc-handle-parsed-server-response.
So the bug I was seeing was due specifically to missing PRIVMSG response
handlers, but the patch fixes all response handlers generically.
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#19363: Acknowledgement (24.4.1; Notifications can make ERC unusable)
2015-01-04 21:26 ` Dima Kogan
@ 2015-01-05 1:54 ` Stefan Monnier
2015-01-07 0:38 ` Dima Kogan
0 siblings, 1 reply; 14+ messages in thread
From: Stefan Monnier @ 2015-01-05 1:54 UTC (permalink / raw)
To: Dima Kogan; +Cc: 19363
> Hi. I suspect that hard-coding this would be a very big and unwelcome
> change in this case because there's some indirection here. Each ERC
> "response handler" has a hook such as this. The (defvar ...)
> (add-to-list ...) happens in the macro that defines a response handler:
> define-erc-response-handler, and the hooks are invoked in a general way
> for all server responses in erc-handle-parsed-server-response.
> So the bug I was seeing was due specifically to missing PRIVMSG response
> handlers, but the patch fixes all response handlers generically.
Right. So maybe it can be fixed "better" via a more significant
restructuring, but that's still to be shown. Feel free to install your
patch, thank you,
Stefan
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#19363: Acknowledgement (24.4.1; Notifications can make ERC unusable)
2014-12-31 8:59 ` Dima Kogan
2014-12-31 15:47 ` Dima Kogan
@ 2014-12-31 17:46 ` Dima Kogan
1 sibling, 0 replies; 14+ messages in thread
From: Dima Kogan @ 2014-12-31 17:46 UTC (permalink / raw)
To: 19363
[-- Attachment #1: Type: text/plain, Size: 1581 bytes --]
Here's a solution to THIS bug, for real this time. I'm attaching a
tested-working patch. I'm no longer sure this is a regression since it's
very sensitive to load order. In emacs 24.3 if ERC is byte-compiled then
the bug manifests, but if it isn't then it works ok. In any case, the
patch solves the issue regardless.
To reiterate, ERC works if you start emacs with no init file, and then
evaluate
(require 'erc)
(require 'erc-desktop-notifications)
(erc-notifications-enable)
However ERC does NOT work if you start with just this in the init file:
(eval-after-load 'erc
'(progn
(require 'erc-desktop-notifications)
(erc-notifications-enable)
))
The issue is that in the working case, the value of
erc-server-PRIVMSG-functions ends up as
(erc-notifications-PRIVMSG erc-auto-query erc-server-PRIVMSG)
and in the broken case as
(erc-auto-query erc-notifications-PRIVMSG)
erc-server-PRIVMSG is important, so ERC does not work correctly if it is
missing. This missing element is normally added in
erc-backend.el by
(define-erc-response-handler (PRIVMSG NOTICE) ...
which is a macro. In the macro, the significant line is
(defvar ,hook-name ',fn-name ,(format hook-doc name))
If we have some (eval-after-load 'erc ...) stuff then by the time this
(defvar) is evaluated, the list may already have a value, so the defvar
then does NOT add its value to the list. The patch explicitly changes
the (defvar list default) idiom to
(defvar list nil) (add-to-list 'list default) and thus the default value
always appears in the list.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-ERC-no-longer-gets-confused-by-eval-after-load-erc.patch --]
[-- Type: text/x-diff, Size: 1575 bytes --]
From 28bc9430ce6342d210e986586af7b6f12e103043 Mon Sep 17 00:00:00 2001
From: Dima Kogan <dima@secretsauce.net>
Date: Wed, 31 Dec 2014 08:13:57 -0800
Subject: [PATCH] ERC no longer gets confused by (eval-after-load 'erc ...)
ERC was initializing one of its lists with (defvar list default). If
the list already had a value due to (eval-after-load 'erc ...) for
instance, then (defvar) would see an initialized variable, and would NOT
add the default value to the list. This was breaking things. This
patch changes the above defvar idiom to
(defvar list nil)
(add-to-list 'list default)
This way the default value is added to the list unconditionally
Closes: #19363
---
lisp/erc/erc-backend.el | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/lisp/erc/erc-backend.el b/lisp/erc/erc-backend.el
index fa95d7e..43e56c0 100644
--- a/lisp/erc/erc-backend.el
+++ b/lisp/erc/erc-backend.el
@@ -1179,8 +1179,11 @@ add things to `%s' instead."
(cl-loop for alias in aliases
collect (intern (format "erc-server-%s-functions" alias)))))
`(prog2
- ;; Normal hook variable.
- (defvar ,hook-name ',fn-name ,(format hook-doc name))
+ ;; Normal hook variable. The variable may already have a
+ ;; value at this point, so I default to nil, and (add-hook)
+ ;; unconditionally
+ (defvar ,hook-name nil ,(format hook-doc name))
+ (add-to-list ',hook-name ',fn-name)
;; Handler function
(defun ,fn-name (proc parsed)
,fn-doc
--
2.1.3
^ permalink raw reply related [flat|nested] 14+ messages in thread