bug#51753: ERC switches to channel buffer on reconnect

bug#51753: ERC switches to channel buffer on reconnect

[Stefan Kangas]

Severity: important

When ERC reconnects, it switches buffer to the channel buffers.  This is
*very* dangerous.

Consider the situation when the user is about to paste a password and
hit enter with a quick "C-y RET", when all of a sudden the ERC buffer
pops up a fraction of a second before you hit those keys.  Now your
password is on IRC.

At the very least, this behavior should be disabled by default, and
preferably also come with a big warning sign for anyone that intends to
enable it.

bug#51753: ERC switches to channel buffer on reconnect

[J.P.]

Stefan Kangas <stefan@marxist.se> writes:

> Severity: important
>
> When ERC reconnects, it switches buffer to the channel buffers.  This is
> *very* dangerous.

Hi Stefan. Any idea if this is a recent phenomenon or more along the
lines of traditional behavior, perhaps involving `erc-join-buffer', for
example? I ask this mostly out of self interest re

  commit 17e7cab1507a185d2c493548494abcad6a55d159
  Normalize usage of variable erc-server-reconnecting

and its followup

  commit ec9ddd6eaf3f1b118d3ce95a69e1d046166362d3
  Deprecate instead of redefine erc-server-reconnecting

(both recent and both mine).

Regardless, there's a solid chance what you're experiencing is
`erc-setup-buffer' related, which comes into play whenever `erc-open'
runs. For reconnects, this happens both during `erc-server-reconnect'
and again whenever `erc-server-JOIN' runs (for channels you join).

It'd also be nice to know how these JOIN replies are being triggered
(server-initiated, erc-join.el, manual /join, etc.). With the join
module, they'd likely be the result of a JOIN command (request) sent by
`erc-autojoin-channels', which runs on 376/422. This matters when trying
to pinpoint problematic interplay with the reconnect logic, if such a
thing exists.

> Consider the situation when the user is about to paste a password and
> hit enter with a quick "C-y RET", when all of a sudden the ERC buffer
> pops up a fraction of a second before you hit those keys.  Now your
> password is on IRC.
>
> At the very least, this behavior should be disabled by default, and
> preferably also come with a big warning sign for anyone that intends to
> enable it.

I don't think many would argue that this behavior isn't at least
annoying. While specific scenarios (like accidentally broadcasting creds
to a channel) are important to confront, we should also remember to take
in the long view whenever possible (IMO). And in doing so, I think we'll
find that the many downsides of interfacing with a services bot have
over time compelled the IRC world to embrace SASL (part of the IRCv3
initiative) as the preferred means of authentication. Thanks.

bug#51753: ERC switches to channel buffer on reconnect

[Lars Ingebrigtsen]

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

> Hi Stefan. Any idea if this is a recent phenomenon or more along the
> lines of traditional behavior, perhaps involving `erc-join-buffer', for
> example?

erc has behaved this way as long as I can remember, and it's really
annoying (especially when a server bounces and erc reconnects, popping
up windows everywhere while I'm trying to get some work done).

So I'm all for making erc stopping doing this.

erc does not pop up windows when somebody messages you (but just
displays something in the mode line), and I think that's what erc should
do for all buffers (by default; there could be a user option to allow it
to pop up windows, of course).

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

bug#51753: ERC switches to channel buffer on reconnect

[J.P.]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> erc does not pop up windows when somebody messages you (but just
> displays something in the mode line), and I think that's what erc should
> do for all buffers (by default; there could be a user option to allow it
> to pop up windows, of course).

The simplest approach might be to just change the default values of
`erc-join-buffer' and `erc-auto-query' to 'bury. However, if people want
something different to happen when automatically reconnecting, we'd
probably have to remember whether `erc-server-reconnect-count' was ever
positive before crossing the logical connection threshold for the
current session.

This may come down to having `erc-connection-established' record the
count prior to resetting it (perhaps in a new, internal variable). And
then, during re-JOINs, `erc-setup-buffer' could weigh that recorded
value against some new option, like an `erc-display-reconnect' (or
whatever), and proceed accordingly.

bug#51753: ERC switches to channel buffer on reconnect

[Lars Ingebrigtsen]

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

> The simplest approach might be to just change the default values of
> `erc-join-buffer' and `erc-auto-query' to 'bury.

Yup.

> However, if people want something different to happen when
> automatically reconnecting, we'd probably have to remember whether
> `erc-server-reconnect-count' was ever positive before crossing the
> logical connection threshold for the current session.
>
> This may come down to having `erc-connection-established' record the
> count prior to resetting it (perhaps in a new, internal variable). And
> then, during re-JOINs, `erc-setup-buffer' could weigh that recorded
> value against some new option, like an `erc-display-reconnect' (or
> whatever), and proceed accordingly.

Sounds good to me.

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

bug#51753: ERC switches to channel buffer on reconnect

[J.P.]

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

Lars Ingebrigtsen <larsi@gnus.org> writes:

>> However, if people want something different to happen when
>> automatically reconnecting, we'd probably have to remember whether
>> `erc-server-reconnect-count' was ever positive before crossing the
>> logical connection threshold for the current session.
>>
>> This may come down to having `erc-connection-established' record the
>> count prior to resetting it (perhaps in a new, internal variable). And
>> then, during re-JOINs, `erc-setup-buffer' could weigh that recorded
>> value against some new option, like an `erc-display-reconnect' (or
>> whatever), and proceed accordingly.
>
> Sounds good to me.

I wasn't sure if that meant I was supposed to work on this. If not,
please disregard.

