Issues with displaying previews

[mpedramfar]

I'll explain the issues with an example.

Create a new folder, create a file named test.el in it with the following content and run eval-buffer:

(vertico-buffer-mode -1)

(dolist (f '("left-side.txt"
             "right-side.txt"
             "top-side.txt"
             "bottom-side.txt"
             "right-direction.txt"
             "left-direction.txt"))
  (when (file-exists-p f)
    (kill-buffer f)
    (delete-file f))
  (with-temp-buffer
    (insert (format "This is the test file:\n%s" f))
    (write-file f))
  (find-file-noselect f))

(setq vertico-buffer-display-action
      '(display-buffer-in-side-window
        (window-height . 0.15)
        (side . bottom)))

(setq display-buffer-alist nil)
  
(add-to-list 'display-buffer-alist
             '("left-side.txt"
               display-buffer-in-side-window
               (side . left)
               (window-width . 0.15)))

(add-to-list 'display-buffer-alist
             '("right-side.txt"
               display-buffer-in-side-window
               (side . right)
               (window-width . 0.15)))

(add-to-list 'display-buffer-alist
             '("top-side.txt"
               display-buffer-in-side-window
               (side . top)
               (window-height . 0.15)))

(add-to-list 'display-buffer-alist
             '("bottom-side.txt"
               display-buffer-in-side-window
               (side . bottom)
               (window-height . 0.15)))

(add-to-list 'display-buffer-alist
             '("right-direction.txt"
               display-buffer-in-direction
               (direction . right)
               (window-width . 0.25)))

(add-to-list 'display-buffer-alist
             '("left-direction.txt"
               display-buffer-in-direction
               (direction . left)
               (window-width . 0.25)))

Issue 1:
Run consult-buffer. As you go over different buffers, the previous previews are not closed, instead they pile up:

Possible solution:
Some kind of save-window-excursion that would make consult show at most a single preview at a time.

Issue 2:
Activate vertico-bufffer-mode, then run consult-buffer. Not only preview buffers pile up, but the buffer bottom-side.txt hides the vertico buffer:

Note that the vertico buffer is active and current. We can move up and down and select buffers.

Possible solution:
This specific example can be be solved by modifying consult-preview-excluded-files, but this is not a general solution.
For example, it could be vterm instead of bottom-side.txt. A better solution would be to have a variable consult-preview-exclude which is a list, where each item in the list is like a condition of display-buffer-alist, i.e. each item can be a condition that can be passed to buffer-match-p.

[minad]

Hi @mpedramfar! It is a known issue that fine-grained display-buffer-alists don't play well together with Consult preview. And as you found, vertico-buffer doesn't simplify the situation. I don't have a solution ready for this, but there were experiments at some point (#670, #671, which only worked on 28 and newer). I would be happy to consider patches improving the status quo. Adding buffer exclusion lists would be one solution, but not a really satisfactory one.

We better restore windows as you propose. This is not easily to do since almost anything can happen, so one would need to save the full window configuration every time. The problem gets even worse if one adds frames to the equations.

[mpedramfar]

I think it makes sense to ignore the vertico-buffer issue until the first one is solved. Then we can revisit the issue and see if it's worth the trouble. I agree that no one can stop an Emacs user if they want to shoot their foot. Having said that, side windows are a pretty common and useful thing and many packages recommend using them, (e.g. vterm-toggle or the vertico-buffer itself through the default cutomization options in vertico-buffer-display-action) so it would be good to have support for it if it's not too much hassle. Alternatively, if it turns out to be not worth the trouble, a general consult-preview-exclude would be good to have, so that people who play with display-buffer-alist have the option of sorting out the issue in their personal config.

[mpedramfar]

