Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ivy.el (ivy-call): Restore previous buffer #1607

Closed
wants to merge 1 commit into from

Conversation

raxod502
Copy link
Contributor

@raxod502 raxod502 commented Jun 1, 2018

In cases where set-buffer was called before ivy-read, the correct window was restored but the effects of set-buffer were lost. Restoring the buffer explicitly fixes the problem.

Example code to reproduce the issue:

(with-current-buffer "*scratch*"
  (set-buffer "*Messages*")
  (ivy-read "Select one: " (list "X"))
  (pcase (buffer-name)
    ("*scratch*" (message "Buffer was not restored correctly."))
    ("*Messages*" (message "Buffer was restored correctly."))
    (_ (message "Something unexpected has happened."))))

@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented Jun 1, 2018

Running your recipe from make plain, either in the *scratch* buffer or an arbitrary one, always yields "Buffer was restored correctly." for me, on all Emacs versions 24-27. What am I missing?


Swiper version: 7df7ab6
Emacs verson:

In GNU Emacs 27.0.50 (build 5, x86_64-pc-linux-gnu, X toolkit, Xaw3d scroll bars)
 of 2018-05-31 built on thunk
Repository revision: fb9c52bb7d29c85f3baee770355260830dacea50
Windowing system distributor 'The X.Org Foundation', version 11.0.11906000
System Description: Debian GNU/Linux buster/sid

@raxod502
Copy link
Contributor Author

@raxod502 raxod502 commented Jun 2, 2018

Oops! Let me look into this further. I know something is wrong, but I think I probably misidentified the problem in this pull request :(

@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented Jun 2, 2018

I know something is wrong, but I think I probably misidentified the problem in this pull request

Or perhaps the recipe above just doesn't showcase the problem?

@raxod502
Copy link
Contributor Author

@raxod502 raxod502 commented Jun 2, 2018

I think that it does showcase the problem, but the problem only exists due to some other package or configuration that was active when I did the testing.

@raxod502
Copy link
Contributor Author

@raxod502 raxod502 commented Jun 2, 2018

Welp, I did a lot of bisecting, and here's a new recipe that works from emacs -Q:

(with-current-buffer "*scratch*"
  (set-buffer "*Messages*")
  (ivy-read "Select one: " (list "X") :action (lambda (x) x))
  (pcase (buffer-name)
    ("*scratch*" (message "Buffer was not restored correctly."))
    ("*Messages*" (message "Buffer was restored correctly."))
    (_ (message "Something unexpected has happened."))))

Note the explicit :action. If you substitute 'identity instead of (lambda (x) x), then the issue does not reproduce. This is because for some reason there is an explicit check against the symbol 'identity here:

swiper/ivy.el

Line 1219 in 7df7ab6

(if (eq action 'identity)

In case you were wondering, the reason the original recipe was working for me was because I was using ivy-prescient.el, which adds an advice to ivy-read that causes :action to always be non-nil.

In cases where `set-buffer' was called before `ivy-read', the correct
window was restored but the effects of `set-buffer' were lost.
Restoring the buffer explicitly fixes the problem.

Evaluate the following code to check if the bug is fixed:

(with-current-buffer "*scratch*"
  (set-buffer "*Messages*")
  (ivy-read "Select one: " (list "X") :action (lambda (x) x))
  (pcase (buffer-name)
    ("*scratch*" (message "Buffer was not restored correctly."))
    ("*Messages*" (message "Buffer was restored correctly."))
    (_ (message "Something unexpected has happened."))))
@raxod502 raxod502 force-pushed the feat/restore-set-buffer branch from 6b515ee to 6a7aeac Compare Jun 2, 2018
@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented Jun 2, 2018

Welp, I did a lot of bisecting, and here's a new recipe that works from emacs -Q:

Nice.

This is because for some reason there is an explicit check against the symbol 'identity here:

Not sure what the purpose of that is, but even if there is a valid one, it can nevertheless be simplified:

diff --git a/ivy.el b/ivy.el
index f8aceb2..83c46dd 100644
--- a/ivy.el
+++ b/ivy.el
@@ -1216,8 +1216,8 @@ ivy-call
                      ivy-text)
                     (t
                      (ivy-state-current ivy-last)))))
-          (if (eq action 'identity)
-              (funcall action x)
+          (if (eq action #'identity)
+              x
             (select-window (ivy--get-window ivy-last))
             (prog1 (with-current-buffer (ivy-state-buffer ivy-last)
                      (unwind-protect (funcall action x)

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Jun 13, 2018

I like the fix proposed by @basil-conto more. @raxod502 does it solve your problem? In case it does I suggest we close this PR and apply the fix by @basil-conto instead.

@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented Jun 13, 2018

@abo-abo My patch (#1607 (comment)) is not a solution to @raxod502's issue; it was just a suggestion for a truly trivial additional simplification.

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Jun 14, 2018

Thanks.

@raxod502 raxod502 deleted the feat/restore-set-buffer branch Jun 14, 2018
basil-conto added a commit to basil-conto/swiper that referenced this issue Sep 26, 2018
The call to with-current-buffer was made redundant by the preceding
set-buffer added in abo-abo#1607, and incorrectly restores ivy-state-buffer
after calling action.

(ivy-call): Remove call to with-current-buffer.

Closes abo-abo#1766
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants