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

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

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

This may come as a shock, but problems exist in the `bufbar' changes
recently introduced (by me) to erc-status-sidebar.el.

The first involves the new boolean option `erc-status-sidebar-singular'.
No matter what its doc string says, the purpose of this option remains
difficult to describe and its behavior hard to predict. This makes it
only really practical as an internal flag, which is what it should have
been from the get-go and how it's been repurposed in the attached patch.
Initially, this option was intended to alter the behavior of the
minor-mode toggle `erc-bufbar-mode' so that when the option is nil,
toggling the mode in a frame where the sidebar is hidden would summon
it, regardless of the value of the mode's variable. This kind of extreme
DWIM'ness may suit normal, standalone minor modes, but users likely
expect mode toggles for global ERC modules to work more or less
traditionally.

A somewhat related problem stems from my altering the original behavior
of existing status-sidebar commands, like `erc-status-sidebar-open', to
accommodate and promote `erc-bufbar-mode' as a unified entry point. In
retrospect, that probably threatens to anger the rare person who's
become accustomed to those commands over the past couple years and who
may not be interested in the new module at all (for whatever reason).
I've attempted to rectify this in the attached patch so that the newer
behavior only takes effect when `erc-bufbar-mode' is active. This means
users of the module must now issue an M-x erc-status-sidebar-open RET to
see the sidebar in (additional) frames that don't yet have one, but
non-module users will still only ever encounter a single sidebar
instance in an Emacs session.

The last issue involves the sidebar intermittently clobbering the
window's scroll position. I've tried to eradicate this while also
improving the overall responsiveness by having it refresh when users
select another window. Basically, updates were initially tethered to
mode-line changes, which often left users with the wrong buffer
highlighted whenever updates were few and far between. Although this is
now much improved, the change likely adds some overhead, so I've
included an escape hatch while we investigate further.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-5.6-Simplify-multi-frame-behavior-of-erc-bufbar-mode.patch --]
[-- Type: text/x-patch, Size: 13129 bytes --]

From 7b47f05fbc7093db798f2ee421a3082c9febc363 Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
Date: Sun, 23 Jul 2023 23:09:42 -0700
Subject: [PATCH 1/2] [5.6] Simplify multi-frame behavior of erc-bufbar-mode

* lisp/erc/erc-status-sidebar.el (erc-status-sidebar-singular,
erc-status-sidebar--singular-p): Replace public option, new in ERC
5.6, with the latter, an internal state flag.
(erc-status-sidebar-get-window): Use new name for option turned
ordinary variable `erc-status-sidebar--singular-p'.
(erc-status-sidebar-close): Add comment.
(erc-status-sidebar--open): New function containing the old body of
`erc-status-sidebar-open'.
(erc-bufbar-mode, erc-bufbar-enable, erc-bufbar-disable): Update
variable names.  When disabling, always close window on all frames.
Don't set mode variable to nil when enabling outside of an `erc-mode'
buffer.  This may have made some practical sense but was illogical
since the `erc--setup-buffer-hook' was left as is, meaning the
activation state was still partially enabled.
(erc-status-sidebar-open): Move definition to lower down in file,
after `erc-bufbar-mode'.  When `erc-bufbar-mode' is active, always
create a sidebar if needed, even when another frame is already
displaying one.
(erc-status-toggle-sidebar): When `erc-bufbar-mode' is disabled,
revert to pre-5.6 behavior, ignoring bug fixes.  When the module is
enabled, adopt new behavior of always ensuring the current frame shows
a sidebar, even if another frame already has one.
(erc-status-sidebar-refresh): Save and restore window start in all
windows showing status sidebar buffer after refreshing.  Update
option and variable names.
(erc-status-sidebar--refresh-unless-input): New function to run
`erc-status-sidebar-refresh' unless input is pending or the selected
window's buffer is a minibuffer.
(erc-status-sidebar--post-refresh): Call `erc-status-sidebar-refresh'
wrapper `erc-status-sidebar--refresh-unless-input' instead.
(erc-status-sidebar-refresh-triggers): Add doc string, noting that the
variable is set locally when the option
`erc-status-sidebar-highlight-active-buffer' is non-nil.
(erc-status-sidebar--highlight-refresh-triggers): New variable
containing additional triggers enabled when the option
`erc-status-highlight-active-buffer' is non-nil.
(erc-status-sidebar-set-window-preserve-size): Update var name to
`erc-status-sidebar--singular-p'.
(erc-status-sidebar-mode): Run `erc-status-sidebar--post-refresh' on
`window-selection-change-functions' globally when highlighting active
buffers.  (bug#63595)
---
 lisp/erc/erc-status-sidebar.el | 107 +++++++++++++++++++++------------
 1 file changed, 70 insertions(+), 37 deletions(-)

diff --git a/lisp/erc/erc-status-sidebar.el b/lisp/erc/erc-status-sidebar.el
index b8bd7b0065e..a82c846ff1a 100644
--- a/lisp/erc/erc-status-sidebar.el
+++ b/lisp/erc/erc-status-sidebar.el
@@ -45,8 +45,8 @@
 ;; Use M-x erc-status-sidebar-kill RET to kill the sidebar buffer and
 ;; close the sidebar on all frames.
 
-;; In addition to the commands above, you can also try the all-in-one,
-;; "DWIM" command, `erc-bufbar-mode'.  See its doc string for usage.
+;; In addition to the commands above, you can also try the all-in-one
+;; entry point `erc-bufbar-mode'.  See its doc string for usage.
 
 ;; If you want the status sidebar enabled whenever you use ERC, add
 ;; `bufbar' to `erc-modules'.  Note that this library also has a major
