ivy-switch-buffer does not restore the correct environment for post-command-hook

[darkfeline]

With the built-in switch-to-buffer, the current-buffer while
post-command-hook is running is the buffer that was switched to.

With ivy-switch-buffer, the current-buffer while
post-command-hook is running is the buffer that was switched
from.

This neatly breaks all post-command-hook functions that rely on
current-buffer being correct.

ivy version is 20180809.1712

[darkfeline]

Looking at ivy--switch-buffer-action, I suspect that the problem is that post-command-hook is running outside of the with-ivy-window environment (the proper fix is probably different however).

When I print this-command from post-command-hook, switch-to-buffer fires post-command-hook twice, once when invoking the command and once when exiting the minibuffer, with this-command = switch-to-buffer. ivy-switch-buffer fires post-command-hook at the same points, when invoking the command this-command = ivy-switch-buffer, when exiting the minibuffer this-command = ivy-done

[basil-conto]

This neatly breaks all post-command-hook functions that rely on
current-buffer being correct.

WDYM by "correct"? Is this requirement documented somewhere? In other words, are these post-command-hook functions you speak of correct to assume things about the current buffer during a buffer switch operation? Unless this is documented somewhere, it doesn't sound like a reliable condition.

[darkfeline]

That's a good question. The expectation is simply stated as "run after executing each command". Thus, I would expect current-buffer, after running a buffer switching command like ivy-switch-buffer, to be the buffer I switched to.

So strictly speaking, the current behavior isn't violating any explicitly documented constraints, but I think the current behavior is disingenuous.

Edit: here's the manual page https://www.gnu.org/software/emacs/manual/html_node/elisp/Command-Overview.html

[basil-conto]

The expectation is simply stated as "run after executing each command". Thus, I would expect current-buffer, after running a buffer switching command like ivy-switch-buffer, to be the buffer I switched to.

Right, thanks for clarifying @darkfeline.

[darkfeline]

I patched this locally by defining around advice like:

;; Macro for defining a func and adding as around advice.
(mir-nero--define ivy-switch-buffer (old)
  "Fix bug https://github.com/abo-abo/swiper/issues/1727"
  (funcall old)
  (set-buffer (window-buffer (selected-window))))

After observing that after the (funcall old), the selected-window is as expected but current-buffer is not. I guess current-buffer is updated according to selected-window at the beginning of the command loop?

[darkfeline]

When an editing command returns to the editor command loop, Emacs automatically calls set-buffer on the buffer shown in the selected window. This is to prevent confusion: it ensures that the buffer that the cursor is in, when Emacs reads a command, is the buffer to which that command applies (see Command Loop). Thus, you should not use set-buffer to switch visibly to a different buffer; for that, use the functions described in Switching Buffers.

When writing a Lisp function, do not rely on this behavior of the command loop to restore the current buffer after an operation. Editing commands can also be called as Lisp functions by other programs, not just from the command loop; it is convenient for the caller if the subroutine does not change which buffer is current (unless, of course, that is the subroutine's purpose).

https://www.gnu.org/software/emacs/manual/html_node/elisp/Current-Buffer.html

[abo-abo]

@darkfeline with-ivy-window wrapper is legacy in actions, it's not needed anymore.

Do you still get a problem with this version:

(defun ivy--switch-buffer-action (buffer)
  "Switch to BUFFER.
BUFFER may be a string or nil."
  (if (zerop (length buffer))
      (switch-to-buffer
       ivy-text nil 'force-same-window)
    (let ((virtual (assoc buffer ivy--virtual-buffers))
          (view (assoc buffer ivy-views)))
      (cond ((and virtual
                  (not (get-buffer buffer)))
             (find-file (cdr virtual)))
            (view
             (delete-other-windows)
             (let (
                   ;; silence "Directory has changed on disk"
                   (inhibit-message t))
               (ivy-set-view-recur (cadr view))))
            (t
             (switch-to-buffer
              buffer nil 'force-same-window))))))

Please test with this version and your advice disabled and let me know.

[darkfeline]

@abo-abo That doesn't fix the problem.

[abo-abo]

@darkfeline Could you reformulate your advice as a patch to ivy--switch-buffer-action?

[darkfeline]

Unfortunately, I don't think it's possible to fix this in ivy--switch-buffer-action. Something higher up the call stack is resetting the current buffer.

(mir-nero--define ivy-switch-buffer (old)
  "Fix bug https://github.com/abo-abo/swiper/issues/1727"
  (funcall old)
  (let ((inhibit-message t))
    (message "@@@@ Cbuffer %s" (current-buffer))
    (message "@@@@ Cwindow %s" (window-buffer (selected-window)))))

(mir-nero--define ivy--switch-buffer-action (old buf)
  "Fix bug https://github.com/abo-abo/swiper/issues/1727"
  (funcall old buf)
  (let ((inhibit-message t))
    (message "@@@@ Abuffer %s" (current-buffer))
    (message "@@@@ Awindow %s" (window-buffer (selected-window))))
  (set-buffer (window-buffer (selected-window)))
  (let ((inhibit-message t))
    (message "@@@@ Bbuffer %s" (current-buffer))
    (message "@@@@ Bwindow %s" (window-buffer (selected-window)))))

@@@@ Abuffer *scratch*
@@@@ Awindow mir-nero.el
@@@@ Bbuffer mir-nero.el
@@@@ Bwindow mir-nero.el
@@@@ Cbuffer *scratch*
@@@@ Cwindow mir-nero.el