kill-buffer calls frame's buffer-predicate for all buffers even if the killed buffer was not shown in any window.

kill-buffer calls frame's buffer-predicate for all buffers even if the killed buffer was not shown in any window.

[Constantin Kulikov]

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

For example:

emacs -Q

M-: (set-frame-parameter nil 'buffer-predicate #'(lambda (b) (message
"f-b-p: %s" b))) RET
M-: (with-temp-buffer nil) RET
C-x b *Messages* RET

See that all buffers listed except of the *temp*(no window buffers were
changed). Why?
This is inefficient, especially for the with-temp-buffer which is used very
frequently in emacs and in side packages.

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

Re: kill-buffer calls frame's buffer-predicate for all buffers even if the killed buffer was not shown in any window.

[Eli Zaretskii]

> From: Constantin Kulikov <zxnotdead@gmail.com>
> Date: Sun, 15 Jan 2017 22:32:10 +0300
> 
> For example:
> 
> emacs -Q
> 
> M-: (set-frame-parameter nil 'buffer-predicate #'(lambda (b) (message "f-b-p: %s" b))) RET
> M-: (with-temp-buffer nil) RET
> C-x b *Messages* RET
> 
> See that all buffers listed except of the *temp*(no window buffers were changed). Why?

Because it needs to find another buffer to become the current one.
And it only searches until it finds the first buffer that is not
already visible in some window.  Your recipe just leaves it with too
few buffers, so it looks to you like it goes over all of them.

> This is inefficient, especially for the with-temp-buffer which is used very frequently in emacs and in side
> packages.

As long as with-temp-buffer calls kill-buffer, there's no way around
it, because kill-buffer is a command that must fit user expectations.
We could perhaps introduce a new kill-temp-buffer primitive to avoid
the overhead.

Re: kill-buffer calls frame's buffer-predicate for all buffers even if the killed buffer was not shown in any window.

[Constantin Kulikov]

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

> it needs to find another buffer to become the current one
The current in what sense? In the sense of 1)(current-buffer) or 2)
(windown-buffer (selected-window))?
If 1) then why require that this buffer is not visible?
If 2) then why call the buffer-predicate in the case where the killed
buffer was not shown in any window?

What I understand of how kill-buffer must work:
First call kill-buffer hooks
Then find windows with the buffer and replace it, here the call to
buffer-predicate it legit if the buffer was visible in any window.
Then really kill the buffer.
At this point we can set selected window buffer as current because we
already have called the replace-buffer-in-window.
If there is no selected window, then we can take the first buffer in
(buffer-list) as current
or create the *sctatch* and set it as current.

I can not see why there is a need to call buffer-predicate when killing a
buffer that was not visible in any window.)

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

Re: kill-buffer calls frame's buffer-predicate for all buffers even if the killed buffer was not shown in any window.

[Eli Zaretskii]

> From: Constantin Kulikov <zxnotdead@gmail.com>
> Date: Mon, 16 Jan 2017 11:14:10 +0300
> Cc: emacs-devel <emacs-devel@gnu.org>
> 
> > it needs to find another buffer to become the current one
> The current in what sense? In the sense of 1)(current-buffer) or 2) (windown-buffer (selected-window))?

I meant 1), but it does 2) as well.

> If 1) then why require that this buffer is not visible?

Because it makes the most sense when kill-buffer is invoked
interactively.  Replacing the killed buffer with a buffer that is
displayed in some other window would be less useful than showing a
buffer not displayed anywhere else.

> What I understand of how kill-buffer must work:
> First call kill-buffer hooks
> Then find windows with the buffer and replace it, here the call to buffer-predicate it legit if the buffer was visible
> in any window.
> Then really kill the buffer.
> At this point we can set selected window buffer as current because we already have called the
> replace-buffer-in-window.
> If there is no selected window, then we can take the first buffer in (buffer-list) as current
> or create the *sctatch* and set it as current.

It sounds like you are assuming that current-buffer and the buffer
shown in the selected window are the same buffer.  But that doesn't
have to be true: a Lisp program could very well switch to another
buffer while doing its job.  And since Emacs must have a valid
current-buffer at all times, it must choose another one when the
current buffer is about to be killed.

> I can not see why there is a need to call buffer-predicate when killing a buffer that was not visible in any
> window.)

Once again, kill-buffer (and other-buffer, which kill-buffer calls,
and which actually calls the predicate) are interactive commands, so
their implementation is heavily biased towards doing what the user who
invoked these commands wants and expects.  I can see your point about
the same logic being of questionable value when with-temp-buffer kills
its temporary buffer, but I don't think we should consider changing
how kill-buffer and other-buffer work to make that use case more
efficient and its overhead smaller.  That's because these two commands
work like that for a very long time, and evidently do what the users
expect (the evidence being that I don't remember any complaints about
that).  So I'd suggest to leave these two commands alone, and instead
consider adding some new function, say kill-temp-buffer, which will
only do a small subset of what kill-buffer does, and disregard stuff
like visibility of candidate buffers to which current-buffer needs to
switch.

Re: kill-buffer calls frame's buffer-predicate for all buffers even if the killed buffer was not shown in any window.

[Eli Zaretskii]