@@ -130,8 +130,11 @@ erc-status-sidebar-style
   `erc-status-sidebar-pad-hierarchy'
 
 for the above-mentioned purposes.  ERC also accepts a list of
-functions to preform these roles a la carte.  See doc strings for
-a description of their expected arguments and return values."
+functions to preform these roles a la carte.  Since the members
+of the above sets aren't really interoperable, we don't offer
+them here as customization choices, but you can still specify
+them manually.  See doc strings for a description of their
+expected arguments and return values."
   :package-version '(ERC . "5.6") ; FIXME sync on release
   :type '(choice (const channels-only)
                  (const all-mixed)
@@ -158,10 +161,12 @@ erc-status-sidebar-click-display-action
                               :key-type symbol
                               :value-type (sexp :tag "Value")))))
 
-(defcustom erc-status-sidebar-singular t
-  "Whether to show the sidebar on all frames or just one (default)."
-  :package-version '(ERC . "5.6") ; FIXME sync on release
-  :type 'boolean)
+(defvar erc-status-sidebar--singular-p t
+  "Whether to restrict the sidebar to a single frame.
+This variable only affects `erc-bufbar-mode'.  Disabling it does
+not arrange for automatically showing the sidebar in all frames.
+Rather, disabling it allows for displaying the sidebar in the
+selected frame even if it's already showing in some other frame.")
 
 (defvar hl-line-mode)
 (declare-function hl-line-highlight "hl-line" nil)
@@ -178,7 +183,7 @@ erc-status-sidebar-get-window
 
 If NO-CREATION is non-nil, the window is not created."
   (let ((sidebar-window (get-buffer-window erc-status-sidebar-buffer-name
-                                           erc-status-sidebar-singular)))
+                                           erc-status-sidebar--singular-p)))
     (unless (or sidebar-window no-creation)
       (with-current-buffer (erc-status-sidebar-get-buffer)
         (setq-local vertical-scroll-bar nil))
@@ -214,7 +219,7 @@ erc-status-sidebar-close
 containing it on the current frame is closed.  See
 `erc-status-sidebar-kill'."
   (interactive "P")
-  (mapcar #'delete-window
+  (mapcar #'delete-window ; FIXME use `mapc'.
           (get-buffer-window-list (erc-status-sidebar-get-buffer)
                                   nil (if all-frames t))))
 
