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

Never auto-switch to helm menu buffers #6159

Closed
Stebalien opened this issue May 29, 2016 · 32 comments
Closed

Never auto-switch to helm menu buffers #6159

Stebalien opened this issue May 29, 2016 · 32 comments

Comments

@Stebalien
Copy link
Contributor

Description :octocat:

If there's only one normal buffer left, spacemacs/alternate-buffer will switch to a random helm menu (if it exists). After deleting all buffers, spacemacs will switch to an old helm menu.

Preferably, these would never be displayed. I don't know how fixable this is but it's kind of an annoying papercut.

Reproduction guide 🪲

  • Start Emacs
  • Hit SPC SPC then ESC to open and close a helm menu.
  • repeatedly hit SPC bd to close all open buffers.
  • Eventually, you'll switch to some helm menu.

Observed behaviour: 👀

I switch to old helm menus.

Expected behaviour: 💔

When deleting the last normal buffer, I expect to see a new *scratch* buffer. When calling spacemacs/alternate-buffer on the last buffer, I expect nothing to happen.

System Info 💻

  • OS: gnu/linux
  • Emacs: 24.5.1
  • Spacemacs: 0.105.20
  • Spacemacs branch: develop (rev. 1623ff6)
  • Graphic display: t
  • Distribution: spacemacs
  • Editing style: vim
  • Completion: helm
  • Layers:
