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

Issues with updating the display of the list of candidates #226

Closed
vijaymarupudi opened this issue May 10, 2022 · 9 comments
Closed

Issues with updating the display of the list of candidates #226

vijaymarupudi opened this issue May 10, 2022 · 9 comments

Comments

@vijaymarupudi
Copy link

vijaymarupudi commented May 10, 2022

Hello,

I am having a problem with vertico not updating the list of candidates when I type keys in the minibuffer. The list of candidates is updated when I navigate through the list of candidates using C-n or C-p, but not before that. This occurs semi-consistently, probably due to a problem with debouncing or throttling logic if it exists?

I've included a screencast for a demo. I included the lossage at the end of the video. I am on the latest version of vertico on MELPA, the source code says 0.23. I am on the emacs master branch.

The issue might be happening when

(while-no-input (vertico--recompute-candidates pt content)))))
is interrupted by input.

GNU Emacs 29.0.50 (build 14, x86_64-pc-linux-gnu, GTK+ Version 3.24.33, cairo version 1.17.6) of 2022-05-09

Screencast: https://webm.red/t6j6.webm

@minad
Copy link
Owner

minad commented May 10, 2022

Can you reproduce this in Emacs 28? It could also be due to gtk, is this pgtk (which is unstable according to its maintainer)? The bug here seems to be that input events get lost, which should not happen.

I don't investigate Emacs 29 issues currently since there is too much churn to keep up with it right now. I get Emacs 29 reports every week, yours is the second third today. (All of those issues haven't been observed before on 28.)

Please reopen if you also observe this on Emacs 28.

@minad minad closed this as completed May 10, 2022
@vijaymarupudi
Copy link
Author

Makes sense, thank you for providing your rationale. Will reopen this issue once the next version of Emacs is out. I was using pgtk indeed. Really great for performance, but I guess it has some conflicts with vertico.

Some information for the future: I set a post-command-hook and inspected the value of (minibuffer-contents) for every keypress. I was reliably seeing the correct minibuffer-contents for each keypress. The return value of while-no-input also seemed to make sense to me. However, it looks like vertico.el is not handling the possibility of the return value of while-no-input being t?
Quoting the documentation:

Execute BODY only as long as there’s no pending input. If input
arrives, that ends the execution of BODY, and ‘while-no-input’
returns t. Quitting makes it return nil. If BODY finishes,
‘while-no-input’ returns whatever value BODY produced.

I suspect fixing handling this might fix the issue, but I lack the context and the elisp expertise to fix this myself.

Until then, I will revert to Emacs 28, because vertico runs my life.

@minad
Copy link
Owner

minad commented May 10, 2022

It is not about conflicts between vertico and pgtk. Pgtk is just not yet stable. It has bugs and in this case it seems to lose input events. You could ask the pgtk maintainer about this. I've read that there are no performance benefits of pgtk on x11 and only moderate benefits on Wayland.

@minad
Copy link
Owner

minad commented May 10, 2022

There is no need to handle the t return value. If while-no-input terminates with t due to input, the new input should then also run the vertico--exhibit post-command-hook such that the update happens then. But Vertico should not get stuck.

@vijaymarupudi
Copy link
Author

Thank you for the clarification on the design of vertico, that helped me out greatly. After playing around with message statements, I think the issue with pgtk/emacs 29 that impacts vertico is becoming clear to me. The problem is with the timing sequence of how pgtk or emacs 29 is relaying input as pending to elisp code. I've attached a screencast that I think demonstrates the issue well.

Emacs 29 is correctly calling post-command-hook with the full up-to-date content of the minibuffer (after consuming all input from the user). However, it erroneously interrupts while-no-input blocks when that hook is running. In other instances, it marks input as pending (returning t from input-pending-p) when input is not really pending. Both instances have been demonstrated in the screencast.
Here are the modified functions with message statements that I used for testing.

In the screencast, I am typing the string "not" very fast.

Screencast: https://webm.red/ds8i.webm

(defun vertico--exhibit ()
  "Exhibit completion UI."
  
  (let* ((buffer-undo-list t) ;; Overlays affect point position and undo list!
         (pt (max 0 (- (point) (minibuffer-prompt-end))))
         (content (minibuffer-contents)))
    (message "in vertico--exhibit, processing %S, input-pending-p: %S" content (input-pending-p))
    (unless (or (input-pending-p) (equal vertico--input (cons content pt)))
      (vertico--update-candidates pt content))
    (vertico--prompt-selection)
    (vertico--display-count)
    (vertico--display-candidates (vertico--arrange-candidates))))
    