@@ -223,10 +228,8 @@ erc-status-sidebar-writable
   `(let ((buffer-read-only nil))
      ,@body))
 
-;;;###autoload
-(defun erc-status-sidebar-open ()
-  "Open or create a sidebar."
-  (interactive)
+(defun erc-status-sidebar--open ()
+  "Maybe open the sidebar, respecting `erc-status-sidebar--singular-p'."
   (save-excursion
     (if (erc-status-sidebar-buffer-exists-p)
         (erc-status-sidebar-get-window)
@@ -237,11 +240,15 @@ erc-status-sidebar-open
 ;;;###autoload(autoload 'erc-bufbar-mode "erc-status-sidebar" nil t)
 (define-erc-module bufbar nil
   "Show `erc-track'-like activity in a side window.
-When enabling, show the sidebar immediately if called from a
-connected ERC buffer.  Otherwise, arrange for doing so on connect
-or whenever next displaying a new ERC buffer.  When disabling,
-hide the status window if it's showing.  With a negative prefix
-arg, also shutdown the session."
+When enabling, show the sidebar immediately in the current frame
+if called from a connected ERC buffer.  Otherwise, arrange for
+doing so on connect or whenever next displaying a new ERC buffer.
+When disabling, hide the status window in all frames.  With a
+negative prefix arg, also shutdown the session.  Normally, this
+module only allows one sidebar window in an Emacs session.  To
+override this, use `erc-status-sidebar-open' to force creation
+and `erc-status-sidebar-close' to hide a single instance on the
+current frame only."
   ((unless erc-track-mode
      (unless (memq 'track erc-modules)
        (erc--warn-once-before-connect 'erc-bufbar-mode
@@ -249,30 +256,38 @@ bufbar
          " This will affect \C-]all\C-] ERC sessions."
          " Add `track' to `erc-modules' to silence this message."))
      (erc-track-mode +1))
-   (add-hook 'erc--setup-buffer-hook #'erc-status-sidebar-open)
+   (add-hook 'erc--setup-buffer-hook #'erc-status-sidebar--open)
    (unless erc--updating-modules-p
      (if (erc-with-server-buffer erc-server-connected)
-         (erc-status-sidebar-open)
-       (setq erc-bufbar-mode nil)
+         (erc-status-sidebar--open)
        (when (derived-mode-p 'erc-mode)
          (erc-error "Not initializing `erc-bufbar-mode' in %s"
                     (current-buffer))))))
-  ((remove-hook 'erc--setup-buffer-hook #'erc-status-sidebar-open)
-   (erc-status-sidebar-close erc-status-sidebar-singular)
+  ((remove-hook 'erc--setup-buffer-hook #'erc-status-sidebar--open)
+   (erc-status-sidebar-close 'all-frames)
    (when-let ((arg erc--module-toggle-prefix-arg)
               ((numberp arg))
               ((< arg 0)))
      (erc-status-sidebar-kill))))
 
+;;;###autoload
+(defun erc-status-sidebar-open ()
+  "Open or create a sidebar window in the current frame.
+When `erc-bufbar-mode' is active, do this even if one already
+exists in another frame."
+  (interactive)
+  (let ((erc-status-sidebar--singular-p (not erc-bufbar-mode)))
+    (erc-status-sidebar--open)))
+
 ;;;###autoload
 (defun erc-status-sidebar-toggle ()
   "Toggle the sidebar open/closed on the current frame.
-Do this regardless of `erc-status-sidebar-singular'."
+When opening and `erc-bufbar-mode' is active, create a sidebar
+even if one already exists in another frame."
   (interactive)
   (if (get-buffer-window erc-status-sidebar-buffer-name nil)
       (erc-status-sidebar-close)
-    (let (erc-status-sidebar-singular)
-      (erc-status-sidebar-open))))
+    (erc-status-sidebar-open)))
 
 (defun erc-status-sidebar-get-channame (buffer)
   "Return name of BUFFER with all leading \"#\" characters removed."
@@ -413,11 +428,10 @@ erc-status-sidebar-refresh
                      erc-status-sidebar-pad-hierarchy))
                   (v v)))
                (chanlist (apply sort-fn (funcall list-fn nil) nil))
-               (window nil)
-               (winstart nil))
+               (windows nil))
     (with-current-buffer (erc-status-sidebar-get-buffer)
-      (setq window (get-buffer-window nil erc-status-sidebar-singular)
-            winstart (and window (window-start window)))
+      (dolist (window (get-buffer-window-list nil nil t))
+        (push (cons window (window-start window)) windows))
       (erc-status-sidebar-writable
        (delete-region (point-min) (point-max))
        (goto-char (point-min))
@@ -443,9 +457,10 @@ erc-status-sidebar-refresh
             0 cnlen 'help-echo
             "mouse-1: switch to buffer in other window" channame)
            (funcall insert-fn channame chanbuf chanlist)))
-       (when winstart
-         (set-window-point window winstart)
-         (with-selected-window window (recenter 0)))
+       (when windows
+         (pcase-dolist (`(,window . ,winstart) windows)
+           (set-window-point window winstart)
+           (with-selected-window window (recenter 0))))
        (when (and erc-status-sidebar-highlight-active-buffer
                   (marker-buffer erc-status-sidebar--active-marker))
          (goto-char erc-status-sidebar--active-marker)
@@ -519,14 +534,28 @@ erc-status-sidebar-refresh-triggers
     erc-kill-server-hook
     erc-kick-hook
     erc-disconnected-hook
-    erc-quit-hook))
+    erc-quit-hook)
+  "Hooks to refresh the sidebar on.
+This may be set locally in the status-sidebar buffer under
+various conditions, like when the option
+`erc-status-sidebar-highlight-active-buffer' is non-nil.")
+
+(defvar erc-status-sidebar--highlight-refresh-triggers
+  '(window-selection-change-functions)
+  "Triggers enabled with `erc-status-sidebar-highlight-active-buffer'.")
+
+(defun erc-status-sidebar--refresh-unless-input ()
+  "Run `erc-status-sidebar-refresh' unless there are unread commands.
+Also abstain when the user is interacting with the minibuffer."
+  (unless (or (input-pending-p) (minibuffer-window-active-p (selected-window)))
+    (erc-status-sidebar-refresh)))
 
 (defun erc-status-sidebar--post-refresh (&rest _ignore)
   "Schedule sidebar refresh for execution after command stack is cleared.
 
 Ignore arguments in IGNORE, allowing this function to be added to
 hooks that invoke it with arguments."
-  (run-at-time 0 nil #'erc-status-sidebar-refresh))
+  (run-at-time 0 nil #'erc-status-sidebar--refresh-unless-input))
 
 (defun erc-status-sidebar-mode--unhook ()
   "Remove hooks installed by `erc-status-sidebar-mode'."
@@ -541,7 +570,7 @@ erc-status-sidebar-set-window-preserve-size
 Note that preserve status needs to be reset when the window is
 manually resized, so `erc-status-sidebar-mode' adds this function
 to the `window-configuration-change-hook'."
-  (when (and (eq (selected-window) (let (erc-status-sidebar-singular)
+  (when (and (eq (selected-window) (let (erc-status-sidebar--singular-p)
                                      (erc-status-sidebar-get-window)))
              (fboundp 'window-preserve-size))
     (unless (eq (window-total-width) (window-min-size nil t))
@@ -563,6 +592,10 @@ erc-status-sidebar-mode
 
   (add-hook 'window-configuration-change-hook
             #'erc-status-sidebar-set-window-preserve-size nil t)
+  (when erc-status-sidebar-highlight-active-buffer
+    (setq-local erc-status-sidebar-refresh-triggers
+                `(,@erc-status-sidebar--highlight-refresh-triggers
+                  ,@erc-status-sidebar-refresh-triggers)))
   (dolist (hk erc-status-sidebar-refresh-triggers)
     (add-hook hk #'erc-status-sidebar--post-refresh))
 
-- 
2.41.0