Revamping bs.el

Revamping bs.el

[Juanma Barranquero]

[-- Attachment #1.1: Type: text/plain, Size: 3167 bytes --]

As a long-time user of bs.el, I've often been quite frustrated by some of
its limitations. It shows its age here and there, but mainly in two aspects:

- It uses kludgy code to save & restore window configurations to set the
frame back to a "normal" state after exiting; that code just doesn't play
nice with the newer display-buffer-alist window setup.

- It doesn't use tabulated-list-mode and instead creates its own clunky
"header line", sort functions, etc. One example where that goes wrong is
when the current bs-configuration wants to show more than
`bs-max-window-height' buffers; in this case, it is perfectly possible to
just move along the list until you push the "header line" out of the window.

So I'd like to work on this on master, for the 30.1 release.

For the first problem, the attached patch makes bs-show to use
pop-to-buffer, and adds a bs-default-action-list customizable option,
preloaded with a value that imitates the current, non-patched behavior,
while allowing setting display-buffer-alist, or the customizable option, to
further adapt bs to modern practices. The code also removes one function
and one variable, which I haven't obsoleted because they're intern.

Now, in my tests the behavior of bs-show has not changed (except for a tiny
detail where the patched behavior is, I think, superior to the old one**).
However, I'm pretty sure there will be some bugs lurking. As this is a
small change, easily reverted, and for a quite non-critical piece of Emacs,
I'd prefer to install the patch ASAP in master and let the bug reports
come, instead of trying to develop it in a feature branch that nobody will
fetch and test (and understandably so).

--------------
**The difference is that, with the old code,

emacs -Q
C-x 2
M-x bs-show <ret>

makes the bottom window grow while the *bs-selection* buffer is active
because the bs window is created at half the height of the top window, and
then shrunk. In my patch, I use window-combination-limit to force the top
window and the new bs window to be a combination group so their combined
height doesn't change.
--------------

Now, the second problem (deriving bs-mode from tabulated-list-mode) is not
as straightforward. Using the tabulated-list machinery to populate
*buffer-selection*, and create the header-line on the fly, is of course
easy enough. The problems come from the impedance mismatch between the
extremely column-oriented sorting machinery of tabulated-list-mode, vs the
freedom of bs sort functions that are not so limited. A few weeks ago I
spent a couple of days looking at the issue and came to the conclusion that
making bs work with tabulated-list-mode would require adding some
functionality to the later, so it can better communicate with its derived
mode and be aware that it has sorted the data in... "unexpected ways".

So, at this point,

1) Is there some interest (or, at the very least, indifference ;-) in my
revamping bs.el to work better with display-buffer-alist?
2) If so, it's ok to install the change in master?

Once this is settled, we can discuss whether the second part of the rework
is worth pursuing or not, and if so, what would be necessary.

[-- Attachment #1.2: Type: text/html, Size: 3726 bytes --]

[-- Attachment #2: 0001-lisp-bs.el-Adapt-to-modern-display-buffer-alist-wind.patch --]
[-- Type: application/octet-stream, Size: 5158 bytes --]

From 298ae623055175d198f16fecd71760eef113f3b2 Mon Sep 17 00:00:00 2001
From: Juanma Barranquero <lekktu@gmail.com>
Date: Tue, 6 Dec 2022 02:22:06 +0100
Subject: [PATCH] * lisp/bs.el: Adapt to modern display-buffer-alist window
 setup

* lisp/bs.el (bs--window-config-coming-from): Delete.
(bs-default-action-list): New user customizable option.
(bs--restore-window-config): Delete.
(bs-kill, bs-select, bs-select-other-window)
(bs-select-other-frame): Use `quit-window' instead.
(bs--show-with-configuration): Use `pop-to-buffer' to display
the "*buffer-selection*" buffer.
---
 lisp/bs.el | 58 ++++++++++++++++++++++++------------------------------
 1 file changed, 26 insertions(+), 32 deletions(-)

diff --git a/lisp/bs.el b/lisp/bs.el
index 1fd31fb3b8..289ede6819 100644
--- a/lisp/bs.el
+++ b/lisp/bs.el
@@ -420,9 +420,6 @@ bs--show-all
 Non-nil means to show all buffers.  Otherwise show buffers
 defined by current configuration `bs-current-configuration'.")
 
-(defvar bs--window-config-coming-from nil
-  "Window configuration before starting Buffer Selection Menu.")
-
 (defvar bs--intern-show-never "^ \\|\\*buffer-selection\\*"
   "Regular expression specifying which buffers never to show.
 A buffer whose name matches this regular expression will never be
@@ -491,7 +488,23 @@ bs-mode-map
   "<mouse-2>" #'bs-mouse-select
   "<mouse-3>" #'bs-mouse-select-other-frame)
 
-;; ----------------------------------------------------------------------
+(defcustom bs-default-action-list '((display-buffer-reuse-window
+				     display-buffer-below-selected)
+				    (window-height . bs-max-window-height))
+  "Default action list for showing the '*bs-selection*' buffer.
+
+This list will be passed to `pop-to-buffer' as its ACTION argument.
+It should be a cons cell (FUNCTIONS . ALIST), where FUNCTIONS is
+an action function or a list of action functions and ALIST is an
+action alist.  Each such action function should accept two
+arguments: a buffer to display and an alist of the same form as
+ALIST.  See ‘display-buffer’ for details."
+  :type display-buffer--action-custom-type
+  :risky t
+  :version "30.1"
+  :group 'bs)
+
+; ----------------------------------------------------------------------
 ;; Functions
 ;; ----------------------------------------------------------------------
 
@@ -668,20 +681,11 @@ bs-mode
   (add-hook 'kill-buffer-hook 'bs--remove-hooks nil t)
   (add-hook 'change-major-mode-hook 'bs--remove-hooks nil t))
 
-(defun bs--restore-window-config ()
-  "Restore window configuration on the current frame."
-  (when bs--window-config-coming-from
-    (let ((frame (selected-frame)))
-      (unwind-protect
-	   (set-window-configuration bs--window-config-coming-from)
-	(select-frame frame)))
-    (setq bs--window-config-coming-from nil)))
-
 (defun bs-kill ()
   "Let buffer disappear and reset window configuration."
   (interactive)
   (bury-buffer (current-buffer))
-  (bs--restore-window-config))
+  (quit-window))
 
 (defun bs-abort ()
   "Ding and leave Buffer Selection Menu without a selection."
@@ -742,7 +746,7 @@ bs-select
   (interactive)
   (let ((buffer (bs--current-buffer)))
     (bury-buffer (current-buffer))
-    (bs--restore-window-config)
+    (quit-window)
     (switch-to-buffer buffer)
     (when bs--marked-buffers
       ;; Some marked buffers for selection
@@ -765,7 +769,7 @@ bs-select-other-window
   (interactive)
   (let ((buffer (bs--current-buffer)))
     (bury-buffer (current-buffer))
-    (bs--restore-window-config)
+    (quit-window)
     (switch-to-buffer-other-window buffer)))
 
 (defun bs-tmp-select-other-window ()
@@ -781,7 +785,7 @@ bs-select-other-frame
   (interactive)
   (let ((buffer (bs--current-buffer)))
     (bury-buffer (current-buffer))
-    (bs--restore-window-config)
+    (quit-window)
     (switch-to-buffer-other-frame buffer)))
 
 (defun bs-mouse-select-other-frame (event)
@@ -1438,21 +1442,11 @@ bs--show-with-configuration
       ;; Only when not in buffer *buffer-selection*
       ;; we have to set the buffer we started the command
       (setq bs--buffer-coming-from (current-buffer)))
-    (let ((liste (bs-buffer-list))
-	  (active-window (get-window-with-predicate
-			  (lambda (w)
-			    (string= (buffer-name (window-buffer w))
-				     "*buffer-selection*"))
-			  nil (selected-frame))))
-      (if active-window
-	  (select-window active-window)
-	(bs--restore-window-config)
-	(setq bs--window-config-coming-from (current-window-configuration))
-	(when (> (window-height) 7)
-          ;; Errors would mess with the window configuration (bug#10882).
-          (ignore-errors (select-window (split-window-below)))))
-      (bs-show-in-buffer liste)
-      (bs-message-without-log "%s" (bs--current-config-message)))))
+    (let ((window-combination-limit 'window-size))
+      (pop-to-buffer (get-buffer-create "*buffer-selection*")
+		     bs-default-action-list))
+    (bs-show-in-buffer (bs-buffer-list))
+    (bs-message-without-log "%s" (bs--current-config-message))))
 
 (defun bs--configuration-name-for-prefix-arg (prefix)
   "Convert prefix argument PREFIX to a name of a buffer configuration.
-- 
2.38.1.windows.1

Re: Revamping bs.el

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Tue, 6 Dec 2022 03:13:15 +0100
> 
> So, at this point,
> 
> 1) Is there some interest (or, at the very least, indifference ;-) in my revamping bs.el to work better with
> display-buffer-alist?
> 2) If so, it's ok to install the change in master?

Yes, on both counts.

Thanks.

Re: Revamping bs.el

[Juanma Barranquero]

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

Thanks, I'll install it.

The patch is missing a NEWS entry, but I prefer to wait until the second
part of my upgrade is done (or abandoned for good), if you don't mind.

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

Re: Revamping bs.el

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Tue, 6 Dec 2022 13:23:16 +0100
> Cc: emacs-devel@gnu.org
> 
> Thanks, I'll install it.
> 
> The patch is missing a NEWS entry, but I prefer to wait until the second part of my upgrade is done (or
> abandoned for good), if you don't mind.

I thought this first patch had (almost) no user-visible changes?

Re: Revamping bs.el

[Juanma Barranquero]

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

On Tue, Dec 6, 2022 at 1:30 PM Eli Zaretskii <eliz@gnu.org> wrote:

> I thought this first patch had (almost) no user-visible changes?

Just one, bs-default-action-list, but I think NEWS should eventually
mention both the variable and the fact that bs' *buffer-selection* can now
be configured via display-buffer-alist.

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

Re: Revamping bs.el

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Tue, 6 Dec 2022 13:37:09 +0100
> Cc: emacs-devel@gnu.org
> 
> On Tue, Dec 6, 2022 at 1:30 PM Eli Zaretskii <eliz@gnu.org> wrote:
> 
> > I thought this first patch had (almost) no user-visible changes?
> 
> Just one, bs-default-action-list, but I think NEWS should eventually mention both the variable and the fact
> that bs' *buffer-selection* can now be configured via display-buffer-alist.

Up to you.  Adding a short text to NEWS about this should not take you more
than a few moments, so consider doing that anyway, for the benefit of the
people who track the master branch.

Re: Revamping bs.el

[Juanma Barranquero]

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

On Tue, Dec 6, 2022 at 3:24 PM Eli Zaretskii <eliz@gnu.org> wrote:


>  Adding a short text to NEWS about this should not take you more
> than a few moments
>

You're an optimist, I see. I spent fifteen minutes for a two-lines comment
and I'm wildly unsatisfied with the result. Feel free to beat it into
proper-English submission.

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

Re: Revamping bs.el

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Wed, 7 Dec 2022 02:39:48 +0100
> Cc: emacs-devel@gnu.org
> 
> On Tue, Dec 6, 2022 at 3:24 PM Eli Zaretskii <eliz@gnu.org> wrote:
>  
>   Adding a short text to NEWS about this should not take you more
>  than a few moments
> 
> You're an optimist, I see. I spent fifteen minutes for a two-lines comment and I'm wildly unsatisfied with the
> result. Feel free to beat it into proper-English submission.

It was fine, except that it used passive voice.  Now fixed.

Thanks.

Re: Revamping bs.el

[Juanma Barranquero]

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

There are a few errors in the current bs.el that nobody has reported,
likely because
nobody is using the functionality.

- The docstring for `bs-attributes-list' says that there are four fields
(HEADER,
  MINIMUM-LENGTH, MAXIMUM-LENGTH and FUN-OR-STRING) that can either be a
value
  (string for HEADER and FUN-OR-STRING, a number for the LENGTH fields) or
a function
  that returns the relevant value. And then:

  The function gets as parameter the buffer where we have started
  buffer selection and the list of all buffers to show.  The function must
  return a string representing the column's value.

but the first sentence is only true for FUN-OR-STRING. The only case of
MINIMUM-LENGTH being a function in bs.el is called without arguments (and
presumably the
MAXIMUM-LENGTH function, if any, would too, but that column is 'currently
ignored' since
forever). There are no examples of a HEADER function, but presumably it
would also be
called without arguments (as it is relevant to the header line, and not the
content).

- Then there's `bs--make-header-match-string', which totally ignores the
possibility of
  HEADER being a function. Also, it constructs a regexp by concatenating
strings without
  regexp-quote'ing them. But there's no guarantee anywhere that a column
title can not be
  something like "THIS*MUCH" or "Money$" or whatever.

`bs--make-header-match' is an internal function, and will disappear in 30.1
with my
tabulated-list changes, because it's no longer relevant. I can "fix" it in
29.1, but I'd
prefer not to do it, as obviously the "call function to get the column
header"
functionality seems to have been mostly ignored. There's not much point in
introducing a
change now that could lead to other bugs.

Fixing the docstring of `bs-attributes-list' is another matter. I'd like to
document the
profile parameter of HEADER and MINIMUM-LENGTH, reduce the description of
MAXIMUM-LENGTH to just "(currently ignored)" and clarify that the last
paragraph
of the docstring is talking only about FUN-OR-STRING.

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

Re: Revamping bs.el

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Mon, 12 Dec 2022 23:48:16 +0100
> Cc: emacs-devel@gnu.org
> 
> There are a few errors in the current bs.el that nobody has reported, likely because
> nobody is using the functionality.

Thanks, it is fine with me to make all those cleanups and improvements
on the master branch.

Re: Revamping bs.el

[Juanma Barranquero]

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

I was wondering whether perhaps the docstring-only fix to
bs-attributes-list should go in the release branch instead.

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

Re: Revamping bs.el

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Tue, 13 Dec 2022 13:31:29 +0100
> Cc: emacs-devel@gnu.org
> 
> I was wondering whether perhaps the docstring-only fix to bs-attributes-list should go in the release branch
> instead.

Documentation changes, let alone fixes, are always okay for the
release branch.  You don't need to ask about that.