Otherwise, the tests are in #48598 [1]. As for the name of the option
itself, I basically punted by going with `erc-reconnect-buffer' to try
and stay close to `erc-join-buffer'. If that doesn't matter, perhaps
`erc-reconnect-display' would be a better fit since we already have an
`erc-query-display' (even though that one's not as closely related).
Anyone with an opinion, please advise. Thanks.


[1] Around line 4736:

    https://gitlab.com/jpneverwas/erc-tools/-/raw/master/bugs/48598/patches/wip/0013-Update-ERC-scenarios-with-session-centric-naming.patch

    Or browsable (JS):

    https://gitlab.com/jpneverwas/erc-v3/-/blob/1333bda3c0d11ff06b1b2acbb27864c90d5ba303/test/erc-scenarios.el#L1668


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Customize-displaying-of-ERC-buffers-on-reconnect.patch --]
[-- Type: text/x-patch, Size: 3415 bytes --]

From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
Date: Thu, 18 Nov 2021 23:39:54 -0800
Subject: [PATCH 01/29] Customize displaying of ERC buffers on reconnect

* lisp/erc/erc-backend.el (erc--server-last-reconnect-count):
Add variable to record last reconnect tally.

* lisp/erc/erc.el (erc-reconnect-buffer): Add option to specify
channel-buffer display behavior on reconnect.
(erc-setup-buffer): Use option `erc-reconnect-buffer' if warranted.
(erc-connection-established): Record reconnect count in internal var
before resetting.
---
 lisp/erc/erc-backend.el |  3 +++
 lisp/erc/erc.el         | 24 ++++++++++++++++++++++--
 2 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/lisp/erc/erc-backend.el b/lisp/erc/erc-backend.el
index 69f63dfbc4..4b39bd8a30 100644
--- a/lisp/erc/erc-backend.el
+++ b/lisp/erc/erc-backend.el
@@ -193,6 +193,9 @@ erc-server-connected
 (defvar-local erc-server-reconnect-count 0
   "Number of times we have failed to reconnect to the current server.")
 
+(defvar-local erc--server-last-reconnect-count 0
+  "Snapshot of reconnect count when connection established.")
+
 (defvar-local erc-server-quitting nil
   "Non-nil if the user requests a quit.")
 
diff --git a/lisp/erc/erc.el b/lisp/erc/erc.el
index c5a4fbe5a0..21ae25d920 100644
--- a/lisp/erc/erc.el
+++ b/lisp/erc/erc.el
@@ -1521,6 +1521,22 @@ erc-join-buffer
                  (const :tag "Use current buffer" buffer)
                  (const :tag "Use current buffer" t)))
 
+(defcustom erc-reconnect-buffer nil
+  "How (and whether) to display a channel buffer upon reconnecting.
+
+This only affects automatic reconnections and is ignored when issuing a
+/reconnect command or reinvoking `erc-tls' with the same args (assuming
+success, of course).  See `erc-join-buffer' for a description of
+possible values."
+  :group 'erc-buffers
+  :type '(choice (const :tag "Use value of `erc-join-buffer'" nil)
+                 (const :tag "Split window and select" window)
+                 (const :tag "Split window, don't select" window-noselect)
+                 (const :tag "New frame" frame)
+                 (const :tag "Bury in new buffer" bury)
+                 (const :tag "Use current buffer" buffer)
+                 (const :tag "Use current buffer" t)))
+
 (defcustom erc-frame-alist nil
   "Alist of frame parameters for creating erc frames.
 A value of nil means to use `default-frame-alist'."
@@ -1950,7 +1966,10 @@ erc-update-modules
 
 (defun erc-setup-buffer (buffer)
   "Consults `erc-join-buffer' to find out how to display `BUFFER'."
-  (pcase erc-join-buffer
+  (pcase (if (zerop (erc-with-server-buffer
+                      erc--server-last-reconnect-count))
+             erc-join-buffer
+           (or erc-reconnect-buffer erc-join-buffer))
     ('window
      (if (active-minibuffer-window)
          (display-buffer buffer)
@@ -4722,7 +4741,8 @@ erc-connection-established
             (nick (car (erc-response.command-args parsed)))
             (buffer (process-buffer proc)))
         (setq erc-server-connected t)
-	(setq erc-server-reconnect-count 0)
+        (setq erc--server-last-reconnect-count erc-server-reconnect-count
+              erc-server-reconnect-count 0)
         (erc-update-mode-line)
         (erc-set-initial-user-mode nick buffer)
         (erc-server-setup-periodical-ping buffer)
-- 
2.31.1

bug#51753: ERC switches to channel buffer on reconnect

[Stefan Kangas]

tags 51753 + patch
thanks

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

> I wasn't sure if that meant I was supposed to work on this. If not,
> please disregard.

You did exactly the right thing here.  Thank you for working on this!

> Otherwise, the tests are in #48598 [1]. As for the name of the option
> itself, I basically punted by going with `erc-reconnect-buffer' to try
> and stay close to `erc-join-buffer'. If that doesn't matter, perhaps
> `erc-reconnect-display' would be a better fit since we already have an
> `erc-query-display' (even though that one's not as closely related).
> Anyone with an opinion, please advise. Thanks.

On balance, I don't have a strong opinion either way.  (I do like
`erc-reconnect-display' slightly better, because many variables that end
with `-buffer' have to do with the _name_ of some buffer.  But it is
true that it is also nice to have a name similar to `erc-join-buffer'.)

I just have some minor nits below:

> +(defvar-local erc--server-last-reconnect-count 0
> +  "Snapshot of reconnect count when connection established.")

"when the connection was established", perhaps?

> +(defcustom erc-reconnect-buffer nil
> +  "How (and whether) to display a channel buffer upon reconnecting.
> +
> +This only affects automatic reconnections and is ignored when issuing a
> +/reconnect command or reinvoking `erc-tls' with the same args (assuming
> +success, of course).  See `erc-join-buffer' for a description of
> +possible values."
> +  :group 'erc-buffers
> +  :type '(choice (const :tag "Use value of `erc-join-buffer'" nil)
> +                 (const :tag "Split window and select" window)
> +                 (const :tag "Split window, don't select" window-noselect)
> +                 (const :tag "New frame" frame)
> +                 (const :tag "Bury in new buffer" bury)
> +                 (const :tag "Use current buffer" buffer)
> +                 (const :tag "Use current buffer" t)))

