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

Add a simple initial counsel-switch-buffer #1895

Closed
wants to merge 1 commit into from

Conversation

@errge
Copy link
Contributor

@errge errge commented Jan 17, 2019

This first implementation already provides the interactive element of
functionality: in the current window the user can preview her buffer
selection.

Copy link

@Alexander-Shukaev Alexander-Shukaev left a comment

A few points to tidy, but looks and works great. I like the tool, the question remains though whether this should be part of ivy itself instead.

counsel.el Outdated Show resolved Hide resolved
counsel.el Outdated Show resolved Hide resolved
counsel.el Show resolved Hide resolved
@errge
Copy link
Contributor Author

@errge errge commented Jan 18, 2019

A few points to tidy, but looks and works great. I like the tool, the question remains though whether this should be part of ivy itself instead.

@Alexander-Shukaev

I was thinking about that too, but I always felt this way: ivy is a general framework for a better completing-read. Counsel has all the task specific commands (how to awesomely browse files, functions, variables and now buffers). If later we have more and more ideas to make this initial implementation better, than that would fit right into counsel, but would feel awkward in ivy.

This is why I have put the new feature into counsel.

Another reason, which I think is quite important: there are already many users and this is a change, which can be upsetting for people. E.g. if you don't like huge part of the screen changing when you are not focusing there with your eye. Or you work on a slow X connection or terminal. If we put this into counsel, then ivy-switch-buffer doesn't have to change, we need no confusing customization values with debatable defaults and the like.

I would be happy to hear your opinions too, if you don't mind!

This first implementation already provides the interactive element of
functionality: in the current window the user can preview her buffer
selection.
Copy link

@Alexander-Shukaev Alexander-Shukaev left a comment

Agree with your points. Let it stay in counsel. I've noticed one more thing though, I see that ivy-switch-buffer also has :keymap ivy-switch-buffer-map, shouldn't we add it here as well?

@errge
Copy link
Contributor Author

@errge errge commented Jan 18, 2019

@Alexander-Shukaev I tested that for a while and then decided to remove it.

There are a lot of corner cases to work out with the kill functionality of that keymap. And if I saw it correctly, there is no other keys in it, only the killing.

I would prefer to handle all that strangeness and bug hunting in a second PR if we decide to.

Would it make sense for documentation purposes to do the following: have a company-switch-buffer-map, which is a copy-keymap of the ivy one, but then undefining the killing in that with the comment that it's too buggy with the preview. Then whenever in the future new keys are added to ivy, we will have those and also people will understand that it's a good project to work on the kinks of the killing functionality.

@Alexander-Shukaev
Copy link

@Alexander-Shukaev Alexander-Shukaev commented Jan 18, 2019

I would then stub an implementation of counsel-switch-buffer-kill which is currently doing nothing and put a TODO in it. Then I'd do what you said, copy-keymap of ivy-switch-buffer-map and [remap ivy-switch-buffer-kill] to counsel-switch-buffer-kill.

@errge
Copy link
Contributor Author

@errge errge commented Jan 18, 2019

Does it worth the hassle though? I'm unsure.

Let's see what @abo-abo has to say about the kill functionality and its future.

@Alexander-Shukaev
Copy link

@Alexander-Shukaev Alexander-Shukaev commented Jan 18, 2019

Yup, I mean it's also important to understand what exactly was wrong with ivy-switch-buffer-kill in this case as it could suffice to do a couple of fixes there directly.

@errge
Copy link
Contributor Author

@errge errge commented Jan 18, 2019

Yeah, I agree with all you said, but i would like to call all this out of scope :)

Copy link

@Alexander-Shukaev Alexander-Shukaev left a comment

Green light from me. Looking forward to see the kill functionality as discussed above. Being able to kill buffers based on a quick preview rather than only by looking at their name (as in ivy-switch-buffer) is a "killer" feature :).

@mookid
Copy link
Contributor

@mookid mookid commented Jan 18, 2019

What about support for ivy-use-virtual-buffers?

@errge
Copy link
Contributor Author

@errge errge commented Jan 18, 2019

