bug#37856: 27.0.50; 'next-error' advised with 'recenter' signals error when run from "*grep*" buffer

bug#37856: 27.0.50; 'next-error' advised with 'recenter' signals error when run from "*grep*" buffer

[Stefan Kangas]

I get an error when advising 'next-error' with 'recenter'.

Steps to reproduce:
1. Under "emacs -Q", evaluate:
    (defun my-advice (&rest args)
      (recenter))
    (advice-add 'next-error :after #'my-advice)
2. M-x rgrep RET [... grep for something]
3. Run 'next-error' (M-g M-n) from the "*grep*" buffer.

Result: I get the error:
    recenter-top-bottom: ‘recenter’ing a window that does not display
current-buffer.

(I also get the error when I run it in the second window when
'next-error' would take you to a different file.)

Expected result: I get no error.

---

Backtrace:
Debugger entered--Lisp error: (error "‘recenter’ing a window that does
not display curre...")
  recenter()
  my-advice(nil)
  apply(my-advice nil)
  next-error(nil)
  funcall-interactively(next-error nil)
  #<subr call-interactively>(next-error nil nil)
  apply(#<subr call-interactively> next-error (nil nil))
  call-interactively@ido-cr+-record-current-command(#<subr
call-interactively> next-error nil nil)
  apply(call-interactively@ido-cr+-record-current-command #<subr
call-interactively> (next-error nil nil))
  call-interactively(next-error nil nil)
  command-execute(next-error)

---

This seems to be due to this code in window.c:

  /* For reasons why we signal an error here, see
     https://lists.gnu.org/r/emacs-devel/2014-06/msg00053.html,
     https://lists.gnu.org/r/emacs-devel/2014-06/msg00094.html.  */
  if (buf != current_buffer)
    error ("`recenter'ing a window that does not display current-buffer.");

This code was added here:
3da983f8c4 2014-06-06 Stefan Monnier * src/window.c (Frecenter):
Signal an error if window-buffer is not current-buffer.

---

In GNU Emacs 27.0.50 (build 1, x86_64-apple-darwin17.7.0, NS
appkit-1561.61 Version 10.13.6 (Build 17G8037))
 of 2019-10-21 built on Stefans-MBP
Repository revision: 0e6f4628d8fff53505e4399e71da9f531a64fff7
Repository branch: master
Windowing system distributor 'Apple', version 10.3.1561
System Description:  Mac OS X 10.13.6

bug#37856: 27.0.50; 'next-error' advised with 'recenter' signals error when run from "*grep*" buffer

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Mon, 21 Oct 2019 20:38:47 +0200
> 
> 1. Under "emacs -Q", evaluate:
>     (defun my-advice (&rest args)
>       (recenter))
>     (advice-add 'next-error :after #'my-advice)
> 2. M-x rgrep RET [... grep for something]
> 3. Run 'next-error' (M-g M-n) from the "*grep*" buffer.
> 
> Result: I get the error:
>     recenter-top-bottom: ‘recenter’ing a window that does not display
> current-buffer.

Why isn't this a cockpit error: you force a function that happens to
switch buffers to recenter.  It's like you've added a call to
'recenter' in some arbitrary place in a random function: you are
responsible for that code, and if it does stuff that it shouldn't,
your code is wrong.  No?

> This seems to be due to this code in window.c:
> 
>   /* For reasons why we signal an error here, see
>      https://lists.gnu.org/r/emacs-devel/2014-06/msg00053.html,
>      https://lists.gnu.org/r/emacs-devel/2014-06/msg00094.html.  */
>   if (buf != current_buffer)
>     error ("`recenter'ing a window that does not display current-buffer.");

Yes, and that's on purpose, see the cited discussion.

bug#37856: 27.0.50; 'next-error' advised with 'recenter' signals error when run from "*grep*" buffer

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

> > 1. Under "emacs -Q", evaluate:
> >     (defun my-advice (&rest args)
> >       (recenter))
> >     (advice-add 'next-error :after #'my-advice)
> > 2. M-x rgrep RET [... grep for something]
> > 3. Run 'next-error' (M-g M-n) from the "*grep*" buffer.
> >
> > Result: I get the error:
> >     recenter-top-bottom: ‘recenter’ing a window that does not display
> > current-buffer.
>
> Why isn't this a cockpit error: you force a function that happens to
> switch buffers to recenter.  It's like you've added a call to
> 'recenter' in some arbitrary place in a random function: you are
> responsible for that code, and if it does stuff that it shouldn't,
> your code is wrong.  No?

In this case my expectation was that this code would be run *after*
the command, i.e. when it had already done all the switching.  I'm not
sure that's comparable to placing recenter in an arbitrary place
inside the function.

It was surprising to me that this didn't work, and it's not clear what
I should do instead.  For example, the following code exhibits the
same problem:

(defun my-next-error ()
  (interactive)
  (next-error)
  (recenter))

What am I missing?

Best regards,
Stefan Kangas

bug#37856: 27.0.50; 'next-error' advised with 'recenter' signals error when run from "*grep*" buffer

[Juanma Barranquero]

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

On Mon, Oct 21, 2019 at 10:13 PM Stefan Kangas <stefan@marxist.se> wrote:

> (defun my-next-error ()
>   (interactive)
>   (next-error)
>   (recenter))
>
> What am I missing?

What's wrong with

  (add-hook 'next-error-hook 'recenter)

?

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

bug#37856: 27.0.50; 'next-error' advised with 'recenter' signals error when run from "*grep*" buffer

[Stefan Kangas]

Juanma Barranquero <lekktu@gmail.com> writes:

> > What am I missing?
>
> What's wrong with
>
>   (add-hook 'next-error-hook 'recenter)

Oops, how silly of me.  Yes, I'll just use that.  Thanks.

I'm still a bit confounded by the above behaviour, but if everyone
else feels that everything is in order I'll just go ahead and close
this.  Sorry about the noise.

Best regards,
Stefan Kangas

bug#37856: 27.0.50; 'next-error' advised with 'recenter' signals error when run from "*grep*" buffer

[Robert Pluim]

>>>>> On Tue, 22 Oct 2019 00:19:13 +0200, Stefan Kangas <stefan@marxist.se> said:

    Stefan> Juanma Barranquero <lekktu@gmail.com> writes:
    >> > What am I missing?
    >> 
    >> What's wrong with
    >> 
    >> (add-hook 'next-error-hook 'recenter)

    Stefan> Oops, how silly of me.  Yes, I'll just use that.  Thanks.

    Stefan> I'm still a bit confounded by the above behaviour, but if everyone
    Stefan> else feels that everything is in order I'll just go ahead and close
    Stefan> this.  Sorry about the noise.

The add-function docstring says:

    Add a piece of advice on the function stored at PLACE.
    FUNCTION describes the code to add.  WHERE describes where to add it.
    WHERE can be explained by showing the resulting new function, as the
    result of combining FUNCTION and the previous value of PLACE, which we
    call OLDFUN here:
    `:before'	(lambda (&rest r) (apply FUNCTION r) (apply OLDFUN r))
    `:after'	(lambda (&rest r) (prog1 (apply OLDFUN r) (apply FUNCTION r)))
    `:around'	(lambda (&rest r) (apply FUNCTION OLDFUN r))

so I think Iʼd agree with your expectations. Of course, you could use
:around and call 'next-error' yourself in your function (untested).

Robert

bug#37856: 27.0.50; 'next-error' advised with 'recenter' signals error when run from "*grep*" buffer

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Mon, 21 Oct 2019 22:11:50 +0200
> Cc: 37856@debbugs.gnu.org
> 
> It was surprising to me that this didn't work, and it's not clear what
> I should do instead.  For example, the following code exhibits the
> same problem:
> 
> (defun my-next-error ()
>   (interactive)
>   (next-error)
>   (recenter))
> 
> What am I missing?

The advice effectively "injects" your code into the advised function,
just before it returns.