What is the difference between `buffer' and t?  Should they really have
the same :tag?

If they are just two names for the same thing, I suggest we drop one of
them.

Other than that, LGTM (but I didn't test it).

bug#51753: ERC switches to channel buffer on reconnect

[J.P.]

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

Stefan Kangas <stefan@marxist.se> writes:

> On balance, I don't have a strong opinion either way.  (I do like
> `erc-reconnect-display' slightly better, because many variables that end
> with `-buffer' have to do with the _name_ of some buffer.  But it is
> true that it is also nice to have a name similar to `erc-join-buffer'.)

Screw it. Let's go with `erc-reconnect-display'.

> I just have some minor nits below:
>
>> +(defvar-local erc--server-last-reconnect-count 0
>> +  "Snapshot of reconnect count when connection established.")
>
> "when the connection was established", perhaps?

Now reads as ^.

> What is the difference between `buffer' and t?  Should they really have
> the same :tag?

No idea to both, unfortunately. But since the option is only used in
that one function (`erc-setup-buffer'), and since we don't claim
like-for-like compatibility with `erc-join-buffer' (which we've already
deviated from anyway by offering a choice of nil) ...

> I suggest we drop one of them.

Agreed. Thanks.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0000-v1-v2.diff --]
[-- Type: text/x-patch, Size: 2313 bytes --]

From d7e9871283cfc6f0e841adf99251ae7057d39d7b Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
Date: Fri, 19 Nov 2021 04:41:50 -0800
Subject: NOT A PATCH


F. Jason Park (1):
  Customize displaying of ERC buffers on reconnect

 lisp/erc/erc-backend.el |  3 +++
 lisp/erc/erc.el         | 23 +++++++++++++++++++++--
 2 files changed, 24 insertions(+), 2 deletions(-)

Interdiff:
diff --git a/lisp/erc/erc-backend.el b/lisp/erc/erc-backend.el
index 4b39bd8a30..0d586268e9 100644
--- a/lisp/erc/erc-backend.el
+++ b/lisp/erc/erc-backend.el
@@ -194,7 +194,7 @@ erc-server-reconnect-count
   "Number of times we have failed to reconnect to the current server.")
 
 (defvar-local erc--server-last-reconnect-count 0
-  "Snapshot of reconnect count when connection established.")
+  "Snapshot of reconnect count when the connection was established.")
 
 (defvar-local erc-server-quitting nil
   "Non-nil if the user requests a quit.")
diff --git a/lisp/erc/erc.el b/lisp/erc/erc.el
index 21ae25d920..01be8ed397 100644
--- a/lisp/erc/erc.el
+++ b/lisp/erc/erc.el
@@ -1521,7 +1521,7 @@ erc-join-buffer
                  (const :tag "Use current buffer" buffer)
                  (const :tag "Use current buffer" t)))
 
-(defcustom erc-reconnect-buffer nil
+(defcustom erc-reconnect-display nil
   "How (and whether) to display a channel buffer upon reconnecting.
 
 This only affects automatic reconnections and is ignored when issuing a
@@ -1534,8 +1534,7 @@ erc-reconnect-buffer
                  (const :tag "Split window, don't select" window-noselect)
                  (const :tag "New frame" frame)
                  (const :tag "Bury in new buffer" bury)
-                 (const :tag "Use current buffer" buffer)
-                 (const :tag "Use current buffer" t)))
+                 (const :tag "Use current buffer" buffer)))
 
 (defcustom erc-frame-alist nil
   "Alist of frame parameters for creating erc frames.
@@ -1969,7 +1968,7 @@ erc-setup-buffer
   (pcase (if (zerop (erc-with-server-buffer
                       erc--server-last-reconnect-count))
              erc-join-buffer
-           (or erc-reconnect-buffer erc-join-buffer))
+           (or erc-reconnect-display erc-join-buffer))
     ('window
      (if (active-minibuffer-window)
          (display-buffer buffer)
-- 
2.31.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0001-Customize-displaying-of-ERC-buffers-on-reconnect.patch --]
[-- Type: text/x-patch, Size: 3368 bytes --]

From d7e9871283cfc6f0e841adf99251ae7057d39d7b Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
Date: Thu, 18 Nov 2021 23:39:54 -0800
Subject: [PATCH 1/1] Customize displaying of ERC buffers on reconnect

* lisp/erc/erc-backend.el (erc--server-last-reconnect-count):
Add variable to record last reconnect tally.

* lisp/erc/erc.el (erc-reconnect-buffer): Add option to specify
channel-buffer display behavior on reconnect.
(erc-setup-buffer): Use option `erc-reconnect-buffer' if warranted.
(erc-connection-established): Record reconnect count in internal var
before resetting.
---
 lisp/erc/erc-backend.el |  3 +++
 lisp/erc/erc.el         | 23 +++++++++++++++++++++--
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/lisp/erc/erc-backend.el b/lisp/erc/erc-backend.el
index 69f63dfbc4..0d586268e9 100644
--- a/lisp/erc/erc-backend.el
+++ b/lisp/erc/erc-backend.el
@@ -193,6 +193,9 @@ erc-server-connected
 (defvar-local erc-server-reconnect-count 0
   "Number of times we have failed to reconnect to the current server.")
 