@mookid The PR handles them in the sense that they don't confuse the PR. So they are ignored and won't cause problems. Proper support I think would need further work and some decisions, for which I would like to know @abo-abo's opinion. E.g. do we kill the files that we have opened just for preview or do we only bury them? Do we really open every file which is randomly focused during typing or do we have a delay? What is the default value for the delay?

So I'm happy to work on these, but would like to have small PRs one-by-one, if that's OK with the community.

If you do some initial work towards this on top of my PR, I would be very-very happy to take a look and give it a test-drive!

@mookid
Copy link
Contributor

@mookid mookid commented Jan 18, 2019

What visual studio does in that kind of scenario is quite acceptable: open the buffer for preview, and kill it if it is not selected.
For performance, one could imagine the buffers opened for preview and not selected being all killed at the end of the ivy-read call (to avoid paying the opening overhead several times for the same buffer in a single session).

If you do some initial work towards this on top of my PR, I would be very-very happy to take a look and give it a test-drive!

It may be possible. If i have something concrete to show, I'll let you know :)

in the current window."
(interactive)
(ivy-read "Switch to buffer: " 'internal-complete-buffer
:preselect (buffer-name (other-buffer (current-buffer)))
Copy link
Contributor

@mookid mookid Jan 20, 2019

Suggested change
:preselect (buffer-name (other-buffer (current-buffer)))
:keymap ivy-switch-buffer-map
:preselect (buffer-name (other-buffer (current-buffer)))

@mookid
Copy link
Contributor

@mookid mookid commented Jan 20, 2019

If you do some initial work towards this on top of my PR, I would be very-very happy to take a look and give it a test-drive!

It may be possible. If i have something concrete to show, I'll let you know :)

see #1897

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Jan 21, 2019

Thanks. I like the functionality.

Do you have an Emacs Copyright Assignment?

I notice quite a bit of code repetition between ivy-switch-buffer and counsel-switch-buffer. Maybe we could introduce ivy-update-fn-alist to make every ivy-read customizable.

Here's how it could work:

(add-to-list 'ivy-update-fn-alist
             '((ivy-switch-buffer . counsel--switch-buffer-update-fn)))

And it would be useful elsewhere. For instance, by default counsel-ag doesn't jump to the current candidate unless RET is used.

We could do:

(add-to-list 'ivy-update-fn-alist
             '((counsel-ag . (lambda ()
                               (with-ivy-window
                                 (counsel-git-grep-action (ivy-state-current ivy-last)))))))

@errge
Copy link
Contributor Author

@errge errge commented Jan 21, 2019

@abo-abo I will look into your technical comments and reply soon.

About the copyright assignment: I have it for ages, since 10 years or something. Do you need some kind of proof? Because then I have to look into it. What do you need, some kind of ticket number or a PDF?

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Jan 21, 2019

I have it for ages, since 10 years or something

Great. I can check it later in the copyright.list file on the FSF server. I don't have the SSH key on this machine to do the check.

@Alexander-Shukaev
Copy link

@Alexander-Shukaev Alexander-Shukaev commented Jan 24, 2019

I just thought why not have the same for counsel-*g and friends? Maybe we should come up with a general approach to enable this behavior for all/selected commands (for those for which it is appropriate at all of course).

@abo-abo abo-abo closed this in 9718962 Feb 12, 2019
@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Feb 12, 2019

@errge merged, thanks.

abo-abo added a commit that referenced this issue Oct 11, 2019
* ivy.el (ivy-unwind-fns-alist): New defvar.

Reduce code duplication by having `counsel-switch-buffer' become a
customized `ivy-switch-buffer'.

Re #1895
@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Oct 11, 2019

counsel-switch-buffer now re-uses ivy-switch-buffer. Please review and test.

The user can now configure ivy-switch-buffer to behave like counsel-switch-buffer in their config. But maybe we should keep both. Are you using both commands in your workflow?

astoff added a commit to astoff/swiper that referenced this issue Jan 1, 2021
This first implementation already provides the interactive element of
functionality: in the current window the user can preview her buffer
selection.

Fixes abo-abo#1895
astoff added a commit to astoff/swiper that referenced this issue Jan 1, 2021
* ivy.el (ivy-unwind-fns-alist): New defvar.

Reduce code duplication by having `counsel-switch-buffer' become a
customized `ivy-switch-buffer'.

Re abo-abo#1895
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants