minad / consult

:mag: consult.el - Consulting completing-read
GNU General Public License v3.0
1.22k stars 102 forks source link

EXWM: X window preview issues #204

Closed thomasheartman closed 3 years ago

thomasheartman commented 3 years ago

When using EXWM and Consult's buffer preview functionality with X windows across multiple frames/monitors, the buffers aren't properly restored after switching buffers.

This is similar to this issue, but doesn't seem to be the same. It is also independent of emacs-mini-frame.

While the window layout restoration works well if the buffer with the preview and the X window were on the same frame, the restoration doesn't work properly when they're on different frames, and the X window ends up being buried if not selected. For other buffers it seems to work as expected (but then again, other buffers don't have the same limitation as X buffers).

There's also an issue with the preview functionality for X buffers, where the X window 'steals focus' from the minibuffer. When this happens, the next and previous key bindings for consult don't work anymore (because the X window has the focus), but the minibuffer is still open/active. I'll see if I can reproduce this reliably and get back to you (I'm a bit short on time just now; sorry), but wanted to raise it in case it's already a known issue.

Thanks for your time and attention! Let me know if you need anything else and I'll try and respond as quickly as I can. Cheers!

minad commented 3 years ago

This is mostly a duplicate of #178. If I understand this correctly, EXWM does not allow duplicating X buffers. This is different from how Emacs buffers usually behave. Basically this is an EXWM bug. The focus stealing is also an EXWM bug.

However we have a solution now, since I added configurable buffer sources since then. Basically you have to replace consult--source-buffer (and the other problematic buffer sources) with buffer sources which filter out the X buffers or do not perform preview for them. Alternatively you can directly modify the consult--source-buffer and the other problematic buffer sources.

This is the current source definition:

(defvar consult--source-buffer
  `(:name     "Buffer"
    :narrow   ?b
    :category buffer
    :face     consult-buffer
    :history  buffer-name-history
    :state    ,#'consult--buffer-state
    :items
    ,(lambda ()
       (let ((filter (consult--regexp-filter consult-buffer-filter)))
         (seq-remove (lambda (x) (string-match-p filter x))
                     (consult--cached-buffer-names)))))
  "Buffer candidate source for `consult-buffer'.")

Modify it like this:

(defvar consult--source-x-buffers
  `(:name     "Buffer"
    :narrow   ?b
    :category buffer
    :face     consult-buffer
    :history  buffer-name-history
    :action  ,(lambda (buf) (funcall consult--buffer-display buf)) ;; action instead of state, disables preview!
    :items
    ,(lambda ()
       (let ((filter (consult--regexp-filter consult-buffer-filter)))
         ;; Filter out the x buffers here!
         (seq-remove (lambda (x) (string-match-p filter x))
                     (consult--cached-buffer-names)))))
  "Buffer candidate source for `consult-buffer'.")

I cannot provide a better out-of-the-box configuration for this unfortunately, and I can also not offer more extended support here. I am not an EXWM user. If you manage to create a working configuration, I would appreciate if you can post the configuration to the https://github.com/minad/consult/wiki.

If this configuration effort turns out too difficult or makes other problems, you can also disable preview for EXWM by setting the preview key to nil, or change preview to manual preview by setting the preview key to a key instead of 'any.

minad commented 3 years ago

The bug is actually not a duplicate of #178, but of #186.

minad commented 3 years ago

I just checked the EXWM issue tracker - it seems there is no issue for this window restoration problem. When entering the minibuffer the window configuration is saved and restored afterwards, see https://github.com/emacs-mirror/emacs/blob/a304b22bc9f1396cd6958b0a6fc6092437e36d96/src/minibuf.c#L502. This is pretty low-level in the Emacs codebase. However for some reason EXWM does not manage to correctly restore the configuration afterwards. Can you please raise the issue there too and link it to this issue? It should also be possible to create a very small reproducible example independent of Consult, which reads from the minibuffer and switches buffers in a post-command-hook from inside the minibuffer.

While unrelated to the problem here, the issue in #178 was pretty similar since there emacs-miniframe did not properly save/restore the window-configuration. But this has been fixed in emacs-miniframe as far as I know.

I reopened the issue here since it has not been fixed completely. We can work around it by configuring buffer sources, sure. But EXWM should actually make sure that the configuration is properly restored.

thomasheartman commented 3 years ago

Super appreciate you taking the time to look into this! I've already disabled the previewing of buffers completely, so it's not something that bothers me anymore, but I quite liked the preview and would like to be able to use it.

I don't quite have the time right now, but I'll look into the filtering you suggested in the coming days. I'll also open an issue on EXWM's repo. Feel free to chase me if I haven't gotten back to you by, say, Monday.

Again: thanks very much for your time and energy! I'll try and do my part in the next few days. Cheers!

thomasheartman commented 3 years ago

Alright, here's an update:

On the code snippet you mentioned above (and configuring the source buffers): I started playing around with it, but wasn't quite able to grok it at this point. Is this correct: if you create two sources, one for regular buffers, one for X buffers (without preview), then they'll both show up in the full list of buffers, but only the regular buffers are previewed? For filtering, what sort of information is available? Is it only the buffer name? When using Marginalia, X buffers also have what I believe to be their major mode listed. Is it possible to use this somehow? Specifically, I'm talking about this:

2021-02-05-170157_2231x247_scrot

The column that says "EXWM", "Markdown", "Nix", etc. That would make the filtering much easier.

And I opened an issue with EXWM as you requested. Thanks for the suggestion and insight you've offered so far.

Cheers.

minad commented 3 years ago

Is this correct: if you create two sources, one for regular buffers, one for X buffers (without preview), then they'll both show up in the full list of buffers, but only the regular buffers are previewed?

Yes, that's right.

For filtering, what sort of information is available? Is it only the buffer name?

I don't know. Probably the major-mode if Marginalia prints EXWM for the major mode, so it is probably exwm-mode?

oantolin commented 3 years ago

I don't know. Probably the major-mode if Marginalia prints EXWM for the major mode, so it is probably exwm-mode?

I think he meant filtering as opposed to just being displayed as an annotation, @minad. If that is what @thomasheartman meant, the answer would be that only the buffer name is available for filtering.

minad commented 3 years ago

@oantolin No, @thomasheartman meant how he could filter the buffers to create custom sources. He wants to create two multi sources one with only EXWM buffers without preview (and thus without preview problems) and another source without the EXWM buffers but with preview.

thomasheartman commented 3 years ago

@minad and @oantolin, I'm afraid I don't understand the differences between what you're saying.

I want to:

To do that, as I understand it, I can create a new source which takes only EXWM buffers and modify the current buffer source to exclude EXWM buffers. That way, the EXWM source can not preview, while the other source can. Both sources should also appear in the buffer list.

Because the buffer names don't necessarily have anything in them that identifies them as EXWM buffers (it can be done by modifying how EXWM names buffers, but I was hoping I'd not have to do this), I thought maybe I could use some information about the buffer, such as the major mode, to filter the buffers.

I asked about Marginalia because that is also under @minad's user, so I thought @minad might know whether the same information is available to Consult (to the filtering specifically), but I might not have worded the question well enough.

Does that clarify the question? And does it change any answers? Thanks.

edit: If it wasn't clear, I was wondering whether I can filter buffers in a source based on their major mode or other information besides their name?

minad commented 3 years ago

Yes, sure the major-mode is available to the multi source filtering. Access it via (buffer-local-value buffer 'major-mode) in the :items function of the source you are creating.

When I said - "I don't know" in my comment above, I meant that I am unsure about the proper way to detect the EXWM buffers. Sorry for the misunderstanding. If the major-mode variable contains a proper value we can use that for filtering. It seems to do that since Marginalia prints "EXWM", so you can use the major-mode variable and filter based on its value. In fact you can access any information present in any buffer, there is no limitation.

thomasheartman commented 3 years ago

Aight, sweet! I’ll look into it tomorrow (probably). Thanks 🙏

On 5 Feb 2021, at 18:24, Daniel Mendler notifications@github.com wrote:

 Yes, sure the major-mode is available to the multi source filtering. Access it via (buffer-local-value buffer 'major-mode) in the :items function of the source you are creating.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

minad commented 3 years ago

Cool, it would probably be good to have a working configuration for EXWM since there seem to be some users after all of the full Emacs/OS. Maybe I will even use it myself at some point ;)

But for now there are some issues preventing me from using it, mainly separation concerns and I am also not sure about its architecture since I would not want my wm to hang-up when Emacs is doing things. Maybe one needs a separate Emacs instance only for the WM? But then the advantage would be gone I guess.

thomasheartman commented 3 years ago

Yeah, there are a few things about EXWM that aren't ideal (such as if you start a long running shell command and the entire WM freezes), but I'm very happy with it in the grand scheme of things. It's a bit of effort, sure, but you reap what you sow, I suppose.

As for the problem at hand, I got this working:


  (setq consult--source-buffer
    `(:name     "Buffer"
            :narrow   ?b
            :category buffer
            :face     consult-buffer
            :history  buffer-name-history
            :state    ,#'consult--buffer-state
            :items
            ,(lambda ()
               (let* ((filter (consult--regexp-filter consult-buffer-filter))
                  (no-x (seq-remove
                     (lambda (b) (eq 'exwm-mode (buffer-local-value 'major-mode b)))
                     (consult--cached-buffers)))
                  (buffer-names (mapcar (lambda (b) (buffer-name b)) no-x)))
             (seq-remove (lambda (x) (string-match-p filter x))
                     buffer-names)))))

  (defvar consult--source-x-buffers
    `(:name     "X buffers"
                :narrow   ?x
                :category buffer
                :face     consult-buffer
                :history  buffer-name-history
                :action  ,(lambda (buf) (funcall consult--buffer-display buf)) ;; action instead of state, disables preview!
                :items
                ,(lambda ()
           (mapcar (lambda (b) (buffer-name b))
               (seq-filter (lambda (b) (eq 'exwm-mode (buffer-local-value 'major-mode b)))
                       (buffer-list)))))
    "X buffer candidate source for `consult-buffer'.")

  (add-to-list 'consult-buffer-sources 'consult--source-x-buffers)

EXWM and regular buffers are now separate sources. However, if I set preview-key to any for consult-buffer, it still previews X windows. I thought the :action was supposed to prevent that? Also, because they're separate sources now I suppose, EXWM buffers end up either at the very top or at the very end of the buffer list (depending on whether you 'append the source or not). Previously they would blend together, which I think was more intuitive. Though from what I understand, that's part of the design, so I don't expect that to be something that can be easily modified.

