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-read in temporary buffer #717

Closed
ericdanan opened this Issue Oct 19, 2016 · 12 comments

Comments

Projects
None yet
2 participants
@ericdanan
Contributor

ericdanan commented Oct 19, 2016

It seems that if I call ivy-read in a temporary buffer, select a candidate and choose an action, then the action is not executed in the temp buffer but in the buffer from which the temp buffer was created.

Example. I am in a buffer where default-directory is "dir1" and evaluate:

(with-temp-buffer
  (let ((default-directory "dir2"))
    (ivy-read "choose: "
          '("foo" "bar")
          :action (lambda (x)
               (message default-directory)))))

After I choose a candidate, the message is dir1 and not dir2.

Is this intended, or if not is there a simple way to make sure the action is run in the buffer in which ivy-read was called?

@abo-abo abo-abo closed this in e49fb6e Oct 20, 2016

@abo-abo

This comment has been minimized.

Owner

abo-abo commented Oct 20, 2016

Thanks, please test.

@ericdanan

This comment has been minimized.

Contributor

ericdanan commented Oct 20, 2016

Thanks, it works but actually I was just using default-directory as an example of a buffer-local variable that might be relevant for the ivy-read actions, there might be others. I should have been clearer, sorry about that.

For instance, projectile-switch-project uses a temp buffer to set default-directory to the project to switch to, load this project's directory-local variables and then execute projectile-switch-project-action. Hence if projectile-switch-project-action calls ivy-read, then the ivy-read actions won't be able to use the project's dir-locals.

I'm not sure if this would raise some issues, but perhaps

(select-window (ivy--get-window ivy-last))
(prog1 (let ((default-directory (ivy-state-directory ivy-last)))
              (funcall action x))

could be replaced with

(select-window (ivy--get-window ivy-last))
(prog1 (with-current-buffer (ivy-state-buffer ivy-last)))
              (funcall action x))

or simply

(select-window (ivy--get-window ivy-last))
(set-buffer (ivy-state-buffer ivy-last))
(prog1 (funcall action x)

?

abo-abo added a commit that referenced this issue Nov 29, 2016

@abo-abo

This comment has been minimized.

Owner

abo-abo commented Nov 29, 2016

The code I used to close this issue is not removed. It was causing too much trouble elsewhere.

could be replaced with

    (select-window (ivy--get-window ivy-last))
    (prog1 (with-current-buffer (ivy-state-buffer ivy-last)))
                  (funcall action x))

This needs to be well-tested. I'll apply this change to my config to check if anything bad happens. If it doesn't, I'll apply this change.

@ericdanan

This comment has been minimized.

Contributor

ericdanan commented Nov 29, 2016

Sorry about the trouble.

I also applied the change locally and tested a few things:

  • #717 (this issue). The code in my first post now correctly yields "dir2". I also tried switching project with projectile + ivy backend or with counsel-projectile (this creates a temp buffer with default-directory let-bound to the root of the project to switch to and calls the switching action projectile-find-file or counsel-projectile from this temp buffer). It works in both cases.

  • #760 . counsel-M-x cd sets default-directory correctly.

  • #779 . Switching back and forth between projects A and B as described in @kaushalmodi 's comment works correctly, again both with projectile+ivy and with counsel-projectile.

  • #810 . I checked that the code in @seudut 's first post works as expected.

Of course there could be other issues I didn't come across. Please let me know if I can help with testing.

PS you surely noticed that the parentheses are not balanced in the code I posted above, it should be:

(select-window (ivy--get-window ivy-last))
(prog1 (with-current-buffer (ivy-state-buffer ivy-last)
         (funcall action x))
@abo-abo

This comment has been minimized.

Owner

abo-abo commented Dec 4, 2016

@ericdanan Thanks for helping. I've applied the suggested change.

@ericdanan

This comment has been minimized.

Contributor

ericdanan commented Dec 4, 2016

You're welcome, hope it won't cause more trouble.

@ericdanan

This comment has been minimized.

Contributor

ericdanan commented Aug 11, 2017

I have actually found an undesirable (I think) effect of this with-current-buffer call: after the action is executed, the current buffer is systematically reset to what it was just before callling it, ie the buffer from (ivy-state-window ivy-last). This does not seem right to me when the action switches buffer. For instance, if I execute the following from buffer1

(progn
  (ivy-read "buffer: "
	    '("buffer2" "buffer3")
	    :action #'switch-to-buffer)
  (message (buffer-name)))

and choose eg buffer2, then the buffer is correctly switched to buffer2 but the message is buffer1.

The solution that seems natural to me is to permanently switch to the buffer from which ivy-read was called rather than just temporarily switch to it to execute the action. That is, to use set-buffer (or switch-to-buffer) instead of with-currrent-buffer like this:

(select-window (ivy--get-window ivy-last))
(set-buffer (ivy-state-buffer ivy-last))
(prog1 (unwind-protect (funcall action x)
          (ivy-recursive-restore)))

I have tested that this works for the same four issues as above : #717 (current issue), #760 , #779 and #810 . But I'm not sure if this could create other issues (perhaps there are situations where one deliberately switches buffer during the candidate selection and wants the action to run in the new buffer?)

Incidentally, if I rewrite the above example using ivy-switch-buffer like this:

(progn
  (ivy-switch-buffer)
  (buffer-name))

then the same problem occurs but my proposed solution does not work. This is apparently because ivy--switch-buffer-action relies on with-ivy-window which also resets the current buffer when restoring the window. Removing the with-ivy-window call solves the problem. I am not sure what is the role of this call since ivy-call already selects the same window.

@abo-abo

This comment has been minimized.

Owner

abo-abo commented Aug 12, 2017

This does not seem right to me when the action switches buffer.

I think it's fine: not perfect, but good enough.

The main reason why it's fine is that due to the possibility of :action being called multiple times in succession, and then possibly even more from ivy-occur, any call of ivy-read with :action has to be in the tail of a command. If we do anything else in the tail, it will be done only for the first candidate, not for all others.

Also, the :action is the ultimate side-effect, I think it doesn't make sense to rely on the return value of ivy-read if you use :action. Or even continue the current command.

@ericdanan

This comment has been minimized.

Contributor

ericdanan commented Aug 12, 2017

OK, thanks for the explanation.

@ericdanan

This comment has been minimized.

Contributor

ericdanan commented Sep 4, 2017

Sorry to bother you again, but I think the fix you applied in ivy-call should also be applied in ivy-occur-press. That is, there as well (funcall action ...) should become(with-current-buffer (ivy-state-buffer ivy-last) (funcall action ...)). Otherwise the action is called from the ivy window's displayed buffer but not necessarily the buffer from which ivy-read was called.

And likewise in ivy--format, I think (setq cands (mapcar transformer-fn cands)) should become (with-current-buffer (ivy-state-buffer ivy-last) (setq cands (mapcar transformer-fn cands))).

But perhaps it makes sense to leave things as they are...

@abo-abo

This comment has been minimized.

Owner

abo-abo commented Sep 4, 2017

Please PR if you can, I have too many things on my mind to figure this out.

@ericdanan

This comment has been minimized.

Contributor

ericdanan commented Sep 4, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment