Re: bug#63595: 30.0.50; ERC 5.6: Add buffer-list and nick-list modules

From 645af24230bdf9032f1f0d79ff7e62f5ecf7b72a Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <[email protected]>
Date: Tue, 15 Aug 2023 21:14:07 -0700
Subject: [PATCH] [5.6] Prevent unwanted recursion in erc-nickbar-disable

* lisp/erc/erc-speedbar.el (erc-status-sidebar-mode--unhook): Remove
forward declaration.
(erc-speedbar--toggle-nicknames-sidebar): Inline
`erc-speedbar-close-nicknames-window'.  Don't call
`erc-speedbar-browser' to avoid excess timers being added.
(erc-speedbar--ensure): Inline `speedbar-enable-update' to avoid
unneeded call to `speedbar-set-timer', and ensure it runs in
`speedbar-buffer'.
(erc-speedbar--shutting-down-p): New flag variable to avoid recursive
calls to `dframe-close-frame' and friends.
(erc-nickbar-mode, erc-nickbar-enable, erc-nickbar-disable): Move
logic formerly performed by `speedbar-disable-update' to
`erc-speedbar--toggle-nicknames-sidebar'.  When disabling, guard
against recursive calls to `dframe-close-frame' and friends.
(erc-speedbar--get-timers): New helper function.
(erc-speedbar--dframe-controlled): Bind
`erc-speedbar--shutting-down-p' flag non-nil around call to
`erc-nickbar-mode'.  Remove excess timer left behind due to
incompatible behavior from `dframe-close-frame'.  Let caller kill
buffer.
(erc-speedbar-close-nicknames-window): Remove unused command, new in
ERC 5.6 and Emacs 30.
(erc-speedbar--destroy-nicknames-frame): New function to perform
common teardown.
* test/lisp/erc/erc-scenarios-status-sidebar.el
(erc-speedbar-close-nicknames-window): Remove forward declaration.
(erc-speedbar--get-timers): Add forward declaration.
(erc-scenarios-status-sidebar--nickbar): Fix faulty expectations of
desired behavior when disabling module.  Ensure timers canceled.
* test/lisp/erc/resources/erc-scenarios-common.el
(erc-scenarios-common--make-bindings): Shadow `timer-idle-list' to
avoid polluting global test environment with stray timers.
(Bug#63595)
---
 lisp/erc/erc-speedbar.el                      | 112 +++++++++++-------
 test/lisp/erc/erc-scenarios-status-sidebar.el |  13 +-
 .../erc/resources/erc-scenarios-common.el     |   1 +
 3 files changed, 79 insertions(+), 47 deletions(-)

diff --git a/lisp/erc/erc-speedbar.el b/lisp/erc/erc-speedbar.el
index f5fbaac767d..88400f034ba 100644
--- a/lisp/erc/erc-speedbar.el
+++ b/lisp/erc/erc-speedbar.el
@@ -439,7 +439,6 @@ erc-track--switch-fallback-blockers
 (defvar erc-status-sidebar-buffer-name)
 (declare-function erc-status-sidebar-set-window-preserve-size
                   "erc-status-sidebar" nil)
-(declare-function erc-status-sidebar-mode--unhook "erc-status-sidebar" nil)
 
 (defvar erc-speedbar--buffer-options
   '((speedbar-update-flag . t)
@@ -490,36 +489,64 @@ erc-speedbar--toggle-nicknames-sidebar
           (cl-assert (buffer-live-p speedbar-buffer))
           (if (or (and force (< arg 0))
                   (and (not force) (get-buffer-window speedbar-buffer nil)))
-              (erc-speedbar-close-nicknames-window nil)
+              ;; Close associated windows and stop updating but leave timer.
+              (progn
+                (dolist (window (get-buffer-window-list speedbar-buffer nil t))
+                  (unless (frame-root-window-p window)
+                    (when erc-speedbar--hidden-speedbar-frame
+                      (cl-assert
+                       (not (eq (window-frame window)
+                                erc-speedbar--hidden-speedbar-frame))))
+                    (delete-window window)))
+                (with-current-buffer speedbar-buffer
+                  (setq speedbar-update-flag nil)
+                  (speedbar-set-mode-line-format)))
             (when (or (not force) (>= arg 0))
               (with-selected-frame speedbar-frame
                 (erc-speedbar--emulate-sidebar-set-window-preserve-size)))))
-      (when (or (not force) (>= arg 0))
-        (let ((speedbar-frame-parameters (backquote-list*
-                                          '(visibility . nil)
-                                          '(no-other-frame . t)
-                                          speedbar-frame-parameters))
-              (speedbar-after-create-hook #'erc-speedbar--emulate-sidebar))
-          (erc-speedbar-browser)
-          ;; If we put the remaining parts in the "create hook" along
-          ;; with everything else, the frame with `window-main-window'
-          ;; gets raised and steals focus if you've switched away from
-          ;; Emacs in the meantime.
-          (make-frame-invisible speedbar-frame)
-          (select-frame (setq speedbar-frame (previous-frame)))
-          (erc-speedbar--emulate-sidebar-set-window-preserve-size))))))
+      (when-let (((or (not force) (>= arg 0)))
+                 (speedbar-frame-parameters (backquote-list*
+                                             '(visibility . nil)
+                                             '(no-other-frame . t)
+                                             speedbar-frame-parameters))
+                 (speedbar-after-create-hook #'erc-speedbar--emulate-sidebar))
+        (erc-install-speedbar-variables)
+        ;; Run before toggling mode to prevent timer from being
+        ;; created twice.
+        (speedbar-change-initial-expansion-list "ERC")
+        (speedbar-frame-mode 1)
+        ;; If we put the remaining parts in the "create hook" along
+        ;; with everything else, the frame with `window-main-window'
+        ;; gets raised and steals focus if you've switched away from
+        ;; Emacs in the meantime.
+        (make-frame-invisible speedbar-frame)
+        (select-frame (setq speedbar-frame (previous-frame)))
+        (erc-speedbar--emulate-sidebar-set-window-preserve-size))))
+  (cl-assert (not (cdr (erc-speedbar--get-timers))) t))
 
 (defun erc-speedbar--ensure (&optional force)
   (when (or (erc-server-buffer) force)
     (when erc-track-mode
       (cl-pushnew '(derived-mode . speedbar-mode)
                   erc-track--switch-fallback-blockers :test #'equal))
+    (unless (display-graphic-p)
+      (unless speedbar-update-flag
+        (erc-button--display-error-notice-with-keys
+         (erc-server-buffer)
+         "Module `nickbar' needs `speedbar-update-flag' to be non-nil"
+         " in non-graphic terminals.  Setting to t for the current"
+         " Emacs session. Customize it to avoid this message."))
+      (setq speedbar-update-flag t))
     (erc-speedbar--toggle-nicknames-sidebar +1)
-    (speedbar-enable-update)))
+    (with-current-buffer speedbar-buffer
+      (setq speedbar-update-flag t)
+      (speedbar-set-mode-line-format))))
+
+(defvar erc-speedbar--shutting-down-p nil)
 
 ;;;###autoload(autoload 'erc-nickbar-mode "erc-speedbar" nil t)
 (define-erc-module nickbar nil
-  "Show nicknames in a side window.
+  "Show nicknames for current target buffer in a side window.
 When enabling, create a speedbar session if one doesn't exist and
 show its buffer in an `erc-status-sidebar' window instead of a
 separate frame.  When disabling, close the window or, with a
@@ -527,8 +554,8 @@ nickbar
 
 WARNING: this module may produce unwanted side effects, like the
 raising of frames or the stealing of input focus.  If you witness
-such an occurrence, and can reproduce it, please file a bug
-report with \\[erc-bug]."
+such a thing and can reproduce it, please file a bug report with
+\\[erc-bug]."
   ((add-hook 'erc--setup-buffer-hook #'erc-speedbar--ensure)
    (erc-speedbar--ensure)
    (unless (or erc--updating-modules-p
@@ -542,31 +569,44 @@ nickbar
          (erc-error "Not initializing `erc-nickbar-mode' in %s"
                     (current-buffer))))))
   ((remove-hook 'erc--setup-buffer-hook #'erc-speedbar--ensure)
-   (speedbar-disable-update)
    (when erc-track-mode
      (setq erc-track--switch-fallback-blockers
            (remove '(derived-mode . speedbar-mode)
                    erc-track--switch-fallback-blockers)))
    (erc-speedbar--toggle-nicknames-sidebar -1)
-   (when-let ((arg erc--module-toggle-prefix-arg)
+   (when-let (((not erc-speedbar--shutting-down-p))
+              (arg erc--module-toggle-prefix-arg)
               ((numberp arg))
               ((< arg 0)))
-     (erc-speedbar-close-nicknames-window 'kill))))
+     (with-current-buffer speedbar-buffer
+       (dframe-close-frame)
+       (setq erc-speedbar--hidden-speedbar-frame nil)))))
+
+(defun erc-speedbar--get-timers ()
+  (cl-remove #'dframe-timer-fn timer-idle-list
+             :key #'timer--function
+             :test-not #'eq))
 
 (defun erc-speedbar--dframe-controlled (arg)
+  (when speedbar-buffer
+    (cl-assert (eq speedbar-buffer (current-buffer))))
   (when (and erc-speedbar--hidden-speedbar-frame (numberp arg) (< arg 0))
     (when erc-nickbar-mode
-      (erc-nickbar-mode -1))
+      (let ((erc-speedbar--shutting-down-p t))
+        (erc-nickbar-mode -1)))
     (setq speedbar-frame erc-speedbar--hidden-speedbar-frame
           erc-speedbar--hidden-speedbar-frame nil)
     ;; It's unknown whether leaving the frame invisible interferes
-    ;; with the upstream teardown procedure.
+    ;; with the upstream teardown sequence.
     (when (display-graphic-p)
       (make-frame-visible speedbar-frame))
-    (speedbar-frame-mode arg)
-    (when speedbar-buffer
-      (kill-buffer speedbar-buffer)
-      (setq speedbar-buffer nil))))
+    (speedbar-frame-mode arg) ; -1
+    ;; As of Emacs 29, `dframe-set-timer' can't remove `dframe-timer'.
+    (cl-assert (= 1 (length (erc-speedbar--get-timers))) t)
+    (cancel-function-timers #'dframe-timer-fn)
+    ;; `dframe-close-frame' kills the buffer but no function in
+    ;; erc-speedbar.el resets this to nil.
+    (setq speedbar-buffer nil)))
 
 (defun erc-speedbar-toggle-nicknames-window-lock ()
   "Toggle whether nicknames window is selectable with \\[other-window]."
@@ -578,20 +618,6 @@ erc-speedbar-toggle-nicknames-window-lock
       (set-window-parameter window 'no-other-window (not val))
       (message "nick-window: %s" (if val "selectable" "protected")))))
 
-(defun erc-speedbar-close-nicknames-window (kill)
-  (interactive "P")
-  (if kill
-      (with-current-buffer speedbar-buffer
-        (dframe-close-frame)
-        (cl-assert (not erc-nickbar-mode))
-        (setq erc-speedbar--hidden-speedbar-frame nil))
-    (dolist (window (get-buffer-window-list speedbar-buffer nil t))
-      (unless (frame-root-window-p window)
-        (when erc-speedbar--hidden-speedbar-frame
-          (cl-assert (not (eq (window-frame window)
-                              erc-speedbar--hidden-speedbar-frame))))
-        (delete-window window)))))
-
 
 ;;;; Nicks integration
 
diff --git a/test/lisp/erc/erc-scenarios-status-sidebar.el b/test/lisp/erc/erc-scenarios-status-sidebar.el
index 92229121c9f..3a047bf3983 100644
--- a/test/lisp/erc/erc-scenarios-status-sidebar.el
+++ b/test/lisp/erc/erc-scenarios-status-sidebar.el
@@ -94,7 +94,7 @@ erc-scenarios-status-sidebar--bufbar
 ;; terminal, and we lack a fixture for that.  Please try running this
 ;; test interactively with both graphical Emacs and non.
 (declare-function erc-nickbar-mode "erc-speedbar" (arg))
-(declare-function erc-speedbar-close-nicknames-window "erc-speedbar" (kill))
+(declare-function erc-speedbar--get-timers "erc-speedbar" nil)
 (declare-function speedbar-timer-fn "speedbar" nil)
 (defvar erc-nickbar-mode)
 (defvar speedbar-buffer)
@@ -154,16 +154,21 @@ erc-scenarios-status-sidebar--nickbar
       (ert-info ("Core toggle and kill commands work")
         ;; Avoid using API, e.g., `erc-status-sidebar-buffer-exists-p',
         ;; etc. for testing commands that call those same functions.
-        (erc-nickbar-mode -1)
+        (call-interactively #'erc-nickbar-mode)
+        (should-not erc-nickbar-mode)
         (should-not (and speedbar-buffer
                          (get-buffer-window speedbar-buffer)))
+        (should speedbar-buffer)
+
         (erc-nickbar-mode +1)
         (should (and speedbar-buffer
                      (get-buffer-window speedbar-buffer)))
         (should (get-buffer " SPEEDBAR"))
-        (erc-speedbar-close-nicknames-window 'kill)
+        (erc-nickbar-mode -1)
         (should-not (get-buffer " SPEEDBAR"))
         (should-not erc-nickbar-mode)
-        (should-not (cdr (frame-list)))))))
+        (should-not (cdr (frame-list)))))
+
+    (should-not (erc-speedbar--get-timers))))
 
 ;;; erc-scenarios-status-sidebar.el ends here
diff --git a/test/lisp/erc/resources/erc-scenarios-common.el b/test/lisp/erc/resources/erc-scenarios-common.el
index 972faa5c73f..2eb040d28d9 100644
--- a/test/lisp/erc/resources/erc-scenarios-common.el
+++ b/test/lisp/erc/resources/erc-scenarios-common.el
@@ -122,6 +122,7 @@ erc-scenarios-common--print-trace
       (inhibit-interaction t)
       (auth-source-do-cache nil)
       (timer-list (copy-sequence timer-list))
+      (timer-idle-list (copy-sequence timer-idle-list))
       (erc-auth-source-parameters-join-function nil)
       (erc-autojoin-channels-alist nil)
       (erc-server-auto-reconnect nil)
-- 
2.41.0