bug#38384: (next|previous)-buffer silent about not switching

bug#38384: (next|previous)-buffer silent about not switching

[Juanma Barranquero]

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

Package: emacs
Severity: minor
Tags: patch
X-Debbugs-Cc: rudalics@gmx.at

In some circumstances, next-buffer and previous-buffer do not have any
buffer to switch to, and in that case they silently do nothing.  (Strictly
speaking, they call switch-to-(next|prev)-buffer, which do nothing and
return nil).

I think they should throw a user-error, to inform the user about the
situation.

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

[-- Attachment #2: window.patch --]
[-- Type: application/octet-stream, Size: 841 bytes --]

diff --git i/lisp/window.el w/lisp/window.el
index 49fad75d3c..c8a5816b8c 100644
--- i/lisp/window.el
+++ w/lisp/window.el
@@ -4850,7 +4850,8 @@ next-buffer
     (user-error "Window is strongly dedicated to its buffer"))
    (t
     (dotimes (_ (or arg 1))
-      (switch-to-next-buffer)))))
+      (unless (switch-to-next-buffer)
+        (user-error "No next buffer"))))))
 
 (defun previous-buffer (&optional arg)
   "In selected window switch to ARGth previous buffer.
@@ -4864,7 +4865,8 @@ previous-buffer
     (user-error "Window is strongly dedicated to its buffer"))
    (t
     (dotimes (_ (or arg 1))
-      (switch-to-prev-buffer)))))
+      (unless (switch-to-prev-buffer)
+        (user-error "No previous buffer"))))))
 
 (defun delete-windows-on (&optional buffer-or-name frame)
   "Delete all windows showing BUFFER-OR-NAME.

bug#38384: (next|previous)-buffer silent about not switching

[martin rudalics]

 > In some circumstances, next-buffer and previous-buffer do not have any
 > buffer to switch to, and in that case they silently do nothing.  (Strictly
 > speaking, they call switch-to-(next|prev)-buffer, which do nothing and
 > return nil).
 >
 > I think they should throw a user-error, to inform the user about the
 > situation.

Right.  Please install your patch.

martin

bug#38384: (next|previous)-buffer silent about not switching

[Juanma Barranquero]

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

Committed as e495dbea7035bcb1f26ed82f0d56a5abc90974fa

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

bug#38384: (next|previous)-buffer silent about not switching

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Tue, 26 Nov 2019 13:08:12 +0100
> 
> In some circumstances, next-buffer and previous-buffer do not have any buffer to switch to, and in that case
> they silently do nothing.  (Strictly speaking, they call switch-to-(next|prev)-buffer, which do nothing and return
> nil).
> 
> I think they should throw a user-error, to inform the user about the situation.

Could that disrupt noninteractive calls?

bug#38384: (next|previous)-buffer silent about not switching

[Juanma Barranquero]

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

On Tue, Nov 26, 2019 at 4:42 PM Eli Zaretskii <eliz@gnu.org> wrote:

> Could that disrupt noninteractive calls?

Yes, that's why added the notice in NEWS in the "Incompatible Lisp Changes".

But, on one hand, programs should perhaps be using
switch-to-(prev|next)-buffer. AFAICS, next-buffer and previous-buffer
aren't even documented in the Elisp manual (they are briefly mentioned in
the Emacs manual).

 And on the other, if code is using (next|prev)-buffer, silently getting
the same buffer is likely a subtle bug anyway. At least now they get notice.

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

bug#38384: (next|previous)-buffer silent about not switching

[Juanma Barranquero]

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

After a cursory glance, I find only one instance of (next-buffer) called
from elisp in our sources, and it's in mode-line-next-buffer.  And in that
case, clicking on the buffer name in the modeline and getting a message "no
next buffer" seems like an improvement to me over the silent treatment ;-)

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

bug#38384: (next|previous)-buffer silent about not switching

[martin rudalics]

 >> Could that disrupt noninteractive calls?
 >
 > Yes, that's why added the notice in NEWS in the "Incompatible Lisp Changes".
 >
 > But, on one hand, programs should perhaps be using
 > switch-to-(prev|next)-buffer. AFAICS, next-buffer and previous-buffer
 > aren't even documented in the Elisp manual (they are briefly mentioned in
 > the Emacs manual).
 >
 >   And on the other, if code is using (next|prev)-buffer, silently getting
 > the same buffer is likely a subtle bug anyway. At least now they get notice.

Right.  'previous-buffer' and 'next-buffer' are intended for interactive use
only.  Maybe we should add appropriate warnings to their doc-strings.

martin

bug#38384: (next|previous)-buffer silent about not switching

[martin rudalics]

 > After a cursory glance, I find only one instance of (next-buffer) called
 > from elisp in our sources, and it's in mode-line-next-buffer.  And in that
 > case, clicking on the buffer name in the modeline and getting a message "no
 > next buffer" seems like an improvement to me over the silent treatment ;-)

Right again.  The same holds for 'mode-line-previous-buffer'.

martin

bug#38384: (next|previous)-buffer silent about not switching

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Tue, 26 Nov 2019 16:54:56 +0100
> Cc: 38384@debbugs.gnu.org
> 
> > Could that disrupt noninteractive calls?
> 
> Yes, that's why added the notice in NEWS in the "Incompatible Lisp Changes".

Then how about signaling user-error only in interactive calls?

