bug#51753: ERC switches to channel buffer on reconnect

["J.P." <jp@neverwas.me>]

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

Pankaj Jangid <pankaj@codeisgreat.org> writes:

> "J.P." <jp@neverwas.me> writes:
>
>> Are you saying ERC ought to remember the frame in which an entry-point
>> command, like `erc-tls', was originally invoked? Or might it be enough
>> to make a best effort attempt at finding such a "dedicated" frame?
>
> I have tried the #2 as specified below. I kind of agree that #1 will
> result in the desired behaviour.

Thanks for trying out that patch. Basically, I bungled a few things that
prevented it from faithfully representing #2, even though I claimed
otherwise (sorry). IOW, I'm saying the approach may be salvageable and
only the implementation flawed.

> I tried this and here is what is happening.
>
> I launched emacs then I launched another frame and M-x erc-tls and come
> back to the 1st frame for other work. "erc-tls" running fine in the
> other frame but when it starts to join autojoin channels after
> connecting, it creates one more frame for each channel instead of
> reusing the 2nd frame.

Right, it seems I blindly fixated on the segregation aspect of your
description (again, apologies) and glossed over the obvious fact that
you don't want ERC creating frames, period. I've adapted the existing
patch with this in mind in hopes you're willing to try again.

One thing to be aware of is that even if we pivot to #1, the
connection-detection and buffer-association stuff will still be
unreliable in many cases. However, that should change after bug#48598
lands. That said, if you still feel strongly about #1, then please say
so (others following along as well). And likewise if you have
suggestions on the implementation or would like to try taking the wheel.
Thanks.


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

From 7155fbd9f4afe9c9fd9dd2c89e28830a9d6612d7 Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
Date: Mon, 23 May 2022 00:15:18 -0700
Subject: [PATCH 0/1] *** NOT A PATCH ***

*** BLURB HERE ***

F. Jason Park (1):
  Allow erc-reuse-frames to favor connections

 lisp/erc/erc.el            |  52 +++++++++++++---
 test/lisp/erc/erc-tests.el | 119 +++++++++++++++++++++++++++++++++++++
 2 files changed, 163 insertions(+), 8 deletions(-)

Interdiff:
diff --git a/lisp/erc/erc.el b/lisp/erc/erc.el
index f82fb07a66..13ac0ec979 100644
--- a/lisp/erc/erc.el
+++ b/lisp/erc/erc.el
@@ -1538,11 +1538,12 @@ erc-frame-dedicated-flag
 (defcustom erc-reuse-frames t
   "Non-nil means only create a frame for undisplayed buffers.
 For new target buffers, a value of 'displayed' extends this to mean use
-the frame of any buffer from the same server connection, visible or not.
-When this option is nil, a new frame is always created.  Regardless of
-its value, this option is ignored unless `erc-join-buffer' is set to
-`frame'.  Note that like most options in the `erc-buffer' customize
-group, this has no effect on server buffers while reconnecting."
+the frame of any buffer from the same server connection, visible or not,
+or, as a last resort, a frame showing any ERC buffer.  When this option
+is nil, a new frame is always created.  Regardless of its value, this
+option is ignored unless `erc-join-buffer' is set to `frame'.  Note that
+like most options in the `erc-buffer' customize group, this has no
+effect on server buffers while reconnecting."
   :package-version '(ERC . "5.4.1") ; FIXME update when publishing to ELPA
   :group 'erc-buffers
   :type '(choice boolean
@@ -1948,18 +1949,30 @@ erc-update-modules
             (funcall sym 1)
           (error "`%s' is not a known ERC module" mod))))))
 
-(defun erc--setup-buffer-frame-displayed (buffer)
+(defun erc--setup-buffer-first-win (frame a b)
+  (catch 'found
+    (walk-window-tree
+     (lambda (w)
+       (when (eq (buffer-local-value a (window-buffer w)) b)
+         (throw 'found t)))
+     frame nil 0)))
+
+(defun erc--display-buffer-use-some-frame (buffer alist)
   "Maybe display BUFFER in an existing frame for the same connection.
-If performed, return window used; otherwise, return nil."
+If performed, return window used; otherwise, return nil.  Forward ALIST
+to display-buffer machinery."
   (when-let*
-      ((bs (erc-buffer-list nil erc-server-process))
-       (visit (lambda (w) (when (memq (window-buffer w) bs) (throw 'found t))))
-       (test (lambda (fr) (catch 'found (walk-windows visit 0 fr))))
-       ((or (cdr (frame-list)) (funcall test (selected-frame)))))
-    (display-buffer buffer `((display-buffer-use-some-frame)
-                             (inhibit-switch-frame . t)
-                             (inhibit-same-window . t)
-                             (frame-predicate . ,test)))))
+      ((same-proc-p (lambda (fr)
+                      (erc--setup-buffer-first-win fr 'erc-server-process
+                                                   erc-server-process)))
+       (same-mode-p (lambda (fr)
+                      (erc--setup-buffer-first-win fr 'major-mode 'erc-mode)))
+       ((or (cdr (frame-list)) (funcall same-mode-p (selected-frame))))
+       (frame (car (or (filtered-frame-list same-proc-p)
+                       (filtered-frame-list same-mode-p))))
+       (window (get-lru-window frame nil t)))
+    ;; FIXME don't rely on internal window.el function (tab-bar also does it)
+    (window--display-buffer buffer window 'reuse alist)))
 
 (defun erc-setup-buffer (buffer)
   "Consults `erc-join-buffer' to find out how to display `BUFFER'."
@@ -1975,9 +1988,9 @@ erc-setup-buffer
     ('frame
      (cond
       ((and (eq erc-reuse-frames 'displayed)
-            erc-default-recipients ; change this to `erc--target' after bug#48598
-            (not (get-buffer-window buffer t))
-            (erc--setup-buffer-frame-displayed buffer)))
+            (not (get-buffer-window buffer t)))
+       (display-buffer buffer `((erc--display-buffer-use-some-frame)
+                                (inhibit-same-window . t))))
       ((or (not erc-reuse-frames)
            (not (get-buffer-window buffer t)))
        (let ((frame (make-frame (or erc-frame-alist
diff --git a/test/lisp/erc/erc-tests.el b/test/lisp/erc/erc-tests.el
index 1eac6b22c9..686db3bb2e 100644
--- a/test/lisp/erc/erc-tests.el
+++ b/test/lisp/erc/erc-tests.el
@@ -173,6 +173,10 @@ erc--switch-to-buffer
 
 (ert-deftest erc-reuse-frames ()
   ;; TODO run this in a pseudo terminal subprocess for EMBA
+  ;;
+  ;; TODO case that simulates automatic reconnecting, with an
+  ;; existing, unselected frame containing two windows, one with a
+  ;; dead ERC buffer and the other a non-ERC buffer
   (skip-unless (not noninteractive))
   (should-not erc-frame-dedicated-flag)
   (let ((erc-join-buffer 'frame)
-- 
2.36.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0001-Allow-erc-reuse-frames-to-favor-connections.patch --]
[-- Type: text/x-patch, Size: 10576 bytes --]

From 7155fbd9f4afe9c9fd9dd2c89e28830a9d6612d7 Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
Date: Sat, 21 May 2022 00:04:04 -0700
Subject: [PATCH 1/1] Allow erc-reuse-frames to favor connections

* lisp/erc/erc.el (erc-reuse-frames): Add alternate value to favor
existing frames already displaying other buffers from the same
connection.
(erc-setup-buffer): Add case for "displayed" variant of
`erc-reuse-frames'.
---
 lisp/erc/erc.el            |  52 +++++++++++++---
 test/lisp/erc/erc-tests.el | 119 +++++++++++++++++++++++++++++++++++++
 2 files changed, 163 insertions(+), 8 deletions(-)

diff --git a/lisp/erc/erc.el b/lisp/erc/erc.el
index ff482d4933..13ac0ec979 100644
--- a/lisp/erc/erc.el
+++ b/lisp/erc/erc.el
@@ -1536,12 +1536,18 @@ erc-frame-dedicated-flag
   :type 'boolean)
 
 (defcustom erc-reuse-frames t
-  "Determines whether new frames are always created.
-Non-nil means that a new frame is not created to display an ERC
-buffer if there is already a window displaying it.  This only has
-effect when `erc-join-buffer' is set to `frame'."
+  "Non-nil means only create a frame for undisplayed buffers.
+For new target buffers, a value of 'displayed' extends this to mean use
+the frame of any buffer from the same server connection, visible or not,
+or, as a last resort, a frame showing any ERC buffer.  When this option
+is nil, a new frame is always created.  Regardless of its value, this
+option is ignored unless `erc-join-buffer' is set to `frame'.  Note that
+like most options in the `erc-buffer' customize group, this has no
+effect on server buffers while reconnecting."
+  :package-version '(ERC . "5.4.1") ; FIXME update when publishing to ELPA
   :group 'erc-buffers
-  :type 'boolean)
+  :type '(choice boolean
+                 (const displayed)))
 
 (defun erc-channel-p (channel)
   "Return non-nil if CHANNEL seems to be an IRC channel name."
@@ -1943,6 +1949,31 @@ erc-update-modules
             (funcall sym 1)
           (error "`%s' is not a known ERC module" mod))))))
 
+(defun erc--setup-buffer-first-win (frame a b)
+  (catch 'found
+    (walk-window-tree
+     (lambda (w)
+       (when (eq (buffer-local-value a (window-buffer w)) b)
+         (throw 'found t)))
+     frame nil 0)))
+
+(defun erc--display-buffer-use-some-frame (buffer alist)
+  "Maybe display BUFFER in an existing frame for the same connection.
+If performed, return window used; otherwise, return nil.  Forward ALIST
+to display-buffer machinery."
+  (when-let*
+      ((same-proc-p (lambda (fr)
+                      (erc--setup-buffer-first-win fr 'erc-server-process
+                                                   erc-server-process)))
+       (same-mode-p (lambda (fr)
+                      (erc--setup-buffer-first-win fr 'major-mode 'erc-mode)))
+       ((or (cdr (frame-list)) (funcall same-mode-p (selected-frame))))
+       (frame (car (or (filtered-frame-list same-proc-p)
+                       (filtered-frame-list same-mode-p))))
+       (window (get-lru-window frame nil t)))
+    ;; FIXME don't rely on internal window.el function (tab-bar also does it)
+    (window--display-buffer buffer window 'reuse alist)))
+
 (defun erc-setup-buffer (buffer)
   "Consults `erc-join-buffer' to find out how to display `BUFFER'."
   (pcase erc-join-buffer
@@ -1955,15 +1986,20 @@ erc-setup-buffer
     ('bury
      nil)
     ('frame
-     (when (or (not erc-reuse-frames)
-               (not (get-buffer-window buffer t)))
+     (cond
+      ((and (eq erc-reuse-frames 'displayed)
+            (not (get-buffer-window buffer t)))
+       (display-buffer buffer `((erc--display-buffer-use-some-frame)
+                                (inhibit-same-window . t))))
+      ((or (not erc-reuse-frames)
+           (not (get-buffer-window buffer t)))
        (let ((frame (make-frame (or erc-frame-alist
                                     default-frame-alist))))
          (raise-frame frame)
          (select-frame frame))
        (switch-to-buffer buffer)
        (when erc-frame-dedicated-flag
-         (set-window-dedicated-p (selected-window) t))))
+         (set-window-dedicated-p (selected-window) t)))))
     (_
      (if (active-minibuffer-window)
          (display-buffer buffer)
diff --git a/test/lisp/erc/erc-tests.el b/test/lisp/erc/erc-tests.el
index 520f10dd4e..686db3bb2e 100644
--- a/test/lisp/erc/erc-tests.el
+++ b/test/lisp/erc/erc-tests.el
@@ -171,6 +171,125 @@ erc--switch-to-buffer
     (dolist (b '("server" "other" "#chan" "#foo" "#fake"))
       (kill-buffer b))))
 
+(ert-deftest erc-reuse-frames ()
+  ;; TODO run this in a pseudo terminal subprocess for EMBA
+  ;;
+  ;; TODO case that simulates automatic reconnecting, with an
+  ;; existing, unselected frame containing two windows, one with a
+  ;; dead ERC buffer and the other a non-ERC buffer
+  (skip-unless (not noninteractive))
+  (should-not erc-frame-dedicated-flag)
+  (let ((erc-join-buffer 'frame)
+        (erc-reuse-frames t)
+        (orig-frame (selected-frame))
+        erc-kill-channel-hook erc-kill-server-hook erc-kill-buffer-hook)
+
+    (ert-info ("Value: t")
+      (with-current-buffer (generate-new-buffer "server")
+        (erc-mode)
+        (setq erc-server-process (start-process "server" (current-buffer)
+                                                "sleep" "1")
+              erc-frame-alist (cons '(name . "server") default-frame-alist))
+        (set-process-query-on-exit-flag erc-server-process nil)
+        (should-not (get-buffer-window (current-buffer) t))
+        (erc-setup-buffer (current-buffer))
+        ;; New frame created and raised
+        (should (equal "server" (frame-parameter (window-frame) 'name)))
+        (should (get-buffer-window (current-buffer) t))
+
+        (with-current-buffer (generate-new-buffer "#chan")
+          (erc-mode)
+          (setq erc-server-process erc-server-process
+                erc-frame-alist (cons '(name . "#chan") default-frame-alist)
+                erc-default-recipients '("#chan"))
+          (should-not (get-buffer-window (current-buffer) t))
+          (erc-setup-buffer (current-buffer))
+          ;; Another frame was created just for #chan
+          (should (equal "#chan" (frame-parameter (window-frame) 'name)))
+          (should (get-buffer-window (current-buffer) t))
+          (delete-frame))
+
+        (select-frame-by-name "server")
+        (pop-to-buffer "#chan")
+        ;; The server frame contains two vertical windows
+        (let ((tree (window-tree)))
+          (should (memq (get-buffer-window "server" t) (car tree)))
+          (should (memq (get-buffer-window "#chan" t) (car tree))))
+        (should (eq (get-buffer "#chan") (window-buffer (selected-window))))
+        (should (eq (get-buffer "server") (window-buffer (next-window))))))
+
+    (ert-info ("Value: displayed, scratch frame selected")
+      (select-frame orig-frame)
+      (with-current-buffer "*scratch*"
+        (with-current-buffer (generate-new-buffer "#spam")
+          (erc-mode)
+          (setq erc-server-process (buffer-local-value 'erc-server-process
+                                                       (get-buffer "server"))
+                erc-reuse-frames 'displayed
+                erc-frame-alist (cons '(name . "#spam") default-frame-alist)
+                erc-default-recipients '("#spam"))
+          (should-not (get-buffer-window (current-buffer) t))
+          (erc-setup-buffer (current-buffer))
+          ;; Window shows up in other frame
+          (should (eq (selected-frame) orig-frame))
+          (let ((fr (window-frame (get-buffer-window (current-buffer) t))))
+            (should (equal "server" (frame-parameter fr 'name)))
+            (with-selected-frame fr
+              (should (memq (get-buffer-window "#spam" t)
+                            (car (window-tree))))))))
+
+      (with-current-buffer "server"
+        (ert-info ("Value: displayed, server frame selected")
+          (select-frame-by-name "server")
+          (select-window (get-buffer-window "#spam"))
+          (with-current-buffer (generate-new-buffer "bob")
+            (erc-mode)
+            (setq erc-server-process (buffer-local-value 'erc-server-process
+                                                         (get-buffer "server"))
+                  erc-frame-alist (cons '(name . "bob") default-frame-alist)
+                  erc-default-recipients '("bob"))
+            (should-not (get-buffer-window (current-buffer) t))
+            (erc-setup-buffer (current-buffer))
+            ;; Window shows up in this frame
+            (let ((fr (window-frame (get-buffer-window (current-buffer) t))))
+              (should (eq fr (selected-frame)))
+              (should (equal "server" (frame-parameter fr 'name)))
+              (with-selected-frame fr
+                (should (memq (get-buffer-window "bob" t)
+                              (car (window-tree)))))
+              ;; `inhibit-same-window' respected
+              (should-not (eq (get-buffer-window "bob") (selected-window))))))
+
+        (ert-info ("Value: displayed, other frames deleted")
+          (with-selected-frame orig-frame
+            (delete-frame))
+          (should-not (cdr (frame-list)))
+          (select-window (get-buffer-window "bob"))
+          (with-current-buffer (generate-new-buffer "alice")
+            (erc-mode)
+            (setq erc-server-process (buffer-local-value 'erc-server-process
+                                                         (get-buffer "server"))
+                  erc-frame-alist (cons '(name . "alice") default-frame-alist)
+                  erc-default-recipients '("alice"))
+            (should-not (get-buffer-window (current-buffer) t))
+            (erc-setup-buffer (current-buffer))
+            (let ((fr (window-frame (get-buffer-window (current-buffer) t))))
+              (should (eq fr (selected-frame)))
+              (should (equal "server" (frame-parameter fr 'name)))
+              (with-selected-frame fr
+                (should (memq (get-buffer-window "alice" t)
+                              (car (window-tree)))))
+              (should-not (eq (get-buffer-window "alice")
+                              (selected-window)))))))))
+
+  (should-not (cdr (frame-list)))
+  (delete-other-windows)
+  (kill-buffer "server")
+  (kill-buffer "bob")
+  (kill-buffer "alice")
+  (kill-buffer "#spam")
+  (kill-buffer "#chan"))
+
 (ert-deftest erc-lurker-maybe-trim ()
   (let (erc-lurker-trim-nicks
         (erc-lurker-ignore-chars "_`"))
-- 
2.36.1