I just came across desktop-save-mode. Could something like this be used here? Instead of saving the window configuration (which seems to be the approach in #671), we save the whole frameset. This way even if a buffer uses display-buffer-pop-up-frame, we could be sure to restore the previous frames and windows.

[mpedramfar]

Actually, instead of desktop-save-mode, it might be better to use current-frame-configuration and set-frame-configuration.

I also looked a bit more into #671. I'm not sure I understand the issue. What are the scenarios when a buffer preview is shown and the minibuffer is growing? My understanding was that minibuffer grows if there is change in either the list of items or the input. Does this happen when a preview is being shown in this use-case?

For a more general solution, maybe we could advice window--resize-mini-window and vertico--resize-window so that whenever they are about to be called, we first go back to the previously saved window configuration (basically wc in #671) then this function is called and then the value of wc is set again, so that wc always has the latest information about the size of the minibuffer.

[minad]

Did you make some experiments with these different approaches? This would be helpful to decide on the best way forward.

[mpedramfar]

This seems to solve all the issues, though it probably needs more testing.
I've done some tests with the examples I've written above and also with display-buffer-pop-up-frame, with and without vertico-buffer-mode.

By the way, in the docstring of consult--with-preview, it says "The state function is always executed with the original window selected". This doesn't seem to be always true.

(defun my-set-frame-configuration (configuration)
  (or (frame-configuration-p configuration)
      (signal 'wrong-type-argument
	      (list 'frame-configuration-p configuration)))
  (let* ((config-alist (cdr configuration))
         (mini-win-alist
          (seq-filter #'identity
                      (mapcar (lambda (w)
                                (when (window-minibuffer-p w)
                                  (list w (window-point w)
                                        (window-pixel-width w)
                                        (window-pixel-height w))))
                              (window-list-1 nil t t))))
         (cur-mini-win (minibuffer-window))
         (cur-mini-buf (window-buffer cur-mini-win))
         (cur-mini-point (window-point cur-mini-win))
         (cur-mini-width (window-pixel-width cur-mini-win))
         (cur-mini-height (window-pixel-height cur-mini-win))
	 frames-to-delete delta)
    (dolist (frame (visible-frame-list))
      (let ((parameters (assq frame config-alist)))
        (if parameters
            (set-window-configuration (nth 2 parameters) 'no-frame)
          (setq frames-to-delete (cons frame frames-to-delete)))))
    ;; Restore all minibuffers in all frames to their original size
    ;; Relevant when there are multiple frames with different minibufffers.
    ;; (when `enable-recursive-minibuffers' is non-nil)
    (mapc (lambda (item)
            (let ((w (car item)))
              (when (window-live-p w)
                (unless (frame-root-window-p w)
                  (setq delta (- (nth 2 item) (window-pixel-width w)))
                  (when (/= delta 0)
                    (ignore-errors
                      (window-resize w delta t nil 'pixelwise)))
                  (setq delta (- (nth 3 item) (window-pixel-height w)))
                  (when (/= delta 0)
                    (ignore-errors
                      (window-resize w delta nil nil 'pixelwise))))
                (set-window-point w (nth 1 item)))))
          mini-win-alist)
    ;; Set the size of all windows showing the previously active minibuffer
    ;; to the size of the previous minibuffer window.
    ;; Relevant when `consult--buffer-action' creates a new frame and the
    ;; newly created minibuffer window should match the size of previous one.
    (mapc (lambda (w)
            (unless (frame-root-window-p w)
              (setq delta (- cur-mini-width (window-pixel-width w)))
              (when (/= delta 0)
                (ignore-errors
                  (window-resize w delta t nil 'pixelwise)))
              (setq delta (- cur-mini-height (window-pixel-height w)))
              (when (/= delta 0)
                (ignore-errors
                  (window-resize w delta nil nil 'pixelwise)))
              (set-window-point w cur-mini-point)))
          (get-buffer-window-list cur-mini-buf nil t))
    (mapc 'delete-frame frames-to-delete)))


(defun consult--buffer-preview ()
  "Buffer preview function."
  (let* ((sw (selected-window))
         fc)
    (lambda (action cand)
      (when (eq action 'preview)
        (when fc
          (my-set-frame-configuration fc)
          (setq fc nil))
        (when (and cand (get-buffer cand))
          (let* ((minibuffer-follows-selected-frame t)
                 (frame (selected-frame))
                 (buf (window-buffer (minibuffer-window)))
                 (vertico-buffer--window (car (delq (minibuffer-window)
                                                    (get-buffer-window-list buf)))))
            (setq fc (current-frame-configuration))
            (select-window sw 'norecord)
            (consult--buffer-action cand 'norecord)
            (select-window (minibuffer-window) 'norecord)
            (when (and vertico-buffer--window
                       (not (member vertico-buffer--window
                                    (get-buffer-window-list buf nil frame))))
              (my-set-frame-configuration fc)
              (select-window vertico-buffer--window 'norecord)
              (setq fc nil))))))))

[minad]

On 11/11/23 19:07, mpedramfar wrote: This seems to solve all the issues, though it probably needs more testing. I've done some tests with the examples I've written above and also with |display-buffer-pop-up-frame|, with and without |vertico-buffer-mode|.

Thanks. This is quite an increase in complexity. I am not sure if I want to go that way, if we need a custom set-frame-configuration function. Also there is some Vertico-specific code.

By the way, in the docstring of |consult--with-preview|, it says "The state function is always executed with the original window selected". This doesn't seem to be always true.

When does this happen in your tests? The state function is normally executed in the window selected before the minibuffer was selected. The only scenario where this does not happen is if the window has been closed in the meantime.

[minad]

We could also use `set-window-configuration` with DONT-SET-MINIWINDOW=t on Emacs 28 and newer as I had done in the earlier patch. On older Emacs versions, we simply retain the current solution.

[mpedramfar]

Complexity is not the only issue with the code I sent above! I'll message here when I have a properly working code.

When does this happen in your tests? The state function is normally executed in the window selected before the minibuffer was selected. The only scenario where this does not happen is if the window has been closed in the meantime.

I may have used select-window without the 'norecord flag in some of those test, in which case those tests were wrong.
There is also the issue of the selected frame, if we're in the wrong frame, then "the window selected before the minibuffer was selected" is always wrong, though I'm not sure if this issue actually happens.

Talking about the frame, it seems like vertico-buffer-mode doesn't respect minibuffer-follows-selected-frame, i.e. it acts as if it's nil, while the default value of this variable is actually t.
To see this, make a file named popup-frame.txt and run this:

(add-to-list 'display-buffer-alist
             '("popup-frame.txt" display-buffer-pop-up-frame))
(setq minibuffer-follows-selected-frame t)

Then try consult-buffer with or without vertico-buffer-mode

Actually, it's good that vertico-buffer-mode doesn't respect minibuffer-follows-selected-frame. It's using an arbitrary display function, trying to match its behavior with everything that a minibuffer does is probably not a good idea.

[mpedramfar]

Here is a better version:

(defun my-set-frame-configuration (configuration)
  (dolist (frame (visible-frame-list))
    (if-let ((parameters (assq frame (cdr configuration))))
        (set-window-configuration (nth 2 parameters) 'no-frame 'no-miniwindow)
      (delete-frame frame))))

(setq my-consult-exclude nil)

(defun consult--buffer-preview ()
  "Buffer preview function."
  (let (fc)
    (lambda (action cand)
      (when (eq action 'preview)
        (when fc
          (my-set-frame-configuration fc)
          (setq fc nil))
        (when (and cand (get-buffer cand)
                   (not (seq-some (lambda (x) (buffer-match-p x cand))
                                  my-consult-exclude)))
          (let* ((frame (selected-frame))
                 (buf (window-buffer (minibuffer-window)))
                 (semi-mini-windows (delq (minibuffer-window) ;; windows other than the minibuffer-window that are showing the minibuffer in the current frame
                                          (get-buffer-window-list buf))))
            (setq fc (current-frame-configuration))
            (consult--buffer-action cand 'norecord)
            (select-window (minibuffer-window) 'norecord)
            (when (and semi-mini-windows
                       (not (cl-subsetp semi-mini-windows
                                        (get-buffer-window-list buf nil frame))))
              (my-set-frame-configuration fc)
              (setq fc nil))))))))

It does have a bit of code that might be vertico specific, or it might also be useful for some other completion frameworks as well (semi-mini-windows).

The function set-frame-configuration does 3 things:

• set window config for each frame (with DONT-SET-MINIWINDOW=nil);
• set frame parameters for each frame;
• delete extra frames if there are some.

Here we might need use a custom version to change the first 2 things that it does.
We'd need a custom version if we want to set DONT-SET-MINIWINDOW=t.
Setting frame parameters causes flickering, so we'd need a custom version that doesn't do that.

[mpedramfar]

From the manual:

The dedicated flag (see Dedicated Windows) has a slightly different meaning for side windows. When a side window is created, that flag is set to the value side to prevent display-buffer to use the window in other action functions.

And in the Dedicated Windows page:

Functions supposed to remove a buffer from a window or a window from a frame can behave specially when a window they operate on is dedicated. We will distinguish four basic cases, namely where (1) the window is not the only window on its frame, (2) the window is the only window on its frame but there are other frames on the same terminal left, (3) the window is the only window on the only frame on the same terminal, and (4) the dedication’s value is side (see Displaying Buffers in Side Windows).

In particular, delete-windows-on (see Deleting Windows) handles case (2) by deleting the associated frame and cases (3) and (4) by showing another buffer in that frame’s only window. The function replace-buffer-in-windows (see Buffers and Windows) which is called when a buffer gets killed, deletes the window in case (1) and behaves like delete-windows-on otherwise."

Attempting to use a stronger form of window dedication may not be the best approach since it's against the documentation, and just any documentation, the manual.
Because of this, if consult doesn't implement a workaround, the most straightforward solution for issue 2 might as well be something like this (I haven't tested this code):

(defun my-display-buffer-in-side-window-advice (fun &rest args)
  ;; check the stack trace to see if this function is being called from `consult--buffer-preview':
  (if (backtrace-get-frames 'consult--buffer-preview) 
      (let ((new-win (save-window-excursion (apply fun args))))
        (unless (member new-win
                        (get-buffer-window-list
                         (window-buffer (minibuffer-window))))
          (apply fun args)))
      (apply fun args)))

(advice-add #'display-buffer-in-side-window
            :aournd #'my-display-buffer-in-side-window-advice)

This looks a bit more like hacking, rather than a configuration.

I haven't looked into consult-line-multi and other preview function. I'll take a look if I have time, though it could take a while.

[minad]

Let's not debate the side window issue now. It is not the right place to fix this here and now. vertico-buffer is not completely finalized. For example it misses a vertico-multiform-buffer toggle. This means vertico-buffer may still change. Also I prefer a more cautious approach, not adding workarounds or complexity prematurely.

I haven't looked into consult-line-multi and other preview function. I'll take a look if I have time, though it could take a while.

The only function which needs a modification is consult--jump-preview. I've simplified this function a little bit yesterday.

Would you be interested in preparing a patch? I suggest the following to make some progress:

1. Fix consult--jump-preview such that it relies on consult--buffer-preview
2. Use your implementation of consult--buffer-preview, for now without exclusion and the side window workaround.

If this works out we can proceed handling edge cases. Did you sign the FSF papers? See https://github.com/minad/consult#contributions

[arouene]

Hello, I'm having the same problems, as I'm using display-buffer-alist a lot.

I'm fixing the problems using this code in my configuration :

(defun my/consult-display (consultf)
  "Fix interactions between consult-buffer and display-buffer-alist"
  (let ((save-overriding-action display-buffer-overriding-action)
        (preview (funcall consultf)))
    (lambda (action cand)
      (setq display-buffer-overriding-action '(display-buffer-same-window))
      (funcall preview action cand)
      (setq display-buffer-overriding-action save-overriding-action))))

(advice-add 'consult--buffer-preview :around #'my/consult-display)

If it's helps you.

It's temporarily setting display-buffer-overriding-action to display-buffer-same-window.

I think it's not a perfect solution nor implementation, as if you go do something else leaving the minibuffer open in the middle of the consult-buffer action, the variable display-buffer-overriding-action will still be display-buffer-same-window.

But outside of this use-case, this solution is working great.

[minad]

@arouene I also plan to add a customization variable to exclude certain buffers from preview, see #969. This can also help in another way.

[minad]

@mpedramfar Now that Consult requires Emacs 28.1 with enhanced set-window-configuration and Consult got buffer preview excludes we can take another try to get your patch merged. Do you have FSF copyright assigment and are you interested in creating a PR?