(defun vertico--update-candidates (pt content)
  "Preprocess candidates given PT and CONTENT."
  ;; Redisplay the minibuffer such that the input becomes immediately
  ;; visible before the expensive candidate recomputation is performed (Issue #89).
  ;; Do not redisplay during initialization, since this leads to flicker.
  (message "in vertico--update-candidates, processing %S" content)
  (when (consp vertico--input) (redisplay))
  (let ((metadata (completion-metadata (substring content 0 pt)
                                       minibuffer-completion-table
                                       minibuffer-completion-predicate)))
    (pcase
        (let ((vertico--metadata metadata))
          ;; If Tramp is used, do not compute the candidates in an interruptible fashion,
          ;; since this will break the Tramp password and user name prompts (See #23).
          (if (and (eq 'file (vertico--metadata-get 'category))
                   (or (vertico--remote-p content) (vertico--remote-p default-directory)))
              (vertico--recompute-candidates pt content)
            (let ((non-essential t))
              (while-no-input (vertico--recompute-candidates pt content)))))
      ('nil (abort-recursive-edit))
      ('t (message "interrupted while-no-input!"))
      (`(,base ,total ,def-missing ,index ,candidates ,groups ,all-groups ,hl)       
       (setq vertico--input (cons content pt)
             vertico--index index
             vertico--base base
             vertico--total total
             vertico--highlight-function hl
             vertico--groups groups
             vertico--all-groups all-groups
             vertico--candidates candidates
             vertico--default-missing def-missing
             vertico--metadata metadata)
       ;; If the current index is nil, compute new index. Select the prompt:
       ;; * If there are no candidates
       ;; * If the default is missing from the candidate list.
       ;; * For matching content, as long as the full content after the boundary is empty,
       ;;   including content after point.
       (unless vertico--index
         (setq vertico--lock-candidate nil
               vertico--index
               (if (or vertico--default-missing
                       (= 0 vertico--total)
                       (and (= (length base) (length content))
                            (test-completion content minibuffer-completion-table
                                             minibuffer-completion-predicate)))
                   -1 0)))))))

@minad
Copy link
Owner

minad commented May 11, 2022

Okay, so is this a regression in pgtk, Emacs 29? If yes, you may open a bug report via report-emacs-bug.

On the other hand, input-pending-p is documented to return t in case of doubt. I find this pretty much unacceptable, do we have some Heisenberg input in Emacs or what? This means that while-no-input (which relies on input-pending-p) and input-pending-p fundamentally cannot be trusted. One could remove the input-pending-p check from while-no-input:

(defmacro my-while-no-input (&rest body)
  (declare (debug t) (indent 0))
  (let ((catch-sym (make-symbol "input")))
    `(with-local-quit
       (catch ',catch-sym
	 (let ((throw-on-input ',catch-sym)
               val)
           (setq val (progn ,@body))
           (cond
            ((eq quit-flag throw-on-input)
             (setq quit-flag nil)
             t)
            (quit-flag
             nil)
            (t val)))))))

The other alternative would be to introduce a timer which makes sure that the update is enforced after a certain time if no actual input arrived in time. This has been suggested before in other situations, but I would prefer if we can stick to the simpler logic, which we have now. Note that the Emacs builtin icomplete.el package is necessarily affected of the same issue since it also uses while-no-input. I don't plan to add workarounds here as long as Icomplete is not making similar adjustments. But hopefully the bug can be fixed at the root. As I've read, pgtk is unstable, loses events, has broken child frames etc. Therefore it is not yet ready. The maintainer himself recommends against using it.

@vijaymarupudi
Copy link
Author

I just want to clarify, I understand that pgtk is not stable, I'm trying to help them fix the bug so that it becomes stable! I greatly value the time you're spending on this issue!

This is weird behavior by Emacs indeed. The macro you posted did not solve the issue. throw-on-input seems to face the same issue as input-pending-p.

Here is a minimal reproduction

(require 'cl-lib)

(defun my-hook ()
  (message "%S, %S" (minibuffer-contents)
           (while-no-input
             (cl-loop for i from 0 below 1000000
                      summing i))))

(minibuffer-with-setup-hook (lambda ()
                              (add-hook 'post-command-hook
                                        'my-hook
                                        nil t))
  (read-from-minibuffer "Text: "))

I have reported this as a bug, will update this issue when it's relevant.

@minad
Copy link
Owner

minad commented May 11, 2022

I just want to clarify, I understand that pgtk is not stable, I'm trying to help them fix the bug so that it becomes stable! I greatly value the time you're spending on this issue!

Very good then! I am happy to help where I can. But I don't have much free time for such things unfortunately. I am also using Emacs, not only developing packages, therefore I am always lagging a bit behind (for example I migrated to 28 just around the time where the first release candidate was tagged) 😄

@vijaymarupudi
Copy link
Author

Tracking the bug here: https://lists.gnu.org/archive/html/bug-gnu-emacs/2022-05/msg00844.html with identifier bug#55368

You are doing a lot already! I'll take care of this myself as much as I can.

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

No branches or pull requests

2 participants