+(defvar-local erc--server-last-reconnect-count 0
+  "Snapshot of reconnect count when the connection was established.")
+
 (defvar-local erc-server-quitting nil
   "Non-nil if the user requests a quit.")
 
diff --git a/lisp/erc/erc.el b/lisp/erc/erc.el
index c5a4fbe5a0..01be8ed397 100644
--- a/lisp/erc/erc.el
+++ b/lisp/erc/erc.el
@@ -1521,6 +1521,21 @@ erc-join-buffer
                  (const :tag "Use current buffer" buffer)
                  (const :tag "Use current buffer" t)))
 
+(defcustom erc-reconnect-display nil
+  "How (and whether) to display a channel buffer upon reconnecting.
+
+This only affects automatic reconnections and is ignored when issuing a
+/reconnect command or reinvoking `erc-tls' with the same args (assuming
+success, of course).  See `erc-join-buffer' for a description of
+possible values."
+  :group 'erc-buffers
+  :type '(choice (const :tag "Use value of `erc-join-buffer'" nil)
+                 (const :tag "Split window and select" window)
+                 (const :tag "Split window, don't select" window-noselect)
+                 (const :tag "New frame" frame)
+                 (const :tag "Bury in new buffer" bury)
+                 (const :tag "Use current buffer" buffer)))
+
 (defcustom erc-frame-alist nil
   "Alist of frame parameters for creating erc frames.
 A value of nil means to use `default-frame-alist'."
@@ -1950,7 +1965,10 @@ erc-update-modules
 
 (defun erc-setup-buffer (buffer)
   "Consults `erc-join-buffer' to find out how to display `BUFFER'."
-  (pcase erc-join-buffer
+  (pcase (if (zerop (erc-with-server-buffer
+                      erc--server-last-reconnect-count))
+             erc-join-buffer
+           (or erc-reconnect-display erc-join-buffer))
     ('window
      (if (active-minibuffer-window)
          (display-buffer buffer)
@@ -4722,7 +4740,8 @@ erc-connection-established
             (nick (car (erc-response.command-args parsed)))
             (buffer (process-buffer proc)))
         (setq erc-server-connected t)
-	(setq erc-server-reconnect-count 0)
+        (setq erc--server-last-reconnect-count erc-server-reconnect-count
+              erc-server-reconnect-count 0)
         (erc-update-mode-line)
         (erc-set-initial-user-mode nick buffer)
         (erc-server-setup-periodical-ping buffer)
-- 
2.31.1

bug#51753: ERC switches to channel buffer on reconnect

[J.P.]

It just occurred to me that we may also want to reset
`erc--server-last-reconnect-count' at some point after all (re)JOINing
is done with (both client- and server-initiated) so that the option
`erc-join-buffer' regains precedence over `erc-reconnect-display' for
the remainder of the connection. Doing so in `erc-cmd-JOIN' would cover
manually issued /JOINs as well as invocations of `erc-join-channel'.

bug#51753: ERC switches to channel buffer on reconnect

[Stefan Kangas]

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

> It just occurred to me that we may also want to reset
> `erc--server-last-reconnect-count' at some point after all (re)JOINing
> is done with (both client- and server-initiated) so that the option
> `erc-join-buffer' regains precedence over `erc-reconnect-display' for
> the remainder of the connection. Doing so in `erc-cmd-JOIN' would cover
> manually issued /JOINs as well as invocations of `erc-join-channel'.

Sounds good to me.

bug#51753: ERC switches to channel buffer on reconnect

[J.P.]

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

Stefan Kangas <stefan@marxist.se> writes:

> "J.P." <jp@neverwas.me> writes:
>
>> It just occurred to me that we may also want to reset
>> `erc--server-last-reconnect-count' at some point after all (re)JOINing
>> is done with (both client- and server-initiated) so that the option
>> `erc-join-buffer' regains precedence over `erc-reconnect-display' for
>> the remainder of the connection. Doing so in `erc-cmd-JOIN' would cover
>> manually issued /JOINs as well as invocations of `erc-join-channel'.
>
> Sounds good to me.

Great. Added. Thanks.


Updated tests: https://gitlab.com/jpneverwas/erc-v3/-/blob/454ef4674af3d784bb47a81db5e7d526fb30a705/test/erc-scenarios.el#L1642



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0000-v2-v3.diff --]
[-- Type: text/x-patch, Size: 846 bytes --]

From a9c084d4dbfad1e34e23f1d2451a34beadfdc258 Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
Date: Sun, 21 Nov 2021 00:40:31 -0800
Subject: NOT A PATCH

F. Jason Park (1):
  Customize displaying of ERC buffers on reconnect

 lisp/erc/erc-backend.el |  3 +++
 lisp/erc/erc.el         | 24 ++++++++++++++++++++++--
 2 files changed, 25 insertions(+), 2 deletions(-)

Interdiff:
diff --git a/lisp/erc/erc.el b/lisp/erc/erc.el
index 01be8ed397..181da76f8e 100644
--- a/lisp/erc/erc.el
+++ b/lisp/erc/erc.el
@@ -3243,6 +3243,7 @@ erc-cmd-JOIN
             (switch-to-buffer (if (get-buffer chnl-name)
                                   chnl-name
                                 (concat chnl-name "/" server)))
+          (setq erc--server-last-reconnect-count 0)
 	  (erc-server-join-channel server chnl key)))))
   t)
 