(helm better-defaults git
      (auto-completion :variables auto-completion-return-key-behavior nil)
      org spell-checking syntax-checking
      (shell :variables shell-default-shell 'ansi-term)
      emoji emacs-lisp markdown latex bibtex csv yaml c-c   html haskell go javascript python systemd
      (rust :variables rust-format-on-save nil))
@sooheon
Copy link

sooheon commented Jun 5, 2016

I'm only half kidding when I say the easiest way to do this is to switch to ivy.

@Stebalien
Copy link
Contributor Author

I just got used to helm (coming from vim) so I'd rather not switch again...

@syl20bnr
Copy link
Owner

syl20bnr commented Jun 6, 2016

I believe I encountered this behaviour once in a layout and I was a bit confused since the helm buffer takes the whole screen.

@deb0ch
Copy link
Contributor

deb0ch commented Jun 6, 2016

Even normally switching buffers with M-x I keep encountering helm buffers that should not even be still opened in the first place (I can even edit them, it really is just text).

Is that what you are talking about ?

I do not know if it is supposed to be like that, is it ? Or should it be fixed ?

screenshot from 2016-06-06 23-06-43

@Stebalien
Copy link
Contributor Author

Yes. That's exactly what I'm talking about. As @sooheon pointed out, this doesn't happen with ivy.

@StreakyCobra
Copy link
Contributor

CC @bmag, our windows/buffers specialist 😃

@bmag
Copy link
Collaborator

bmag commented Jun 7, 2016

Well, normally Helm buffers aren't killed after the Helm session ends. The problem here is that spacemacs/alternate-buffer doesn't ignore "uninteresting" buffers:

(defun spacemacs/alternate-buffer ()
  "Switch back and forth between current and last buffer in the
current window."
  (interactive)
  (if (evil-alternate-buffer)
      (switch-to-buffer (car (evil-alternate-buffer)))
    (switch-to-buffer (other-buffer (current-buffer) t))))

We can replace evil-alternate-buffer with another function that scans window-prev-buffers but also filters out uninteresting buffers. We can replace other-buffer with a function that scans buffer-list or persp-buffer-list (when layouts are used) and filters out uninteresting buffers.
I think there is some sort of a list of uninteresting buffers somewhere, but I couldn't find it.

@deb0ch
Copy link
Contributor

deb0ch commented Jun 7, 2016

Something more or less like this ?

(do not trust this code too much)

@nixmaniack
Copy link
Contributor

Spacemacs did define spacemacs-useless-buffers-regexp but it looks like it's not being used at much places.

@deb0ch
Copy link
Contributor

deb0ch commented Jun 7, 2016

And btw, why is Helm not killing these annoying buffers after use in the first place ?

Wouldn't it be better if we made Helm kill these buffers, either by adding some hook somewhere or directly from the upstream code ?

@TheBB
Copy link
Collaborator

TheBB commented Jun 7, 2016

You'll break SPC r l if you kill the helm buffers.

@deb0ch
Copy link
Contributor

deb0ch commented Jun 7, 2016

SPC r l ? Cannot find this one in the docs or in spacemacs

@TheBB
Copy link
Collaborator

TheBB commented Jun 7, 2016

Or whatever helm-resume is bound to on master.

@bmag
Copy link
Collaborator

bmag commented Jun 7, 2016

@nixmaniack thanks for pointing that out, it led me to find spacemacs/useless-buffer-p. I'm thinking we can replace evil-alternate-buffer with something like:

(cl-loop for buffer in (window-prev-buffers)
         ;; with is evaluated once in the beginning of the loop
         with displayed-buffer = (window-buffer)
         ;; return first non-useless buffer that isn't the window's current buffer
         unless (or (eq buffer displayed-buffer)
                    (spacemacs/useless-buffer-p buffer))
         return buffer)

When deleting the last normal buffer, I expect to see a new scratch buffer.

I missed this part earlier. The function that replaces a killed buffer in all relevant windows is called replace-buffer-in-windows, which then uses switch-to-prev-buffer to replace each window's buffer.

Another approach we could use is to set the frame parameter buffer-predicate, which looks like it was meant for exactly our use-case. The only problem is that persp-mode already uses buffer-predicate for its own purposes, and sets it according to persp-set-frame-buffer-predicate. However, maybe we can change persp-set-frame-buffer-predicate so it sets a buffer-predicate that works both for persp-mode and for ignoring useless buffers.

@syl20bnr
Copy link
Owner

syl20bnr commented Jun 7, 2016

As a side note, persp-mode also make it hard to have SPC tab to work properly with layout isolation, it reverts to nil a variable called window-buffers or something like this.

@deb0ch
Copy link
Contributor

deb0ch commented Jun 10, 2016

And so is there a clean way of burying these buffers somewhere, so that we don't have them in the way when cycling around ?

I am not familiar with workspaces and layouts, is there a way of isolating all of the useless buffers (Helm, messages, etc...) in a separate layout / workspace in a way that we would not encounter them when using C-x arrow ?

In addition, Helm generates quite a lot of these and does not seem to ever clean them.

Are they really all needed for helm-resume ?

@bmag
Copy link
Collaborator

bmag commented Jun 10, 2016

And so is there a clean way of burying these buffers somewhere, so that we don't have them in the way when cycling around ?

Yes, the clean way is to use buffer-predicate. However, persp-mode (the package that provides layouts) already makes use of buffer-predicate (I'm not sure what for), so any solution should avoid breaking persp-mode. This also takes care of of cycling via C-x arrow.

In addition, Helm generates quite a lot of these and does not seem to ever clean them. Are they really all needed for helm-resume?

I don't know, but I won't be surprised if they are needed. Besides, sometimes it's useful to manually switch to helm buffers. For example, viewing the results of SPC ? outside of the Helm session to get an overview of all defined key bindings.

@bmag
Copy link
Collaborator

bmag commented Jun 10, 2016

@deb0ch @Stebalien I think adding the following to dotspacemacs/user-init is supposed to fix the issue, tough it may not work because of Bad-ptr/persp-mode.el#41:

(defun spacemacs/useful-buffer-p (buffer)
  (not (spacemacs/useless-buffer-p buffer)))

(let ((buf-pred-entry (assq 'buffer-predicate default-frame-alist)))
  (if buf-pred-entry
      ;; `buffer-predicate' entry exists, modify it
      (setcdr buf-pred-entry #'spacemacs/useful-buffer-p)
    ;; `buffer-predicate' entry doesn't exist, create it
    (push '(buffer-predicate . spacemacs/useful-buffer-p) default-frame-alist)))

Edit: this seems to work as long as you only use one frame and don't create other frames

@Stebalien
Copy link
Contributor Author

@bmag Thanks. Unfortunately, I use emacs in daemon mode and almost always have multiple frames open.

@deb0ch
Copy link
Contributor

deb0ch commented Jun 11, 2016

In that case, overriding previous-buffer, next-buffer and kill-this-buffer would be a temporary solution right ?

It is the solution I am currently using (in my previous link) and it works quite well.

When I want to purposely access a "useless" buffer I just use C-x b.

@bmag
Copy link
Collaborator

bmag commented Jun 11, 2016

In that case, overriding previous-buffer, next-buffer and kill-this-buffer would be a temporary solution right ?

That depends on the problem you are trying to solve. next-buffer and previous-buffer are "high-level" commands that are bound to C-x <right> and C-x <left> - rebinding them to the functions in your link is a valid solution. I should note that spacemacs provides spacemacs/next-useful-buffer and spacemacs/previous-useful-buffer (they work in the same way as your functions).

The original issue is about kill-this-buffer (SPC b d) and spacemacs/alternate-buffer (SPC TAB). You can indeed replace SPC b d with a command that calls spacemacs/next-useful-buffer after kill-this-buffer, similar to kill-this-buffer-avoid-boring from your link. To work-around SPC TAB you'll need to rewrite spacemacs/alternate-buffer, because it uses more "low-level" methods than next-buffer and previous-buffer.

However, I want to make it clear to everyone following this thread that once the issue with persp-mode is fixed, then using buffer-predicate should automagically fix C-x arrow, SPC b d and SPC TAB, as well as make spacemacs/next-useful-buffer and spacemacs/previous-useful-buffer unnecessary.

@bmag
Copy link
Collaborator

bmag commented Jun 13, 2016

The bug in persp-mode was fixed, but it turns out I was wrong when I said that using buffer-predicate will automagically fix SPC TAB and SPC b d. I have added to dotspacemacs/user-init what I suggested in a previous comment. C-x arrow commands do work as expected now (and better than spacemacs/next-useful-buffer).

spacemacs/alternate-buffer (SPC TAB) uses evil-alternate-buffer, which in turn uses window-prev-buffers, which doesn't respect buffer-predicate. Either this is a bug in evil-alternate-buffer that should be reported upstream to Evil repository, or we need to write our own function (it's not hard).

When I press SPC b d on the last useful buffer, after I already killed all the other useful buffers, the last useful buffer is killed and a "useless" buffer (the home buffer) is displayed instead. Interestingly, a new scratch buffer is actually created but not displayed. I don't why it is so, but the relevant functions to research are kill-buffer and replace-buffer-in-windows. However, if the last useful buffer is the scratch buffer, then SPC b d doesn't kill the buffer (WTF?).

I'd appreciate it if someone else would step up to solve this issue, as I will probably be too busy to work on it in the next few weeks. Otherwise, I'll get back to the issue when I'm less busy.

@bmag
Copy link
Collaborator

bmag commented Jun 24, 2016

Reported possible bug in evil-alternate-buffer at Evil's repository: https://bitbucket.org/lyro/evil/issues/680/

@bmag
Copy link
Collaborator

bmag commented Jul 14, 2016

I have addressed this bug in #6574:

SPC TAB switching to random helm buffer: This is fixed. spacemacs/alternate-buffer respects buffer-predicate and will only switch to a useful buffer from the current layout. When called on the last buffer of a layout, it switches to the scratch buffer. There are two caveats, mentioned in #6574 (comment):

SPC b d switching to random helm buffer: Partially fixed. When killing a buffer, it will be replaced by a useful buffer from the current layout. If there's no such buffer (e.g. last buffer in a layout), the killed buffer is replaced by a useful buffer from another layout. A scratch buffer is created in this case, but I couldn't get Emacs to switch to it. To make it show the scratch buffer we will need to modify kill-buffer or replace-buffer-in-windows, which I think is something we don't want to do.

@deb0ch
Copy link
Contributor

deb0ch commented Jul 14, 2016

Why wouldnt we want to do that ?

@TheBB
Copy link
Collaborator

TheBB commented Jul 14, 2016

It's probably not a good idea to redefine core Emacs functions that are relied upon by many packages to work a specific way.

@bmag
Copy link
Collaborator

bmag commented Jul 14, 2016

What @TheBB wrote, plus Emacs creates and kills a ton of temporary buffers, so kill-buffer and replace-buffer-in-windows are called a ton of times, so we don't want to make them slower unless necessary. You can write a simple advice to echo a message each time kill-buffer is called and see for yourself that it's called a lot.

@deb0ch
Copy link
Contributor

deb0ch commented Jul 14, 2016

Oh I see, in these conditions I agree it would be better not messing it up.

But I find that showing the scratch buffer when you killed the last buffer of your layout is pretty important. It is very disconcerting to see a buffer from another layout appear, then you don't know if that buffer belonged to that layout in the first place.
Is there no other solution ?
Or would it be possible to put the modifications to these functions inside an if statement, so that it would not mess up the rest ?

@bmag
Copy link
Collaborator

bmag commented Jul 14, 2016

We could force each layout to have a scratch buffer, and make the buffer "unkillable" (or just generate a new scratch buffer immediately after the scratch buffer is killed). This way only the scratch buffer can be a "last buffer", and kill-buffer doesn't kill the scratch buffer when it's the last buffer.

I think I've encountered the term "unkillable scratch buffer" somewhere.
Edit: https://github.com/EricCrosson/unkillable-scratch

@TheBB
Copy link
Collaborator

TheBB commented Jul 25, 2016

#6574 has been merged.

@Stebalien
Copy link
Contributor Author

NICE! I'm happy with this fix if you want to close this issue (I don't know if you want to keep it open for tracking reasons...).

@d12frosted
Copy link
Collaborator

Fixed with released of Spacemacs v0.200. Let us know if you still have any problems with this issue.

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

No branches or pull requests

9 participants