However, when working on this, a thought struck me: could you leave the consult--source-buffer mostly as it is, but make the filtering happen at the :action or :state stage? I don't know about :state, but if :action is a replacement, then it looks like the lambda receives the buffer as an argument. At that point, you could check the buffer's major mode and decide whether to preview the buffer or not at that point. Is that possible?

Something like this (where do-nothing and preview-buffer are placeholders):

(defvar consult--source-buffer
  `(:name     "Buffer"
          :narrow   ?b
          :category buffer
          :face     consult-buffer
          :history  buffer-name-history
          :action ,(lambda (b)
             (if (eq 'exwm-mode (buffer-local-value 'major-mode b))
                 (do-nothing)
               (preview-buffer)))
          :items
          ,(lambda ()
         (let ((filter (consult--regexp-filter consult-buffer-filter)))
           (seq-remove (lambda (x) (string-match-p filter x))
                   (consult--cached-buffer-names)))))
  "Buffer candidate source for `consult-buffer'.")

edit: added example of :action-filtering idea.

minad commented 3 years ago

There are the :state and :action fields of the sources which have a bit of a different purpose. This needs better documentation, the feature is pretty new. But I think you got it mostly right.

Now to your possibilities:

  1. You can either use two sources, but then as you say, the sources are sorted in a way which you may like or not. Note that you can also disable sorting of consult-buffer completely by overwriting :sort in consult-config. But this is probably not desired.
  2. Then as you say, you have the filtering possibility (this is similar to what I thought about in #186). You can create a single source, with a :state argument (not an :action argument!) where you add filtering based on the candidate. The preview could be performed only for the non-exwm sources and the final action will be performed for any source.

For option 1 the following works for me (I don't see the preview issue you mentioned, preview only works for the source for which I enabled it):

(setq consult--source-lisp-buffers
  `(:name     "Lisp Buffer (Preview)"
    :narrow   ?l
    :category buffer
    :face     consult-buffer
    :history  buffer-name-history
    :state    ,#'consult--buffer-state
    :items
    ,(lambda ()
       (let ((filter (consult--regexp-filter consult-buffer-filter)))
         (mapcar #'buffer-name
                 (seq-filter (lambda (x) (and
                                          (eq (buffer-local-value 'major-mode x) 'emacs-lisp-mode)
                                          (not (string-match-p filter (buffer-name x)))))
                             (consult--cached-buffers)))))))

(setq consult--source-other-buffers
  `(:name     "Other Buffer (No Preview)"
    :narrow   ?o
    :category buffer
    :face     consult-buffer
    :history  buffer-name-history
    :action   ,(lambda (buf) (funcall consult--buffer-display buf))
    :items
    ,(lambda ()
       (let ((filter (consult--regexp-filter consult-buffer-filter)))
         (mapcar #'buffer-name
                 (seq-filter (lambda (x) (and
                                          (not (eq (buffer-local-value 'major-mode x) 'emacs-lisp-mode))
                                          (not (string-match-p filter (buffer-name x)))))
                             (consult--cached-buffers)))))))

(setq consult-buffer-sources '(consult--source-lisp-buffers consult--source-other-buffers))
thomasheartman commented 3 years ago

@minad It seems the reason the preview filtering didn't work as expected was that I was on an older version of the repo. I've updated that now.

That said, that also seemed to intermingle the buffers in the buffers list as expected (not sure why or if I did something wrong). Anyway, I played around with it some more and found that this works, where all I do is change the consult-buffer-state function to a slightly tweaked one.

  (defun my-consult--buffer-state ()
    "Buffer state function that treats X buffers differently."
    (lambda (cand restore)
      (when cand
    (cond
     (restore (funcall consult--buffer-display cand))
     ((and (or (eq consult--buffer-display #'switch-to-buffer)
                   (eq consult--buffer-display #'switch-to-buffer-other-window))
               (let ((buffer (get-buffer cand)))
         (and buffer (not (eq 'exwm-mode (buffer-local-value 'major-mode buffer))))))
          (funcall consult--buffer-display cand 'norecord))))))

  (setq consult--source-buffer
        `(:name     "Buffer"
                    :narrow   ?b
                    :category buffer
                    :face     consult-buffer
                    :history  buffer-name-history
                    :state    ,#'my-consult--buffer-state
                    :items
            ,(lambda ()
               (let ((filter (consult--regexp-filter consult-buffer-filter)))
             (seq-remove (lambda (x) (string-match-p filter x))
                     (consult--cached-buffer-names))))))

I feel like this could have been simpler, but my elisp isn't quite up to par. I thought I might be able to simply delegate to the original consult--buffer-state with cand either as is or set to nil if it's an X buffer, but that didn't seem to work. Also, having to redefine the entire variable (consult--source-buffer) just to change one part of it (:state) seems a bit heavy. Is there as an easier way to do that?

Something like this perhaps?

(nconc consult--source-buffer (list :state #'my-consult--buffer-state))
minad commented 3 years ago

Yes, to both questions. You can reuse the consult--buffer-state function.

  (defun my-consult--buffer-state ()
    (let ((orig-state (consult--buffer-state)))
      (lambda (cand restore)
          ;; my-filter-function should return the cand only when restore=t or when the cand is non-exwm
          (funcall orig-state (my-filter-function cand restore) restore)
       )))

You can also reuse the consult--buffer-source if you want to do that, either by prepending fields (first fields come first in plist) or by using plist-put.

If you find a satisfactory solution can you please copy the configuration to the Consult wiki such that other exwm users can also benefit from it?

thomasheartman commented 3 years ago

Ah, nice, thanks! plist-put seems to do the trick, but I'm not able to get the snippet you posted to work.

Whenever I try and trigger it, I get an error message telling me Symbol’s value as variable is void: orig-state and I can't figure out why. Do you know what's up here?

And yeah, of course! I'd be happy to put the solution I find in the wiki. No problem :smile:

minad commented 3 years ago

This is the lexical-binding issue. Are you using an Emacs older than 27? If you evaluate it in the scratch buffer it should work automatically since for some reason there lexical binding is the default. In el files you have to add this lexical-binding magic comment. (I am really annoyed by this elisp binding mess, I would have never touched elisp if it wouldn't have gotten lexical bindings, but I am also glad that dynamic bindings exist for all the Emacs extensibility :laughing:)

thomasheartman commented 3 years ago

Huh, strange. I’m somewhat aware of the lexical binding issue, but haven’t quite read up on the specifics. And no, I’m using Emacs 28 (the native comp branch). Does that mean it should work? Anyway, I’ll look into it tomorrow. Thanks for the hint! ☺️

On 6 Feb 2021, at 20:34, Daniel Mendler notifications@github.com wrote:

 This is the lexical-binding issue. Are you using an Emacs older than 27? If you evaluate it in the scratch buffer it should work automatically since for some reason there lexical binding is the default. In el files you have to add this lexical-binding magic comment. (I am really annoyed by this elisp binding mess, I would have never touched elisp if it wouldn't have gotten lexical bindings, but I am also glad that dynamic bindings exist for all the Emacs extensibility 😆)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

minad commented 3 years ago

Yes, it should work if you evaluate the code with lexical bindings enabled.

thomasheartman commented 3 years ago

Huh, you're right. It does seem to work if in the scratch buffer or with lexical bindings enabled. Nice. With lexical bindings, it looks something like this:

  (defun consult-buffer-state-no-x ()
    "Buffer state function that doesn't preview X buffers."
    (let ((filter (lambda (cand restore)
                    (if (or restore
                            (let ((buffer (get-buffer cand)))
                              (and buffer
                                   (not (eq 'exwm-mode (buffer-local-value 'major-mode buffer))))))
                        cand
                      nil))))
      (lambda (cand restore)
        (funcall (consult--buffer-state) (funcall filter cand restore) restore))))

However, I couldn't make this work in my config because I use a literal org file, and I can't seem to figure out how to activate lexical bindings for the tangled file (or when evaluating snippets), so I also made this version, which doesn't require lexical bindings. It's a bit uglier, but it does the job:

  (defun consult-buffer-state-no-x ()
    "Buffer state function that doesn't preview X buffers."
    (lambda (cand restore)
      (funcall (consult--buffer-state) (if (or restore
                           (let ((buffer (get-buffer cand)))
                         (and buffer
                              (not (eq 'exwm-mode
                                   (buffer-local-value 'major-mode buffer))))))
                       cand
                     nil) restore)))

If you think that's good, I'll go ahead and write up a section on the wiki. If you've got anything you'd like to point out, please do. Again: thanks for all your help!

minad commented 3 years ago

Great! You are welcome. I suggest you fix the lexical binding issue in your config, this will create fewer problems in the long term. Dynamic binding is a bad default and I am glad that Emacs is moving to lexical binding by default. For this reason I would also prefer if you put the lexical binding version to the wiki. Please also also add a note on how this consult-buffer-state-no-x function can be installed. Maybe there should also be a big note in the beginning of the wiki page and the README that we are assuming lexical bindings everywhere. Maybe you can look at @protesilaos' dotemacs, which is a literate config. Basically after the literate org tangling, the lexical-binding magic header should appear in the generated init.el. I feel that the benefits of individual literate configs are small, except if you are maintaining a rich web document like @protesilaos with detailed documentation, which can serve educational purposes. While for configs it is quite common to see literate versions, in contrast there are nearly no literate Emacs packages. I only stumbled upon a handful of them - but I didn't do an extensive search.

I will close this issue now, since I think the issue has been fixed from the side of Consult by using filtering on the EXWM buffers. There is not much more we can do here. It does not make sense to add special handling/workaround code for EXWM to the Consult codebase. Hopefully EXWM can improve how it restores window configurations (https://github.com/ch11ng/exwm/issues/813). If you have further questions/comments, feel free to ask them here!

minad commented 3 years ago

One comment regarding your functions:

  (defun consult-buffer-state-no-x ()
    "Buffer state function that doesn't preview X buffers."
    (lambda (cand restore)
      (funcall (consult--buffer-state) (if (or restore
                           (let ((buffer (get-buffer cand)))
                         (and buffer
                              (not (eq 'exwm-mode
                                   (buffer-local-value 'major-mode buffer))))))
                       cand
                     nil) restore)))

While this function works, it is only accidental. It is wrong to funcall (consult--buffer-state), since consult--buffer-state maintains state over the whole completing read session (actually it does not, but it could and it is not guaranteed that it will stay like this). Other state functions maintain state for restoration, therefore they are named like this. I should emphasize again that lexical binding is a requirement.

The correct formulation is like this (see my other comment https://github.com/minad/consult/issues/204#issuecomment-774518056):

  (defun consult-buffer-state-no-x ()
    "Buffer state function that doesn't preview X buffers."
    (let ((state (consult--buffer-state)))
      (lambda (cand restore)
        (funcall state (if (or restore
                   (let ((buffer (get-buffer cand)))
                 (and buffer
                      (not (eq 'exwm-mode
                           (buffer-local-value 'major-mode buffer))))))
               cand
             nil) restore))))
minad commented 3 years ago

Another idea for EXWM preview - maybe it is possible to somehow obtain screenshots from the EXWM buffers? Then these screenshots could be used for Consult preview. Maybe even only as some kind of thumbnails, similar to what some window managers do when you press s-TAB. I suspect that taking the screenshots live could introduce some pauses, but it would be possible to cache them per EXWM buffer. But such a functionality would be a bigger project.

thomasheartman commented 3 years ago

Thanks again for all your input, especially regarding the potential issue of using funcall (consult--buffer-state). I've gone and changed that and also figured out how to set lexical binding for my config (which was easier than I thought).

As such, I think this is where we ended up (including how to set the configuration):

(defun consult-buffer-state-no-x ()
  "Buffer state function that doesn't preview X buffers."
  (let ((orig-state (consult--buffer-state))
        (filter (lambda (cand restore)
                  (if (or restore
                          (let ((buffer (get-buffer cand)))
                            (and buffer
                                 (not (eq 'exwm-mode (buffer-local-value 'major-mode buffer))))))
                      cand
                    nil))))
    (lambda (cand restore)
      (funcall orig-state (funcall filter cand restore) restore))))

(setq consult--source-buffer
      (plist-put consult--source-buffer :state #'consult-buffer-state-no-x))

I'll go ahead and add that to the Wiki along with a note about lexical bindings.

As for the value of literate configs: It's definitely a personal preference. While mine isn't nearly as fully fledged as the one you linked, I still get lots of value out of it simply by being able to document why I'm doing what I'm doing, using TODO-states to mark sections that need work, isolating sections, etc. But definitely: to each their own.

Regarding the EXWM preview idea: I think this is a great idea and have thought of something similar myself. However, I don't know where to even start, and this current functionality gets me 80% of the way there, so that's enough for now at least.

minad commented 3 years ago

I'll go ahead and add that to the Wiki along with a note about lexical bindings.

Yes, please! Also add a link to this issue/discussion for reference.

Regarding the EXWM preview idea: I think this is a great idea and have thought of something similar myself. However, I don't know where to even start, and this current functionality gets me 80% of the way there, so that's enough for now at least.

Yes, it is just an idea one can keep in mind. Maybe at some point someone will implement it. But I agree that the current simple filtering config covers the basic support.

thomasheartman commented 3 years ago

Yes, please! Also add a link to this issue/discussion for reference.

Done and done! 🥳

thomasheartman commented 3 years ago

@minad Just a little follow-up question on this. When previewing buffers, consult keeps the current (potentially previewed) buffer when it can't display a preview (i.e. for X buffers). Is there a way to show the original buffer (the one that was displayed in the window before starting the buffer switching) if there is no preview available?

That is, instead of passing nil to the state function if the current buffer shouldn't be displayed, pass the original buffer? Or at least something like that. Is the window's current (non-previewed) buffer available somehow?

It's not a big deal at all, but it can be a bit disorienting when switching between X buffers if there are non-X buffers in between them. I don't know whether this would make it better or not, but it could be worth a shot if it's not too difficult.

minad commented 3 years ago

Yes, this can be done. Just use the original buffer, by storing (current-buffer) in a let bound variable. For some reason I have not done this for consult--buffer-state. Maybe I should also implement this there.

EDIT: I took another look. While this is easily possible in your case, it is more complicated for multi sources. For multiple sources, for preview always the state function of the respective candidate source is called. This would mean that each of these state function would need special restoration behavior. I have to think a bit about this but I may have an idea on how this could be implemented relatively generically.

thomasheartman commented 3 years ago

Thanks! Yeah, that did the trick. Don't know whether it's better yet, but I'll try it out for a bit and see what I think (and also see whether there are any issues with stealing focus again).

Definitely sounds trickier in the general case, but it might be worth thinking about.

Thanks again!

minad commented 3 years ago

I generalized the state function slightly such that the original buffer is reset. Now the state function must accept all combinations of candidate=nil,non-nil and restore=nil,t. Before that, only (candidate/=nil && restore=nil) || restore=t was accepted.

thomasheartman commented 3 years ago

Ah, nice, so the new one version that it accepts now is candidate = nil && restore = nil, right? Should I change anything in the code I got thus far to take advantage of this?

I thought I'd update the wiki with this functionality, but want to make sure I get it right 🤓

And just for reference, this is what I'm rocking at the moment:

(defun consult-buffer-state-no-x ()
    "Buffer state function that doesn't preview X buffers."
    (let* ((orig-state (consult--buffer-state))
           (orig-buffer (buffer-name))
           (filter (lambda (cand restore)
                     (if (or restore
                             (let ((buffer (get-buffer cand)))
                               (and buffer
                                    (not (eq 'exwm-mode (buffer-local-value 'major-mode buffer))))))
                         cand
                       orig-buffer))))
      (lambda (cand restore)
        (funcall orig-state (funcall filter cand restore) restore))))
minad commented 3 years ago

Ah, nice, so the new one version that it accepts now is candidate = nil && restore = nil, right?

Yes.

I think this is wrong since get-buffer fails if cand is nil.

                             (let ((buffer (get-buffer cand)))
                               (and buffer
                                    (not (eq 'exwm-mode (buffer-local-value 'major-mode buffer))))))

Btw, you can use when-let:

                             (when-let (buffer (and cand (get-buffer cand)))
                               (not (eq 'exwm-mode (buffer-local-value 'major-mode buffer)))))
thomasheartman commented 3 years ago

Btw, you can use when-let:

Oh, thanks! Yeah, I wasn't aware that was a function until last night, and then never got around to playing with it.

As for whether it's wrong or not: I've been using it all day today and haven't noticed anything, at least. But I can also say after using it today: I personally prefer to show the original buffer if there is no preview available.

minad commented 3 years ago

As for whether it's wrong or not: I've been using it all day today and haven't noticed anything, at least. But I can also say after using it today: I personally prefer to show the original buffer if there is no preview available.

Yes, it makes sense to show the original buffer. I added general support in https://github.com/minad/consult/commit/58befea1b77bee63290dd3d4ed03b80bd48c943c. This means if one combines multiple sources and a source does not support preview, then the original buffer will be shown.

oantolin commented 3 years ago

Yes, it makes sense to show the original buffer. I added general support in 58befea.

This does feel much nicer. I hadn't realized that this had been bothering me. I was vaguely thinking of not using automatic previews for consult-buffer, but the urge has passed.

codygman commented 3 years ago

I think the code currently in the wiki breaks when after I preview some buffers and then press M-p all the way up to the Switch to: prompt.

I get this error:

Error in post-command-hook (consult--preview-post-command): (wrong-type-argument stringp nil)

Then preview is broken after that until after I start a new consult session.

My config in short looks like:

;; -*- lexical-binding: t -*-

;; tons of config

(use-package consult
  :init

  ;; Configure register preview function.
  ;; This gives a consistent display for both `consult-register' and
  ;; the register preview when editing registers.
  (setq register-preview-delay 0
    register-preview-function #'consult-register-format)
  :config
  (defun consult-buffer-state-no-x ()
    "Buffer state function that doesn't preview X buffers."
    (let ((orig-state (consult--buffer-state))
          (filter (lambda (cand restore)
                    (if (or restore
                            (let ((buffer (get-buffer cand)))
                              (and buffer
                                   (not (eq 'exwm-mode (buffer-local-value 'major-mode buffer))))))
                        cand
                      nil))))
      (lambda (cand restore)
        (funcall orig-state (funcall filter cand restore) restore))))

  (setq consult--source-buffer
        (plist-put consult--source-buffer :state #'consult-buffer-state-no-x))

  (autoload 'projectile-project-root "projectile")
  (setq consult-project-root-function #'projectile-project-root)
  :custom-face
  (consult-preview-cursor ((t (:inherit nil)))))
thomasheartman commented 3 years ago

Yeah; I remember having some issues as well. I ended up turning the previews off a while ago because they were a bit flaky, but I don't know whether I got the same error message. Which is to say: the code in the Wiki is most likely not perfect, but I don't have the relevant context in my head anymore, so I'm not sure I can offer much help here, I'm afraid.

However, if you manage to make it work properly, I'd be very interested in seeing what you did and trying it out for myself :smile:

In the meantime, maybe @minad has some ideas here?

minad commented 3 years ago

I can only speculate since @codygman did not supply a full stacktrace. But by inspecting the code above there is indeed a bug which would give the nil error.

(let ((buffer (get-buffer cand))) ;; <-- cand can be nil when restoring and no candidate has been selected
codygman commented 3 years ago

I can only speculate since @codygman did not supply a full stacktrace. But by inspecting the code above there is indeed a bug which would give the nil error.

(let ((buffer (get-buffer cand))) ;; <-- cand can be nil when restoring and no candidate has been selected

I'm sorry, there was no stacktrace after doing M-x toggle-debug-on-error, only a message. I'm guessing this is like how you can't (well, I can't) get stacktraces out of code I add into the modeline?

Thanks for the quick help again, if I can get time I'll try testing some alternatives and update the wiki.

minad commented 3 years ago

Yes, in the post command hook you don't directly get a stack trace. See here https://github.com/raxod502/selectrum/#contributor-guide. It is a bit complicated but otherwise Emacs would not be robust I guess :shrug:

codygman commented 3 years ago

Yes, in the post command hook you don't directly get a stack trace. See here https://github.com/raxod502/selectrum/#contributor-guide. It is a bit complicated but otherwise Emacs would not be robust I guess

Oh, I definitely didn't know about that! I'm wondering how I may have discovered that. I'd need to have thought something like:

My consult preview has some error, I toggled on debug and it didn't show up. Why? Well the consult preview is running through selectrum... I know go look for the contributor guide for selectrum for tips.

I can't imagine a way or method I would have arrived there though. I suppose if when I switched to selectrum I read (or at least skimmed) the documentation, wiki, and contribution guide in full I may have connected the dots.

I muse about this because I wonder if there's a way to help future users and avoid you linking to that over and over. I suppose a consult contributor guide that duplicates or links to that might help most here?

minad commented 3 years ago

It is all documented. See https://www.gnu.org/software/emacs/manual/html_node/elisp/Command-Overview.html. One cannot expect that everything is replicated multiple times at different places. As we all know there is no easy way to Emacs. Basically every corner of Emacs has weird edge cases and so on. That's on one hand technically bad and annoying, on the other hand it is like a puzzle :)

However I do not disagree with a contributors guide or with extending the contributors section. All documentation improvements are welcome. Generally I think the documentation is in an an acceptable state, but more end user focused than developer focused. But in Emacs, this distinction is a fuzzy, since as soon as you start tweaking your init.el you are already starting to develop.

And well, you can always ask on the issue tracker as you did!