-- 
2.31.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0001-Customize-displaying-of-ERC-buffers-on-reconnect.patch --]
[-- Type: text/x-patch, Size: 3764 bytes --]

From a9c084d4dbfad1e34e23f1d2451a34beadfdc258 Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
Date: Thu, 18 Nov 2021 23:39:54 -0800
Subject: [PATCH 1/1] Customize displaying of ERC buffers on reconnect

* lisp/erc/erc-backend.el (erc--server-last-reconnect-count):
Add variable to record last reconnect tally.

* lisp/erc/erc.el (erc-reconnect-buffer): Add option to specify
channel-buffer display behavior on reconnect.
(erc-setup-buffer): Use option `erc-reconnect-buffer' if warranted.
(erc-connection-established): Record reconnect count in internal var
before resetting.
(erc-cmd-JOIN): Forget last reconnect count when issuing a manual
/JOIN command.
---
 lisp/erc/erc-backend.el |  3 +++
 lisp/erc/erc.el         | 24 ++++++++++++++++++++++--
 2 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/lisp/erc/erc-backend.el b/lisp/erc/erc-backend.el
index 69f63dfbc4..0d586268e9 100644
--- a/lisp/erc/erc-backend.el
+++ b/lisp/erc/erc-backend.el
@@ -193,6 +193,9 @@ erc-server-connected
 (defvar-local erc-server-reconnect-count 0
   "Number of times we have failed to reconnect to the current server.")
 
+(defvar-local erc--server-last-reconnect-count 0
+  "Snapshot of reconnect count when the connection was established.")
+
 (defvar-local erc-server-quitting nil
   "Non-nil if the user requests a quit.")
 
diff --git a/lisp/erc/erc.el b/lisp/erc/erc.el
index c5a4fbe5a0..181da76f8e 100644
--- a/lisp/erc/erc.el
+++ b/lisp/erc/erc.el
@@ -1521,6 +1521,21 @@ erc-join-buffer
                  (const :tag "Use current buffer" buffer)
                  (const :tag "Use current buffer" t)))
 
+(defcustom erc-reconnect-display nil
+  "How (and whether) to display a channel buffer upon reconnecting.
+
+This only affects automatic reconnections and is ignored when issuing a
+/reconnect command or reinvoking `erc-tls' with the same args (assuming
+success, of course).  See `erc-join-buffer' for a description of
+possible values."
+  :group 'erc-buffers
+  :type '(choice (const :tag "Use value of `erc-join-buffer'" nil)
+                 (const :tag "Split window and select" window)
+                 (const :tag "Split window, don't select" window-noselect)
+                 (const :tag "New frame" frame)
+                 (const :tag "Bury in new buffer" bury)
+                 (const :tag "Use current buffer" buffer)))
+
 (defcustom erc-frame-alist nil
   "Alist of frame parameters for creating erc frames.
 A value of nil means to use `default-frame-alist'."
@@ -1950,7 +1965,10 @@ erc-update-modules
 
 (defun erc-setup-buffer (buffer)
   "Consults `erc-join-buffer' to find out how to display `BUFFER'."
-  (pcase erc-join-buffer
+  (pcase (if (zerop (erc-with-server-buffer
+                      erc--server-last-reconnect-count))
+             erc-join-buffer
+           (or erc-reconnect-display erc-join-buffer))
     ('window
      (if (active-minibuffer-window)
          (display-buffer buffer)
@@ -3225,6 +3243,7 @@ erc-cmd-JOIN
             (switch-to-buffer (if (get-buffer chnl-name)
                                   chnl-name
                                 (concat chnl-name "/" server)))
+          (setq erc--server-last-reconnect-count 0)
 	  (erc-server-join-channel server chnl key)))))
   t)
 
@@ -4722,7 +4741,8 @@ erc-connection-established
             (nick (car (erc-response.command-args parsed)))
             (buffer (process-buffer proc)))
         (setq erc-server-connected t)
-	(setq erc-server-reconnect-count 0)
+        (setq erc--server-last-reconnect-count erc-server-reconnect-count
+              erc-server-reconnect-count 0)
         (erc-update-mode-line)
         (erc-set-initial-user-mode nick buffer)
         (erc-server-setup-periodical-ping buffer)
-- 
2.31.1

bug#51753: ERC switches to channel buffer on reconnect

[Stefan Kangas]

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

> Great. Added. Thanks.

Amin, what do you think of this patch?

bug#51753: ERC switches to channel buffer on reconnect

[J.P.]

These are just some related notes, mostly intended for posterity. Feel
free to ignore. Thanks.


I think at some point it'll be worth exploring the best time and place
for forgetting the current connection's origins with respect to whether
it sprang from an automated reconnect. The latest iteration of this
patch does this via `erc-cmd-JOIN'. But that's a bit of a compromise and
one that will hopefully be replaced with something smarter down the
road.

Ideally, resetting would occur immediately after all reconnect-related
JOINs have happened. As might be obvious, doing this ultimately involves
tracking more state, including but not limited to "JOINedness". In the
case of server-initiated joins, that implies doing so across sessions
[1]. For client-initiated ones, it means tracking request context. While
IRCv3 features like "label responses" were designed specifically to
assist with the latter [2], that doesn't mean we can't do so manually,
given sufficient motivation.

Indeed, when armed with a reliable way to uniquely identify a "logical"
channel subscription across connections [3], it becomes possible to do
this manually by stashing callbacks or continuation instructions. (Some
people may know this as the "command" pattern.) The real question then
becomes whether it's worth the added complexity.

BTW, if anyone has alternative ideas for a temporary solution (other
than `erc-cmd-JOIN'), please speak up. I originally thought about doing
the resetting in a subset of /CMDs originating from typed user input,
but that seemed more prone to inconveniencing users who may have long
incorporated the underlying functions into custom code (firing on
connection) [4]. One way around this would be to only do the resetting
when a user actually enters something at the prompt, but unless you want
that to happen on *any* /CMD, we'd need special handling [5] for the
handful desired (possibly chosen via configurable option).


~~~

[1] A related issue is the means of detecting whether we're joined to a
    channel. With traditional pub/sub services (IRC channels are really
    just "topics"), it's in the client's interest to stay abreast of its
    subscription status. And the service API also typically exposes a
    way for a client to query for this out of band. ERC *does* track a
    channel's JOIN'd state, but it's a bit hampered by legacy features
    related to how targets are currently handled (via
    `erc-default-recipients' and functions that access/mutate it).

[2] https://ircv3.net/irc/#labeled-responses

[3] "Logical" meaning including serially, i.e., spanning disconnects.

[4] In ERC's case, these commands aren't designated as being primarily
    intended for /interactive use (`erc-cmd-JOIN`, being one such
    function, naturally suffers from the same flaw).

[5] Hopefully something more flexible than adding/removing properties,
    like `process-not-needed', which creates problems when trying to
    override /CMDs.

bug#51753: bug#55540: 29.0.50; ERC launches autojoin-channels in current frame instead of original frame

[Lars Ingebrigtsen]

erc still pops up the buffer by default, I think?  I don't think it
should do that, because it's both pretty annoying and a security
issue -- you may suddenly be typing a password into an erc buffer.

bug#51753: bug#55540: 29.0.50; ERC launches autojoin-channels in current frame instead of original frame

[J.P.]

Hi Lars,

Lars Ingebrigtsen <larsi@gnus.org> writes:

> erc still pops up the buffer by default, I think?  I don't think it
> should do that, because it's both pretty annoying and a security
> issue -- you may suddenly be typing a password into an erc buffer.

Are you concerned about the behavior during initial connections as well
as automatic reconnections? Regardless, as you say, the default value of
`buffer' for `erc-join-buffer' (and `erc-reconnect-display') causes the
buffer in the selected window to be replaced with the
just-(re)initialized ERC buffer.

Also, depending on various factors, values other than `buffer' can
exhibit similar behavior. For example, in a dual-window split, a value
of `window-noselect' can result in a newly (re)joined channel replacing
the buffer in the selected window when the connection's server buffer is
showing in the other window. Plain `window' is much the same, but at
least there's somewhat of an expectation that the selected window's
buffer may change.

Really, the only safe option is `bury' (well, maybe also the proposed
frame stuff in Pankaj's patch, but only when an extra, ERC-specific
frame already exists, which we can't count on). That said, there's no
reason we'd have to stick with existing options/behavior when choosing a
new default. I guess it just comes down to what users want to happen.

If we're talking both `erc-join-buffer' and `erc-reconnect-display', one
idea would be to make `window-noselect' the default but change it to
mean the selected window is always left unmolested, no matter what. The
justification for the breaking change would be that the existing doc
string in

  (const :tag "Split window, don't select" window-noselect)

has always implied as much, namely that a window displaying the new
buffer will always be shown and never selected.

Although, if this only concerns `erc-reconnect-display', we could just
go with `bury' since (among the available options) that best minimizes
the disruption of a reestablished connection.

Hopefully folks have stronger opinions and/or better ideas. Thanks.

bug#51753: bug#55540: 29.0.50; ERC launches autojoin-channels in current frame instead of original frame

[Lars Ingebrigtsen]

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

> Are you concerned about the behavior during initial connections as well
> as automatic reconnections?

Yes.

> Really, the only safe option is `bury' (well, maybe also the proposed
> frame stuff in Pankaj's patch, but only when an extra, ERC-specific
> frame already exists, which we can't count on). That said, there's no
> reason we'd have to stick with existing options/behavior when choosing a
> new default. I guess it just comes down to what users want to happen.

I think the default action should be to do nothing to the window setup
at all, and just add the notification to the mode line.

bug#51753: bug#55540: 29.0.50; ERC launches autojoin-channels in current frame instead of original frame

[J.P.]

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

Lars Ingebrigtsen <larsi@gnus.org> writes:

> "J.P." <jp@neverwas.me> writes:
>
>> Really, the only safe option is `bury' (well, maybe also the proposed
>> frame stuff in Pankaj's patch, but only when an extra, ERC-specific
>> frame already exists, which we can't count on). That said, there's no
>> reason we'd have to stick with existing options/behavior when choosing a
>> new default. I guess it just comes down to what users want to happen.
>
> I think the default action should be to do nothing to the window setup
> at all, and just add the notification to the mode line.

I believe changing the default for `erc-join-buffer' (alone) to `bury'
is the easiest way to achieve that.

Thus, if no one objects within the next week or so, I will add the patch
below or similar to trunk (and what will become ERC 5.5), along with a
related NEWS entry. Thanks.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Bury-new-ERC-buffers-by-default.patch --]
[-- Type: text/x-patch, Size: 5189 bytes --]

From 8b4edbca2771227d4e6ee464ddd210080339f63b Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
Date: Tue, 6 Sep 2022 19:09:54 -0700
Subject: [PATCH] Bury new ERC buffers by default

* lisp/erc/erc.el (erc-join-buffer): Change default value to `bury'.
* test/lisp/erc/erc-scenarios-base-reconnect.el
(erc-scenarios-common-base-reconnect-options): Update helper to handle
new default value for option `erc-join-buffer'.
(erc-scenarios-base-reconnect-options--buffer): Update and rename
function `erc-scenarios-base-reconnect-options--default'.
(erc-scenarios-base-reconnect-options--default): Update and rename
function `erc-scenarios-base-reconnect-options--bury'.
---
 lisp/erc/erc.el                               |  3 +-
 test/lisp/erc/erc-scenarios-base-reconnect.el | 45 ++++++++++---------
 2 files changed, 25 insertions(+), 23 deletions(-)

diff --git a/lisp/erc/erc.el b/lisp/erc/erc.el
index 151d75e7ce..657ef8cdb3 100644
--- a/lisp/erc/erc.el
+++ b/lisp/erc/erc.el
@@ -1651,7 +1651,7 @@ erc-default-port-tls
   "IRC port to use for encrypted connections if it cannot be \
 detected otherwise.")
 
-(defcustom erc-join-buffer 'buffer
+(defcustom erc-join-buffer 'bury
   "Determines how to display a newly created IRC buffer.
 
 The available choices are:
@@ -1662,6 +1662,7 @@ erc-join-buffer
   `bury'            - bury it in a new buffer,
   `buffer'          - in place of the current buffer,
   any other value  - in place of the current buffer."
+  :package-version '(ERC . "5.4.1") ; FIXME increment upon publishing to ELPA
   :group 'erc-buffers
   :type '(choice (const :tag "Split window and select" window)
                  (const :tag "Split window, don't select" window-noselect)
diff --git a/test/lisp/erc/erc-scenarios-base-reconnect.el b/test/lisp/erc/erc-scenarios-base-reconnect.el
index 30d692058d..49298dc594 100644
--- a/test/lisp/erc/erc-scenarios-base-reconnect.el
+++ b/test/lisp/erc/erc-scenarios-base-reconnect.el
@@ -99,10 +99,11 @@ erc-scenarios-common--base-reconnect-options
 
     (funcall test)
 
+    ;; A manual /JOIN command tells ERC we're done auto-reconnecting
     (with-current-buffer "FooNet" (erc-cmd-JOIN "#spam"))
 
-    (erc-d-t-wait-for 5 "Channel #spam shown when autojoined"
-      (eq (window-buffer) (get-buffer "#spam")))
+    (erc-d-t-ensure-for 1 "Newly joined chan ignores `erc-reconnect-display'"
+      (not (eq (window-buffer) (get-buffer "#spam"))))
 
     (ert-info ("Wait for auto reconnect")
       (with-current-buffer erc-server-buffer
@@ -114,43 +115,43 @@ erc-scenarios-common--base-reconnect-options
       (with-current-buffer (erc-d-t-wait-for 10 (get-buffer "#spam"))
         (funcall expect 10 "her elves come here anon")))))
 
-(ert-deftest erc-scenarios-base-reconnect-options--default ()
+(ert-deftest erc-scenarios-base-reconnect-options--buffer ()
   :tags '(:expensive-test)
-  (should (eq erc-join-buffer 'buffer))
+  (should (eq erc-join-buffer 'bury))
   (should-not erc-reconnect-display)
 
   ;; FooNet (the server buffer) is not switched to because it's
   ;; already current (but not shown) when `erc-open' is called.  See
   ;; related conditional guard towards the end of that function.
 
-  (erc-scenarios-common--base-reconnect-options
-   (lambda ()
-     (pop-to-buffer-same-window "*Messages*")
+  (let ((erc-reconnect-display 'buffer))
+    (erc-scenarios-common--base-reconnect-options
+     (lambda ()
+       (pop-to-buffer-same-window "*Messages*")
 
-     (erc-d-t-ensure-for 1 "Server buffer not shown"
-       (not (eq (window-buffer) (get-buffer "FooNet"))))
+       (erc-d-t-ensure-for 1 "Server buffer not shown"
+         (not (eq (window-buffer) (get-buffer "FooNet"))))
 
-     (erc-d-t-wait-for 5 "Channel #chan shown when autojoined"
-       (eq (window-buffer) (get-buffer "#chan"))))))
+       (erc-d-t-wait-for 5 "Channel #chan shown when autojoined"
+         (eq (window-buffer) (get-buffer "#chan")))))))
 
-(ert-deftest erc-scenarios-base-reconnect-options--bury ()
+(ert-deftest erc-scenarios-base-reconnect-options--default ()
   :tags '(:expensive-test)
-  (should (eq erc-join-buffer 'buffer))
+  (should (eq erc-join-buffer 'bury))
   (should-not erc-reconnect-display)
 
-  (let ((erc-reconnect-display 'bury))
-    (erc-scenarios-common--base-reconnect-options
+  (erc-scenarios-common--base-reconnect-options
 
-     (lambda ()
-       (pop-to-buffer-same-window "*Messages*")
+   (lambda ()
+     (pop-to-buffer-same-window "*Messages*")
 
-       (erc-d-t-ensure-for 1 "Server buffer not shown"
-         (not (eq (window-buffer) (get-buffer "FooNet"))))
+     (erc-d-t-ensure-for 1 "Server buffer not shown"
+       (not (eq (window-buffer) (get-buffer "FooNet"))))
 
-       (erc-d-t-ensure-for 3 "Channel #chan not shown"
-         (not (eq (window-buffer) (get-buffer "#chan"))))
+     (erc-d-t-ensure-for 3 "Channel #chan not shown"
+       (not (eq (window-buffer) (get-buffer "#chan"))))
 
-       (eq (window-buffer) (messages-buffer))))))
+     (eq (window-buffer) (messages-buffer)))))
 
 ;; Upon reconnecting, playback for channel and target buffers is
 ;; routed correctly.  Autojoin is irrelevant here, but for the
-- 
2.37.2

bug#51753: bug#55540: 29.0.50; ERC launches autojoin-channels in current frame instead of original frame

[Lars Ingebrigtsen]

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

> I believe changing the default for `erc-join-buffer' (alone) to `bury'
> is the easiest way to achieve that.
>
> Thus, if no one objects within the next week or so, I will add the patch
> below or similar to trunk (and what will become ERC 5.5), along with a
> related NEWS entry. Thanks.

Thanks; makes sense to me.

bug#51753: bug#55540: 29.0.50; ERC launches autojoin-channels in current frame instead of original frame

[J.P.]

>> Thus, if no one objects within the next week or so, I will add the patch
>> below or similar to trunk (and what will become ERC 5.5), along with a
>> related NEWS entry. Thanks.

This has been carried out.

Pankaj, all that remains is your frame stuff. If problems persist and/or
you've lost interest, please let us know, so I can act accordingly.
Thanks.

bug#51753: bug#55540: 29.0.50; ERC launches autojoin-channels in current frame instead of original frame

[Pankaj Jangid]

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

>>> Thus, if no one objects within the next week or so, I will add the patch
>>> below or similar to trunk (and what will become ERC 5.5), along with a
>>> related NEWS entry. Thanks.
>
> This has been carried out.
>
> Pankaj, all that remains is your frame stuff. If problems persist and/or
> you've lost interest, please let us know, so I can act accordingly.
> Thanks.

Now the behaviour is acceptable when "erc-autojoin-channels-alist" is
set to nil. i.e. the frame in which I launched "erc-tls" opens the erc
window instead of occupying the current frame.

But when I have few channels in "erc-autojoin-channels-alist" then the
channels are still openning in the current frame instead of the
dedicated frame that I started for "erc-tls". Is there some setting that
I missed in the conversation for this? My current "erc" related
settings are:

--8<---------------cut here---------------start------------->8---
(setq erc-prompt-for-password nil
      erc-prompt (lambda () (concat "[" (buffer-name) "]")))
(eval-when-compile (require 'erc-services))
(setq erc-prompt-for-nickserv-password nil
      erc-use-auth-source-for-nickserv-password t)
(setq erc-server "irc.libera.chat")
(setq erc-port 6697)
(setq erc-nick "my-nick")
(setq erc-user-full-name "My Full Name")
(eval-when-compile (require 'erc-join))
(setq erc-autojoin-channels-alist
      '(("libera.chat" "#emacs" "#erc" "#gnus")))
--8<---------------cut here---------------end--------------->8---

bug#51753: bug#55540: 29.0.50; ERC launches autojoin-channels in current frame instead of original frame

[J.P.]

Hi Pankaj,

Pankaj Jangid <pankaj@codeisgreat.org> writes:

> Now the behaviour is acceptable when "erc-autojoin-channels-alist" is
> set to nil. i.e. the frame in which I launched "erc-tls" opens the erc
> window instead of occupying the current frame.

Hm, right, but I think that was always the baseline behavior, no? If
not, I wonder if the recent change to the default for `erc-join-buffer'
somehow affected this.

> But when I have few channels in "erc-autojoin-channels-alist" then the
> channels are still openning in the current frame instead of the
> dedicated frame that I started for "erc-tls". Is there some setting
> that I missed in the conversation for this?

Looks like it, but I should have re-summarized either way. So that's my
bad.

> (setq erc-prompt-for-nickserv-password nil
>       erc-use-auth-source-for-nickserv-password t)

This is unrelated (so feel free to skip ahead), but are you sure
auth-source is even being consulted here? I ask because `services' isn't
a default module, and your snippet doesn't modify `erc-modules', AFAICT.
Regardless, adding services to the repro mix at this point would only
complicate matters, so please forget I said anything. I mean, if really
necessary, you can just use Libera's server-password kludge while
testing (everything will still be encrypted):

  (erc-tls :password "$myaccount:$mypass" ...)

> [...]
> (setq erc-autojoin-channels-alist
>       '(("libera.chat" "#emacs" "#erc" "#gnus")))

Ah, looks like we're missing

  (setq erc-join-buffer 'frame
        erc-auto-query 'frame
        erc-reuse-frames 'displayed)

Also, you *did* apply the patch and rerun make (or at least delete the
.elc), right?:

  https://lists.gnu.org/archive/html/emacs-erc/2022-08/txtAqY2ukHPun.txt

(Yeah, those changes aren't on trunk.) Anyway, please let me know if
something doesn't add up.

Thanks,
J.P.

P.S. If you want, you can use the symbol `Libera.Chat' instead of the
string "libera.chat" in `erc-autojoin-channels-alist'.

bug#51753: bug#55540: 29.0.50; ERC launches autojoin-channels in current frame instead of original frame

[J.P.]

I've added a version of the frames patch as well as an option to control
buffer-display behavior for interactive entry-point invocations.

  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=0e4c07dc
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=3a012d1d

Thanks and closing.

bug#51753: bug#55540: 29.0.50; ERC launches autojoin-channels in current frame instead of original frame

[J.P.]

Just a breadcrumb for posterity: I've opened a related bug that picks up
where this one left off:

  bug#62833: 30.0.50; ERC 5.6: Rethink buffer-display options and behavior