> Date: Mon, 16 Jan 2017 18:26:55 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: emacs-devel@gnu.org
> 
> So I'd suggest to leave these two commands alone, and instead
> consider adding some new function, say kill-temp-buffer, which will
> only do a small subset of what kill-buffer does, and disregard stuff
> like visibility of candidate buffers to which current-buffer needs
> to switch.

Btw, you didn't describe the use case where this issue got in your
way, so perhaps you should.  That would allow to decide whether the
problems are bad enough to look for a solution.

Thanks.

Re: kill-buffer calls frame's buffer-predicate for all buffers even if the killed buffer was not shown in any window.

[Constantin Kulikov]

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

Ok, here is my attempt to code this:
https://github.com/Bad-ptr/emacs/commit/cf6b0d9f08dbdc5dd685dbc6a5ef9ff18575e2b2
This seem to work for my example from the start of discussion(not tested it
much for now).

> you didn't describe the use case where this issue got in your
way, so perhaps you should.

If you set a 'heay' buffer-predicate like here
https://github.com/Bad-ptr/persp-mode.el/blob/master/persp-mode.el#L2793
(This package provide a way to switch between 'projects',
each project has a list of buffers, it is essential to set a
buffer-predicate that
will constrain buffer switching to the current 'project' so it needs to
check if a
buffer is a member of the current project list. There may be many buffers
from
many projects)
And if you are the user of a package that overuse the with-temp-buffer(like
helm
or if you run down the list of candidates of the company-mode) you can
notice a
slowdown.
https://github.com/Bad-ptr/persp-mode.el/issues/35

And I just think that this behavior of the kill-buffer is wrong and could
be fixed
quite easily and without introducing new functions or breaking user
experience.
Maybe I'm wrong.
And I'm not saying that this is a critical bug or somehow urgent, just
wanted to
discuss the idea.

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

Re: kill-buffer calls frame's buffer-predicate for all buffers even if the killed buffer was not shown in any window.

[Stefan Monnier]

> Ok, here is my attempt to code this:
> https://github.com/Bad-ptr/emacs/commit/cf6b0d9f08dbdc5dd685dbc6a5ef9ff18575e2b2
> This seem to work for my example from the start of discussion(not tested it
> much for now).

Here's an alternative, which reuses the existing `other_buffer_safely`,
and which only modifies the behavior in the case that the buffer is not
displayed in any window.  I included in it an optimization: don't call
replace_buffer_in_windows if the buffer is not displayed.


        Stefan


diff --git a/src/buffer.c b/src/buffer.c
index d62c79df09..418ab3698e 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -1642,6 +1642,7 @@ cleaning up all windows currently displaying the buffer to be killed.  */)
   struct buffer *b;
   Lisp_Object tem;
   struct Lisp_Marker *m;
+  bool visible;
 
   if (NILP (buffer_or_name))
     buffer = Fcurrent_buffer ();
@@ -1725,11 +1726,14 @@ cleaning up all windows currently displaying the buffer to be killed.  */)
 	return Qt;
     }
 
+  visible = buffer_window_count (XBUFFER (buffer));
+
   /* Run replace_buffer_in_windows before making another buffer current
      since set-window-buffer-start-and-point will refuse to make another
      buffer current if the selected window does not show the current
      buffer (bug#10114).  */
-  replace_buffer_in_windows (buffer);
+  if (visible)
+    replace_buffer_in_windows (buffer);
 
   /* Exit if replacing the buffer in windows has killed our buffer.  */
   if (!BUFFER_LIVE_P (b))
@@ -1739,7 +1743,8 @@ cleaning up all windows currently displaying the buffer to be killed.  */)
      buffer.  */
   if (b == current_buffer)
     {
-      tem = Fother_buffer (buffer, Qnil, Qnil);
+      tem = visible ? Fother_buffer (buffer, Qnil, Qnil)
+            : other_buffer_safely (buffer);
       Fset_buffer (tem);
       if (b == current_buffer)
 	return Qnil;

Re: kill-buffer calls frame's buffer-predicate for all buffers even if the killed buffer was not shown in any window.

[Constantin Kulikov]

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

New version )
https://github.com/Bad-ptr/emacs/commit/cbde2fcc3e9e1aef86148f9953aff24556b0a156

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

Re: kill-buffer calls frame's buffer-predicate for all buffers even if the killed buffer was not shown in any window.

[Constantin Kulikov]

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

Well I think Stefan's solution is more correct and will not break anything.

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

Re: kill-buffer calls frame's buffer-predicate for all buffers even if the killed buffer was not shown in any window.

[Eli Zaretskii]

> From: Constantin Kulikov <zxnotdead@gmail.com>
> Date: Wed, 18 Jan 2017 13:03:08 +0300
> Cc: emacs-devel <emacs-devel@gnu.org>
> 
> Well I think Stefan's solution is more correct and will not break anything.

I'd like to avoid any changes in behavior of kill-buffer that could
possibly affect existing code.  So if the change should be in
kill-buffer itself, please introduce an additional optional argument
that will control whether buffer visibility is considered and/or
whether the frame predicates are called; with-temp-buffer could then
use that new argument.

This function survived the last 5 years with no real changes, and even
before that most of the code which implements this logic wasn't
touched.  I see no reason to make such changes now in a very popular
function/command, for the benefit of very specialized use cases.

Thanks.