oantolin / embark

Emacs Mini-Buffer Actions Rooted in Keymaps
GNU General Public License v3.0
948 stars 56 forks source link

Proliferation of completions' buffers #114

Closed protesilaos closed 3 years ago

protesilaos commented 3 years ago

Hello Omar! I have a reproducible recipe for unintentionally expanding the number of Embark's completion buffers.

  1. On emacs -Q evaluate those:
(load-file "/path/to/embark.el")
(require 'embark)
(add-hook 'minibuffer-setup-hook #'embark-collect-completions-after-delay)
  1. C-x b and then switch to the completions' buffer.
  2. Instead of selecting a candidate, do C-].
  3. Repeat steps 2 and 3 a few times.
  4. C-x b again and notice that there are multiple completions' buffers to choose from.

What I have for the time being is the following, though I assume you will provide a better way of handling this (bad for recursive minibuffers):

(defun prot-embark-clear-live-buffers ()
  "Remove lingering Embark Collect Completions' buffers.
Add this to `minibuffer-exit-hook'."
  (let* ((buffers (buffer-list))
         (case-fold-search nil)
         (completions
          (cl-remove-if-not (lambda (buf)
                              (string-match "\\*Embark.*Completions.*"
                                            (format "%s" buf)))
                            buffers)))
    (mapc #'kill-buffer completions)))

EDIT: A simpler alternative:

(defun prot-embark--live-buffer-p ()
  "Determine presence of a linked live occur buffer."
  (let ((buf-link embark-collect-linked-buffer))
    (when buf-link
      (window-live-p (get-buffer-window buf-link)))))

(defun prot-embark-keyboard-quit ()
  "Control the exit behaviour for Embark collect buffers.

If in a live Embark collect/completions buffer, run
`abort-recursive-edit'.  Otherwise run `keyboard-quit'.

This is meant to be bound in `embark-collect-mode-map'."
  (interactive)
  (if (and (derived-mode-p 'embark-collect-mode)
           (prot-embark--live-buffer-p))
      (progn
        (kill-buffer)
        (abort-recursive-edit))
    (keyboard-quit)))
oantolin commented 3 years ago

I kill the collect completions buffer in the minibuffer-exit-hook, and abort-recursive-edit called from outside the minibuffer closes the minibuffer without running the exit hook (if abort-recursive-edit is called from the minibuffer, then the exit hook is run).

oantolin commented 3 years ago

Maybe the best thing would be for abort-recursive-edit to run the exit hook of the active minibuffer, so that that exit hook can reliably be used for cleanup operations:

(defun run-minibuffer-exit-hook ()
  (when-let ((miniwin (active-minibuffer-window)))
    (with-selected-window miniwin
      (run-hooks 'minibuffer-exit-hook))))

(advice-add 'abort-recursive-edit :before #'run-minibuffer-exit-hook)
oantolin commented 3 years ago

I don't think the code in your simpler alternative works as is:

prot-embark--live-buffer-p checks if there is a embark collect buffer that was created from the current buffer that is being displayed in window. So if called from the minibuffer during a collect completions session it would return t, but if called from the collect completions buffer it will typically return nil (unless you also ran one of the embark collect commands from that buffer!).

This means that (and (derived-mode-p 'embark-collect-mode) (prot-embark--live-buffer-p)) is unlikely to return t; it only would if called from an embark collect buffer from which you also ran an embark collect command (and this second embark collect buffer where being displayed in a window).

The condition you want, I'm guessing, instead of that one, is:

(and (derived-mode-p 'embark-collect-mode)   ; are we in an embark collect buffer...
     (eq embark-collect--kind :completions)) ; that should be killed when the minibuffer exits?

If that holds, then kill the buffer before running abort-recursive-edit.

protesilaos commented 3 years ago

abort-recursive-edit called from outside the minibuffer closes the minibuffer without running the exit hook (if abort-recursive-edit is called from the minibuffer, then the exit hook is run).

I see. In that case, advising seems to be the right way forward.

This means that (and (derived-mode-p 'embark-collect-mode) (prot-embark--live-buffer-p)) is unlikely to return t

You are right. I noticed it after posting this. My thought was that linked buffer could be known from elsewhere.

The condition you want, I'm guessing, instead of that one, is: [...]

Thank you!

oantolin commented 3 years ago

So, did you discover this because you use C-] to close the minibuffer from the collect completions buffer? I had been using q C-g for that, but I single key would be better. By the way, C-M-c works well, without advice or rebinding it.

protesilaos commented 3 years ago

So, did you discover this because you use C-] to close the minibuffer from the collect completions buffer?

Sometimes I use C-] from a regular buffer while the minibuffer and the completions are open. Though I am not sure how I noticed this today.

I had been using q C-g for that

I have been trying to do that in a single C-g. My intent is to treat the minibuffer+completions as if it were a unit. Then regardless of whether we hit C-g from inside the completions or the minibuffer it should always have the same meaning. q is still useful in its own right though.

oantolin commented 3 years ago

I have been trying to do that in a single C-g.

You can bind C-g to exit-recursive-edit (exit, not abort!) in embark-collect-mode-map, without writing any custom code.

protesilaos commented 3 years ago

You can bind C-g to exit-recursive-edit (exit, not abort!) in embark-collect-mode-map, without writing any custom code.

Yes, better. Thank you!

oantolin commented 3 years ago

Oh, maybe exit-recursive-edit is not what you want: it accepts the current minibuffer input! Sorry, I didn't test carefully.

protesilaos commented 3 years ago

Oh, maybe exit-recursive-edit is not what you want: it accepts the current minibuffer input! Sorry, I didn't test carefully.

Ah, another gotcha! No worries though, adding a tiny snippet is not an issue anyhow.

oantolin commented 3 years ago

The new embark-quit is a good option for C-g in the embark collect buffers. I'm a little reluctant to bind it by default, but I think you'd like it.

protesilaos commented 3 years ago

Thank you!

jaor commented 3 years ago

Thanks!

oantolin commented 3 years ago

@protesilaos Unfortunately my code for embark-quit was mistakenly running the minibuffer-exit-hook twice in some circumstances. @jakanakaevangeli was kind enough to fix that and other bugs in PR #125 (see also the discussion in #123), but as a result running embark-quit from the collect completions buffer doesn't not run the minibuffer-exit-hook anymore.

For C-g in the embark collect buffers you could use the following:

(defun embark-quit-from-collect ()
  (interactive)
  (when (minibufferp embark-collect-from)
    (with-current-buffer embark-collect-from
      (run-hooks 'minibuffer-exit-hook)
      (abort-recursive-edit))))
protesilaos commented 3 years ago

Thank you! I will review my tweaks later today.

jakanakaevangeli commented 3 years ago

Omar Antolín Camarena notifications@github.com writes:

running embark-quit from the collect completions buffer doesn't run the minibuffer-exit-hook anymore.

The hook that does end up running in this case is minibuffer-exit-hook as seen locally from the collect buffer. I reported this as an emacs bug in bug#46097.