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

counsel.el: Improve shell buffer switching #1570

Merged
merged 2 commits into from May 14, 2018

Conversation

@basil-conto
Copy link
Collaborator

commented May 14, 2018

Cc: @mookid

Changelog

(counsel-list-buffers-with-mode):

  • Rename to counsel--buffers-with-mode.

(counsel-switch-to-buffer-or-window):

  • Fix docstring.
  • Rename to counsel--switch-to-shell.
  • Support multiple frames and avoid switch-to-buffer pitfalls by using pop-to-buffer.

(counsel-switch-to-shell-buffer):

  • Use their new names.

Discussion

The problem with counsel-list-buffers-with-mode is that list-* functions such as list-buffers are meant as interactive commands in Emacs. The problem with counsel-switch-to-buffer-or-window is that it doesn't actually do that; it pops to a shell buffer, possibly in another window, creating the shell buffer as required. I'm only keeping the "switch" verb for consistency with counsel-switch-to-shell-buffer.

  1. Do you agree with the function renames?
  2. If so, are obsolete function aliases required?
display-buffer-same-window)
(inhibit-same-window . nil)
(reusable-frames . visible)))
(shell name)))

This comment has been minimized.

Copy link
@mookid

mookid May 14, 2018

Contributor

I think that it should not be hardcoded here; it should either be a customization variable (meh) or left as is (and customization left to the user as en entry in display-buffer-alist).

This comment has been minimized.

Copy link
@basil-conto

basil-conto May 14, 2018

Author Collaborator

The ACTION argument to display-buffer and pop-to-buffer has lower priority than both display-buffer-overriding-action and display-buffer-alist, so this is a sensible default, rather than hard-coded behaviour. What is hard-coded is the shell function and the shell-mode buffer filter. I, for example, always use ansi-term instead of shell, so I want to filter buffers by term-mode.

This comment has been minimized.

Copy link
@mookid

mookid May 14, 2018

Contributor

What is hard-coded is the shell function and the shell-mode buffer filter.

this customization direction can be the object of another PR.

so this is a sensible default, rather than hard-coded behavior

it emacs' default that bad?

This comment has been minimized.

Copy link
@basil-conto

basil-conto May 14, 2018

Author Collaborator

it emacs' default that bad?

What does "it" refer to? When I say that this is a sensible default, I'm talking about the behaviour of counsel-switch-to-buffer-or-window (now counsel--switch-to-shell) to reuse existing windows displaying the desired shell buffer. display-buffer and pop-to-buffer, without an ACTION argument, don't try to reuse existing windows. display-buffer is already capable of flexibly reusing existing windows, so this PR lets display-buffer do it for us instead of manually calling walk-windows and potentially introducing bugs. In other words, the ACTION argument is a declarative way of doing the same thing that counsel-switch-to-buffer-or-window used to do.

This comment has been minimized.

Copy link
@basil-conto

basil-conto May 14, 2018

Author Collaborator

this customization direction can be the object of another PR

Yes, that's why I didn't hold off this PR for that.

This comment has been minimized.

Copy link
@mookid

mookid May 14, 2018

Contributor

What does "it" refer to?

it was supposed to be 'is' :-)

When I say that this is a sensible default ...

ok, understood.

@mookid

This comment has been minimized.

Copy link
Contributor

commented May 14, 2018

Do you agree with the function renames?

i think that you do a better job as I do for that.

If so, are obsolete function aliases required?

I don't think so; there is maybe no user of these (apart from me) so I guess an entry the changelog is enough. I think it's a good occasion not to accumulate dead code.

However, why the code changes apart from renaming?
ok, I understand. if there is no obvious regression, go ahead.

@basil-conto

This comment has been minimized.

Copy link
Collaborator Author

commented May 14, 2018

However, why the code changes apart from renaming?

  1. There are very few cases where switch-to-buffer DTRT; see:
  2. Passing a nil as the ALL-FRAMES argument to walk-windows needlessly limits window lookup to the selected frame. This is a problem for users who set pop-up-frames to t because, for example, they use a tiling window manager.
  3. The whole walk-windows algorithm is duplicating built-in functionality.
@basil-conto

This comment has been minimized.

Copy link
Collaborator Author

commented May 14, 2018

Do you agree with the function renames?

I forgot to ask - is there a convention for the names of Ivy action functions? E.g. should they always be "public", without double hyphens, or can/should they be "internal", with double hyphens? Or is it completely down to a case-by-case basis?

@mookid
mookid approved these changes May 14, 2018
@mookid

This comment has been minimized.

Copy link
Contributor

commented May 14, 2018

I forgot to ask - is there a convention for the names of Ivy action functions? E.g. should they always be "public", without double hyphens, or can/should they be "internal", with double hyphens? Or is it completely down to a case-by-case basis?

I don't really know, but i'd be surprised if any user started using non-commands functions (apart from ivy-read, for instance) in their code.

Especially all that sits in counsel.el, which is 'application' code.

@abo-abo

This comment has been minimized.

Copy link
Owner

commented May 14, 2018

@basil-conto Thanks. But please resolve the merge conflict:)

basil-conto added 2 commits May 14, 2018
counsel.el: Improve shell buffer switching
(counsel-list-buffers-with-mode):
Rename to counsel--buffers-with-mode.
(counsel-switch-to-buffer-or-window): Fix docstring.
Rename to counsel--switch-to-shell.  Support multiple frames and
avoid switch-to-buffer pitfalls by using pop-to-buffer.
(counsel-switch-to-shell-buffer): Use their new names.
counsel.el: Revert recent obsolete function alias
(counsel-ibuffer-visit-ibuffer):
counsel-ibuffer-visit-vanilla-ibuffer was deemed internal enough not
to warrant an obsolete function alias.

Re: #1569

@basil-conto basil-conto force-pushed the basil-conto:blc/switch-shell branch to be6db35 May 14, 2018

@basil-conto

This comment has been minimized.

Copy link
Collaborator Author

commented May 14, 2018

@abo-abo

But please resolve the merge conflict:)

Done. I also added a commit removing the overkill function alias that you mentioned here: #1569 (comment).

@abo-abo abo-abo merged commit be6db35 into abo-abo:master May 14, 2018

1 of 2 checks passed

continuous-integration/travis-ci/push The Travis CI build is in progress
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@abo-abo

This comment has been minimized.

Copy link
Owner

commented May 14, 2018

Thanks.

@basil-conto basil-conto deleted the basil-conto:blc/switch-shell branch May 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.