bug#19363: 24.4.1; Notifications can make ERC unusable

bug#19363: 24.4.1; Notifications can make ERC unusable

[Dima Kogan]

Hi.

I just updated my Debian install from emacs24 24.3+1-5 -> 24.4+1-4, and
my ERC is no longer usable as a result.

The failure mode is that anything I type into a channel IS sent
correctly, but nothing anybody else types shows up in the buffer. So
even in an active channel it looks like I'm the only one there, since I
see nothing anybody else types. The messages do arrive to ERC, since the
notification callback sees them; they just aren't relayed to the buffer.

This is 100% reproducible if you start emacs with just the following in
your .emacs:

 (eval-after-load 'erc
   '(progn
      (require 'erc-desktop-notifications)
      (erc-notifications-enable)
      ))

Then M-x erc and connect to any server and any channel. This is
sensitive to order and/or to timing. For instance, running emacs -Q and
then evaluating

 (require 'erc-desktop-notifications)
 (erc-notifications-enable)

does not trigger the issue. I tried debugging this, but all the
indirection in the sources makes it difficult.

bug#19363: Acknowledgement (24.4.1; Notifications can make ERC unusable)

[Dima Kogan]

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

Here's a patch. It works, but I don't like how un-future-proof it is. An
uncareful coder can simply use (forward-word) somewhere and get things
to break again. Is there a better way?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-ERC-no-longer-gets-confused-by-subword-mode.patch --]
[-- Type: text/x-diff, Size: 3354 bytes --]

From 655e7b9b5735bd62aac104645b5a224636ab97ff Mon Sep 17 00:00:00 2001
From: Dima Kogan <dima@secretsauce.net>
Date: Tue, 30 Dec 2014 23:29:21 -0800
Subject: [PATCH] ERC no longer gets confused by subword-mode

In commit 6ddc44225e743e2b2a0d5c192f50aefd7a4a915b subword-mode was
integrated into the syntax table instead of simply remapping the
interactive motion bindings as was done previously.  This had the
unintended effect of changing the behavior of lisp programs that touch
words.  In the case of ERC, it completely broke it: emacs now throws an
error when ERC is launched, making it unusable when subword-mode is
active.

This commit temporarily disables subword-mode so that ERC functions that
touch words do so the "normal" way.

Closes: #17558
---
 lisp/erc/erc-backend.el |  9 ++++++++-
 lisp/erc/erc-button.el  | 23 +++++++++++++++--------
 2 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/lisp/erc/erc-backend.el b/lisp/erc/erc-backend.el
index bedb20f..fa95d7e 100644
--- a/lisp/erc/erc-backend.el
+++ b/lisp/erc/erc-backend.el
@@ -480,7 +480,14 @@ Currently this is called by `erc-send-input'."
   (with-temp-buffer
     (insert str)
     (goto-char (point-min))
-    (upcase-word 1)
+
+    ;; If we're in subword-mode then functions operating on words act
+    ;; differently. Here I temporarily disable subword-mode before
+    ;; touching the words
+    (let ((find-word-boundary-function-table
+           (if (boundp 'subword-empty-char-table)
+               subword-empty-char-table find-word-boundary-function-table)))
+      (upcase-word 1))
     (buffer-string)))
 
 (defun erc-server-setup-periodical-ping (buffer)
diff --git a/lisp/erc/erc-button.el b/lisp/erc/erc-button.el
index 10e7318..8343425 100644
--- a/lisp/erc/erc-button.el
+++ b/lisp/erc/erc-button.el
@@ -300,14 +300,21 @@ specified by `erc-button-alist'."
     (when (or (eq t form)
               (eval form))
       (goto-char (point-min))
-      (while (forward-word 1)
-        (setq bounds (bounds-of-thing-at-point 'word))
-        (setq word (buffer-substring-no-properties
-                    (car bounds) (cdr bounds)))
-        (when (or (and (erc-server-buffer-p) (erc-get-server-user word))
-                  (and erc-channel-users (erc-get-channel-user word)))
-          (erc-button-add-button (car bounds) (cdr bounds)
-                                 fun t (list word)))))))
+
+      ;; If we're in subword-mode then functions operating on words
+      ;; act differently. Here I temporarily disable subword-mode
+      ;; before touching the words
+      (let ((find-word-boundary-function-table
+             (if (boundp 'subword-empty-char-table)
+                 subword-empty-char-table find-word-boundary-function-table)))
+        (while (forward-word 1)
+          (setq bounds (bounds-of-thing-at-point 'word))
+          (setq word (buffer-substring-no-properties
+                      (car bounds) (cdr bounds)))
+          (when (or (and (erc-server-buffer-p) (erc-get-server-user word))
+                    (and erc-channel-users (erc-get-channel-user word)))
+            (erc-button-add-button (car bounds) (cdr bounds)
+                                   fun t (list word))))))))
 
 (defun erc-button-add-buttons-1 (regexp entry)
   "Search through the buffer for matches to ENTRY and add buttons."
-- 
2.1.3

bug#19363: Acknowledgement (24.4.1; Notifications can make ERC unusable)

[Dima Kogan]

Dima Kogan <dima@secretsauce.net> writes:

> Here's a patch. It works, but I don't like how un-future-proof it is. An
> uncareful coder can simply use (forward-word) somewhere and get things
> to break again. Is there a better way?

Oops; this was meant as a response to a different bug. Please disregard.

bug#19363: Acknowledgement (24.4.1; Notifications can make ERC unusable)

[Dima Kogan]

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

bug#19363: Acknowledgement (24.4.1; Notifications can make ERC unusable)

[Dima Kogan]

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

bug#19363: Acknowledgement (24.4.1; Notifications can make ERC unusable)

[Stefan Monnier]

> 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

bug#19363: Acknowledgement (24.4.1; Notifications can make ERC unusable)

[Dima Kogan]

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.

bug#19363: Acknowledgement (24.4.1; Notifications can make ERC unusable)

[Stefan Monnier]

> 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

bug#19363: Acknowledgement (24.4.1; Notifications can make ERC unusable)

[Dima Kogan]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Feel free to install your patch, thank you

Thanks, Stefan. I don't have commit access. Can somebody push up that
change, please?

bug#19363: Close debbugs #19363

[Kelvin White]

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

Hello Dima,

Your patch has been installed in the emacs-24 branch.

Thanks

[-- Attachment #2: Type: text/html, Size: 948 bytes --]

bug#19363: Close debbugs #19363

[Kelvin White]

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

patch installed. This bug report is closed.

[-- Attachment #2: Type: text/html, Size: 107 bytes --]

bug#19363: Close debbugs #19363

[Glenn Morris]

FYI, you haven't closed the bug report.
You do that by sending a mail to 19363-done.
(It may help to read the quick-start guide in admin/notes/bugtracker.)

bug#19363: Close debbugs #19363

[Kelvin White]

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

I couldn't remember where I read that before, I was looking in admin and
got sidetracked with work.

Thanks Glenn

On Thu, Jan 15, 2015, 1:15 PM Glenn Morris <rgm@gnu.org> wrote:

>
> FYI, you haven't closed the bug report.
> You do that by sending a mail to 19363-done.
> (It may help to read the quick-start guide in admin/notes/bugtracker.)
>
>

[-- Attachment #2: Type: text/html, Size: 600 bytes --]

bug#19363:

[Kelvin White]

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

patch installed

[-- Attachment #2: Type: text/html, Size: 16 bytes --]