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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Empty initial candidate list #115

Closed
cmacrae opened this issue Aug 25, 2021 · 28 comments 路 Fixed by #150
Closed

Empty initial candidate list #115

cmacrae opened this issue Aug 25, 2021 · 28 comments 路 Fixed by #150

Comments

@cmacrae
Copy link

cmacrae commented Aug 25, 2021

Hey @minad 馃憢

Thank you very much for vertico 馃檱

I seem to have some strange issue with candidates not appearing in the initial list.
This seemingly happens at "some point" after I've started Emacs. Initially it seems to work fine, providing candidates like so:

image

But after a while, initial candidates disappear:
image

If I type <SPC> then <Backspace> the list populates.

Any ideas what could be causing this?

Here's the config I'm using:

(use-package vertico
  :hook (after-init . vertico-mode)
  :custom
  (vertico-resize t))
@kings2u
Copy link
Sponsor

kings2u commented Aug 28, 2021

I am having the same issue and am having a hard time getting to the bottom of it. I cannot reproduce it with the following minimal config using emacs -Q:

(package-initialize)
(require 'consult)
(require 'vertico)
(require 'corfu)
(require 'orderless)
(vertico-mode)
(corfu-global-mode)
;; (setq completion-styles '(substring))
(setq completion-styles '(orderless))
(global-set-key (kbd "C-s") 'consult-line)

However, I can reproduce it using my normal config if I do the following:

  1. Start emacs and open an elisp file.
  2. Enable corfu-mode in the buffer.
  3. Perform dabbrev-completion on a string that I know will give me the corfu popup of options.
  4. Cancel the input with C-g and then try to use consult-line. Then I get the empty list of candidates cited by @cmacrae. As I type to search, the minibuffer might pop in and out of existence to display the results inconsistently.

I will try eliminating parts of my config to get at which setting is causing the error. I鈥檓 also willing to supply my entire configs for the relevant packages if @minad needs more info.

@minad
Copy link
Owner

minad commented Sep 4, 2021

@cmacrae I cannot reproduce this with emacs -Q.

@aaronjensen
Copy link
Contributor

I also see this, but don't have a more narrow repro. Restarting emacs fixes it. Disabling mini-frame fixes it too (until I re-enable it).

@aaronjensen
Copy link
Contributor

aaronjensen commented Sep 21, 2021

For me, the problem is the input-pending-p check

After this line in mini-frame input-pending-p is true, but before it, it is not.

Removing the input-pending-p is insufficient because of this while-no-input. If I remove that too, it works.

@aaronjensen
Copy link
Contributor

@cmacrae @kings2u can you try #131 and see if it helps?

@kings2u
Copy link
Sponsor

kings2u commented Sep 21, 2021

Thanks for looking into this @aaronjensen. Unfortunately your fix did not solve my particular bug. I'll dive into the issue deeper asap.

@cmacrae
Copy link
Author

cmacrae commented Sep 22, 2021

@cmacrae @kings2u can you try #131 and see if it helps?

Thanks so much for looking into this @aaronjensen 馃檱 I'm running your changes now. So far so good, but I haven't been using them for long. Will report back in a little while :)

@minad
Copy link
Owner

minad commented Sep 22, 2021

@aaronjensen Thanks for looking into this. Can you please report this issue to mini-frame to see if it can be worked around there?

As an alternative I have a small mini-popup package which is technically a bit simpler than mini-frame and might work for you. However it is more limited than mini-frame, doesn't play well with exwm for example. On the other hand it seems to work more robustly in my limited tests. I've recently seen there is also a vertico-posframe package, but I didn't try it yet. Also note the vertico-buffer extension which allows you to open the minibuffer at the top - but you probably still prefer a floating minibuffer?

@aaronjensen
Copy link
Contributor

I think it'd have to go all the way to emacs. I don't see that mini-frame is doing anything special. Of note, from the docs of input-pending-p:

Actually, the value is nil only if we can be sure that no input is available;
if there is a doubt, the value is t.

I wonder if this is one of those "doubt" situations... I may be able to try and narrow a repro down at some point.

I did try mini-popup, but I can't remember why I switched back to mini-frame other than I already had exclusions and faces set up right on it. There may have been other things... I can try it again. I'll also look at vertico-posframe.

@aaronjensen
Copy link
Contributor

Also, you may want to consider something like #131 regardless. Right now if there is actually input pending for some legitimate reason it would break in the same way, I think.

@minad
Copy link
Owner

minad commented Sep 22, 2021

Also, you may want to consider something like #131 regardless. Right now if there is actually input pending for some legitimate reason it would break in the same way, I think.

I am not inclined to merge this workaround, for these reasons:

  1. I don't like to add fixes here, which should actually be fixed at the root. mini-frame is the issue here, the issue does not occur with the usual minibuffer. mini-frame does a lot of special things and when I tested it a while ago, it lead to breakage of other packages.
  2. The input pending check I have right now is there for a reason - it makes sense like this. If there is input pending (even after init), the expensive candidate computation must be interruptible such that the UI stays responsive. This is particularly important during initialization, since then the initial minibuffer input is usually empty such that no filtering is performed. This leads to the computation and sorting of all available candidates, which is expensive and which must necessarily be interruptible, otherwise one ends up with a pause or lag right at the beginning.

@aaronjensen
Copy link
Contributor

I don't like to add fixes here, which should actually be fixed at the root.

I totally agree with this. I'm not able to be convinced it is a mini-frame issue yet, but that's just from lack of clarity.

If there is input pending (even after init), the expensive candidate computation must be interruptible such that the UI stays responsive.

This also makes sense, but would it be reasonable to have a method of resuming if it is interrupted? That's ultimately the issue right now is that something (unclear what) interrupts the first update, but it will not update again until there is user input. I'd venture a guess that the expectation is that that update would get to run again immediately after the pending input is processed, allowing results to be displayed.

That said, I'm happy to play around with the alternatives and see if I can get something working. I looked at vertico-posframe and it currently lacks a way to opt out of using it for certain commands, but I'm sure I can hack that in if I end up using that.

@minad
Copy link
Owner

minad commented Sep 22, 2021

I'm not able to be convinced it is a mini-frame issue yet, but that's just from lack of clarity.

Why is that? You said yourself that you don't see the issue without mini-frame. From my experience mini-frame is not a robust package, so I am quicker to blame it, which may be unfair. But there is a reason why I attempted something else with mini-popup. I also argue that the Vertico code in its current form makes sense, even if it would happen with the default Emacs minibuffer I would wonder if there doesn't actually exist an underlying input handling bug in while-no-input.

This also makes sense, but would it be reasonable to have a method of resuming if it is interrupted?

No such mechanism exists, one could use a timer. I just assume that while-no-input etc do the right thing. This may not be the case but then one would need some clarification from Emacs upstream on how this API is actually supposed to be used. However I rarely had issues with such spurious interrupts and if it happens you can always press space ;)

I'd venture a guess that the expectation is that that update would get to run again immediately after the pending input is processed, allowing results to be displayed.

Yes, that's the assumption. That's the idea of the while-no-input API. (EDIT: I didn't read you correctly I think. The assumption is that the pending input will lead to subsequent commands and display updates. The actually interrupted code is not to be resumed.)

That said, I'm happy to play around with the alternatives and see if I can get something working. I looked at vertico-posframe and it currently lacks a way to opt out of using it for certain commands, but I'm sure I can hack that in if I end up using that.

Above I criticized that mini-frame is not robust. My criticism also applies to posframe. Therefore I am using the child frame API directly in corfu and mini-popup. But if you look at the code of mini-popup, miniframe, corfu and posframe - they are all full of hacks to get the child frames to work. The underlying cause is that the Emacs child frame code is hard to use and buggy. I don't recommend child frames at all if they can be avoided. Personally I am using Corfu and it works well enough for my purposes but from time to time new child frame bugs crop up there, which I also port to mini-popup then.

@aaronjensen
Copy link
Contributor

aaronjensen commented Sep 22, 2021

Why is that? You said yourself that you don't see the issue without mini-frame.

Because I have no idea how make-frame-visible could trigger there being pending input when there wasn't before. If that's something in mini-frame's usage of it, I just don't know what it is. I'm just not sure if there is something in the bowels of the frame or child-frame code that could set the input pending flag.

Yes, that's the assumption. That's the idea of the while-no-input API.

Well, the idea of it is that it aborts what it is doing when there is input. It will not resume unless you explicitly build that (for example, one could set a timer to run the operation again if it was interrupted, which would allow the pending input to be processed. (I see you just mentioned a timer in the PR 馃憤 )

@minad
Copy link
Owner

minad commented Sep 22, 2021

Because I have no idea how make-frame-visible could trigger there being pending input when there wasn't before.

I also don't know. But what I know is that the child frame API is buggy and hard to use. For me it isn't unexpected to see such issues. On the other hand I haven't seen this with mini-popup, which points at mini-frame being the problem and not the underlying child frame code.

Well, the idea of it is that it aborts what it is doing when there is input. It will not resume unless you explicitly build that.

I disagree with that. If input is pending, the input will be processed and execute further commands and the post-command-hook, which implies resumption. However the assumption is probably too restricted if frames are involved since the post-command-hook is buffer local in the minibuffer and while-no-input is unrestricted. On the other hand there is while-no-input-ignore-events to explicitly exclude special events, if these particular events will not lead to commands and post command hooks. Also note that the while-no-input pattern and post-command-hook pattern are used in the same form by icomplete. I didn't invent this, so I am not particularly happy to invent my own fix here or at least I would like to see a similar fix proposed to upstream.

I see you just mentioned a timer in the PR

You mentioned timer before in your PR - I just acknowledged that this would be a potential solution. This does not mean that I will merge such a work around for the given reasons. At first the root cause should be narrowed down better.

@aaronjensen
Copy link
Contributor

If input is pending, the input will be processed and execute further commands and the post-command-hook

There is the mysterious documentation I referenced earlier: "Actually, the value is nil only if we can be sure that no input is available; if there is a doubt, the value is t." which essentially says this cannot be relied on.

You mentioned timer before in your PR - I just acknowledged that this would be a potential solution. This does not mean that I will merge such a work around for the given reasons. At first the root cause should be narrowed down better.

Sounds good. If I get some time I'll see what I can figure out. The most odd thing is that this is not constant. If I restart emacs, it doesn't happen. Something happens that causes the input-pending issue, and once that happens, it's always there until I restart again.

@minad
Copy link
Owner

minad commented Sep 22, 2021

There is the mysterious documentation I referenced earlier: "Actually, the value is nil only if we can be sure that no input is available; if there is a doubt, the value is t." which essentially says this cannot be relied on.

You are probably right about this if one wants to be on the safe side. I am also kind of astonished that it works so well without a manual restart. I never observed spurious event issues and I've yet to see a reproducible where mini-frame is not involved. And if there are certain spurious events one could add them to while-no-input-ignore-events. However the reason for me not seeing spurious event issues is that these input events are rare since the recomputation is often fast enough. This is like relying on a race condition 馃し However when typing the input quickly the character inputs are usually not leading to such spurious return values. I think here we are talking about another type of event, maybe not even a keypress event. Admittedly I never looked carefully into these input event types, so I am out of my depth here.

Sounds good. If I get some time I'll see what I can figure out.

Sure. But note that I won't merge a special workaround for mini-frame. I've been strict on this one before (also regarding Consult) and it then lead to fixes in mini-frame which is a better outcome. Currently there is exactly one mini-frame work around (as I am aware) in Vertico:

(unless (frame-root-window-p (active-minibuffer-window))

But this work around is of a general nature and therefore feels acceptable. Vertico resizing generally bails out of window resizing when the minibuffer window lives in its own frame.

@minad
Copy link
Owner

minad commented Sep 22, 2021

One more remark - there has been some Emacs upstream discussion in bug#38024 about the usage of while-no-input in icomplete before. It may make sense to dig up that discussion and its conclusion. I haven't seen manual restart using a timer being discussed there, but I also don't remember what the actual problem was.

@kings2u
Copy link
Sponsor

kings2u commented Sep 22, 2021

The issues being discussed are a little above my pay grade, but I did narrow down what was causing my particular issue. It was the package yascroll. To reproduce, eval the following in the scratch buffer of an emacs -Q instance:

(package-initialize)
(require 'consult)
(require 'vertico)
(require 'corfu)
(require 'orderless)
(vertico-mode)
(corfu-global-mode)
(setq completion-styles '(orderless))
(global-set-key (kbd "C-s") 'consult-line)

(use-package yascroll) 
(global-yascroll-bar-mode 1)

Then in a new line type the word "texts", enter a new line, type "tex", M-x dabbrev-completion RET, wait for the mini-frame (not sure if that鈥檚 the right word) to pop up with the suggestions "text" and "texts," press C-g to exit the mini-frame and then press C-s. The resulting consult-line search will have the bug of an empty list of candidate lines in the minibuffer.

My lisp skills are not good enough to diagnose why the yascroll package causes this error in conjunction with vertico and corfu, but hopefully this gives you more clues as to what鈥檚 going on. :)

@aaronjensen
Copy link
Contributor

I have a patch ready to apply, which fixes the underlying emacs issue. See: https://lists.gnu.org/archive/html/emacs-devel/2021-10/msg01242.html

Unfortunately, this workaround prevents it from working because help-echo events are not ignored. I don't understand why it is required and if you can change it to include everything from the default:

'(thread-event file-notify select-window help-echo move-frame iconify-frame make-frame-visible focus-in focus-out config-changed-event selection-request)

@minad
Copy link
Owner

minad commented Oct 22, 2021

@aaronjensen Great, thank you for looking into the underlying issue! How is icomplete affected by this?

;; bug#38024: Icomplete uses `while-no-input-ignore-events' to repair updating issues

@aaronjensen
Copy link
Contributor

I wish I knew. I have no idea why narrowing that list fixes anything or even what the initial problem was.

@minad
Copy link
Owner

minad commented Oct 31, 2021

@aaronjensen Btw, are the other child frame packages (vertico-posframe, mini-popup) a solution for you? See https://github.com/minad/vertico#child-frames-and-popups. vertico-posframe seems to be pretty solid now, while my experience with mini-frame has always been a bit shaky.

@aaronjensen
Copy link
Contributor

I forgot to report back here, but the fix for input-pending-p was installed to master. Another change was installed as well that makes it so the problem should only happen the first time after mousing over the echo area, so even override of while-no-input-ignore-events it should only happen once.

I can't reproduce the issue with vertico-posframe, though I have no idea what it does that is different. I prefer mini-frame because it does dynamic resizing and has a variable that lets me ignore specific commands, which I make use of.

If you added help-echo to the override of while-no-input-ignore-events it should work. I'm still very curious if there is actually an issue still that requires that override.

@aaronjensen
Copy link
Contributor

I found the difference. vertico-posframe sets the frame param (no-accept-focus . t), as does mini-popup. This causes the help-echo event not to get added because the parent frame never loses focus (the event is added on macOS to clear the echo of the frame losing focus). mini-frame does not set this. As a result, mouse selection works better but the help-echo event gets added to the queue because the parent frame loses focus. It should get ignored by the new code in Emacs 29, but it doesn't because of the code borrowed to work around the icomplete bug.

What would you think about adding help-echo here or removing the work-around until the bug surfaces?

@aaronjensen
Copy link
Contributor

I can not reproduce the icomplete bug using the original repro when the workaround is removed. I'm going to submit a patch to remove the work around because it does not seem necessary anymore and it is having downstream impacts when it is copied to other libraries like this.

@gcv
Copy link
Contributor

gcv commented Nov 19, 2021

@aaronjensen: I ran into this problem. Do I understand correctly that your patch applies only to Emacs 29, and a workaround for pre-29 requires use of the (no-accept-focus . t)? Adding that value to mini-frame-show-parameters seems to work for me with minimal side effects (clicking outside the frame no longer dismisses it).

@aaronjensen
Copy link
Contributor

@gcv that all sounds right.

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