> But, on one hand, programs should perhaps be using switch-to-(prev|next)-buffer. AFAICS, next-buffer and
> previous-buffer aren't even documented in the Elisp manual (they are briefly mentioned in the Emacs
> manual).
> 
>  And on the other, if code is using (next|prev)-buffer, silently getting the same buffer is likely a subtle bug
> anyway. At least now they get notice.

IMO, if we can improve the diagnostics only in interactive
invocations, we have all the advantages without any disadvantages (and
can move this out of the "incompatible changes" section).

bug#38384: (next|previous)-buffer silent about not switching

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Tue, 26 Nov 2019 17:00:35 +0100
> Cc: 38384@debbugs.gnu.org
> 
> After a cursory glance, I find only one instance of (next-buffer) called from elisp in our sources, and it's in
> mode-line-next-buffer.  And in that case, clicking on the buffer name in the modeline and getting a message
> "no next buffer" seems like an improvement to me over the silent treatment ;-)

Granted, I'm worried mainly about code outside of the Emacs tree.

bug#38384: (next|previous)-buffer silent about not switching

[Juanma Barranquero]

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

On Tue, Nov 26, 2019 at 7:04 PM Eli Zaretskii <eliz@gnu.org> wrote:

> Granted, I'm worried mainly about code outside of the Emacs tree.

Even before my patch, both functions can in some situations signal
user-error without checking for interactive use.

  (cond
   ((window-minibuffer-p)
    (user-error "Cannot switch buffers in minibuffer window"))
   ((eq (window-dedicated-p) t)
    (user-error "Window is strongly dedicated to its buffer"))

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

bug#38384: (next|previous)-buffer silent about not switching

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Tue, 26 Nov 2019 19:23:55 +0100
> Cc: 38384@debbugs.gnu.org
> 
> On Tue, Nov 26, 2019 at 7:04 PM Eli Zaretskii <eliz@gnu.org> wrote:
> 
> > Granted, I'm worried mainly about code outside of the Emacs tree.
> 
> Even before my patch, both functions can in some situations signal user-error without checking for interactive
> use.
> 
>   (cond
>    ((window-minibuffer-p)
>     (user-error "Cannot switch buffers in minibuffer window"))
>    ((eq (window-dedicated-p) t)
>     (user-error "Window is strongly dedicated to its buffer"))

I don't see how this is relevant to the issue at hand, what did I
miss?

bug#38384: (next|previous)-buffer silent about not switching

[Juanma Barranquero]

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

On Tue, Nov 26, 2019 at 7:44 PM Eli Zaretskii <eliz@gnu.org> wrote:

> I don't see how this is relevant to the issue at hand, what did I
> miss?

Sorry, I fail to see how could it not be relevant. Lisp code calling
(next|previous)-buffer already has to defend itself against a user-error.
My code just adds one specific case (that of the window's buffer list
having only one buffer) in which user-error is also signaled. Am I missing
something?

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

bug#38384: (next|previous)-buffer silent about not switching

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Tue, 26 Nov 2019 19:51:50 +0100
> Cc: 38384@debbugs.gnu.org
> 
> > I don't see how this is relevant to the issue at hand, what did I
> > miss?
> 
> Sorry, I fail to see how could it not be relevant. Lisp code calling (next|previous)-buffer already has to defend
> itself against a user-error. My code just adds one specific case (that of the window's buffer list having only one
> buffer) in which user-error is also signaled. Am I missing something?

We previously signaled user-error in situations where we couldn't
continue at all.  Your addition is in a situation where nothing
particularly bad happened, so from the POV of a caller, we are now
signaling a user-error gratuitously.  I'm bothered only by the change
whereby we signal a user-error with the purpose of attracting the
user's attention, not because we cannot continue.

bug#38384: (next|previous)-buffer silent about not switching

[Juanma Barranquero]

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

On Tue, Nov 26, 2019 at 8:36 PM Eli Zaretskii <eliz@gnu.org> wrote:

> We previously signaled user-error in situations where we couldn't
> continue at all.  Your addition is in a situation where nothing
> particularly bad happened, so from the POV of a caller, we are now
> signaling a user-error gratuitously.  I'm bothered only by the change
> whereby we signal a user-error with the purpose of attracting the
> user's attention, not because we cannot continue.

I don't really see much difference.

In the two cases that already signal user-error, "continuing" would mean
doing nothing; there would be no other bad consequence. In the case I've
changed, continuing means the same: doing nothing. In all three cases,
signaling, instead of continuing, is purely to attract the user's attention.

In my opinion, such code should call switch-to-(prev|next)-buffer instead.

Anyway, there's no point in arguing this; if you feel strongly that the
last case should depend on called-interactively-p, I'll change it. But I
think we should instead leave it as it is and educate code writers to use
the documented calls instead of the user-level commands.

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

bug#38384: (next|previous)-buffer silent about not switching

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Tue, 26 Nov 2019 20:56:42 +0100
> Cc: 38384@debbugs.gnu.org
> 
> > We previously signaled user-error in situations where we couldn't
> > continue at all.  Your addition is in a situation where nothing
> > particularly bad happened, so from the POV of a caller, we are now
> > signaling a user-error gratuitously.  I'm bothered only by the change
> > whereby we signal a user-error with the purpose of attracting the
> > user's attention, not because we cannot continue.
> 
> I don't really see much difference.

Well, I do.

> Anyway, there's no point in arguing this; if you feel strongly that the last case should depend on
> called-interactively-p, I'll change it.

Please do, and thanks.