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

Quitting a completing-read performed in the middle of a completion will break Company #1408

Closed
vemv opened this issue Oct 6, 2023 · 7 comments

Comments

@vemv
Copy link

vemv commented Oct 6, 2023

CIDER can perform a (completing-read) while a Company popup is being shown. That is accomplished here:

https://github.com/clojure-emacs/cider/blob/5edfbc0bedef4c751f2e753477eb3ca4986bf0f0/cider-client.el#L484

Here's a sample of it working:

image

What it accomplishes is to disambiguate the candidate, as it may come from different Java classes. This way, Company will show the right documentation.

This techique works, except if the user aborts with C-g. In that case, completion will no longer work until the buffer is closed and opened again.

I will see in *messages*: Error in post-command-hook (company-post-command): (quit)

It's as if some internal state wasn't cleaned up afterwards?

Thanks - V

@vemv vemv changed the title Quitting a completing-read performed in the middle of of completion will break Company Quitting a completing-read performed in the middle of a completion will break Company Oct 6, 2023
@dgutov
Copy link
Member

dgutov commented Oct 6, 2023

Sounds like this will need a condition-case to catch the quit signal. Around doc-buffer, right? This doesn't happen for meta?

@vemv
Copy link
Author

vemv commented Oct 6, 2023

Around doc-buffer, right?

Maybe, if you provide a patch or branch I'll happily try it out

This doesn't happen for meta?

What does that mean?

@dgutov
Copy link
Member

dgutov commented Oct 6, 2023

What does that mean?

Could this happen in the :company-docsig handler as well?

Maybe, if you provide a patch or branch I'll happily try it out

This might suffice:

diff --git a/company.el b/company.el
index a650cbc..5d732e2 100644
--- a/company.el
+++ b/company.el
@@ -2232,7 +2232,8 @@ For more details see `company-insertion-on-trigger' and
                 (when company-auto-update-doc
                   (condition-case nil
                       (company-show-doc-buffer)
-                    (user-error nil))))
+                    (user-error nil)
+                    (quit nil))))
             (let ((delay (company--idle-delay)))
              (and (numberp delay)
                   (not defining-kbd-macro)

@vemv
Copy link
Author

vemv commented Oct 6, 2023

Thanks much for the prompt response!

I can confirm that the patch fixes it - after applying it, I can proceed after C-g. No other changes were needed.

Could this happen in the :company-docsig handler as well?

Not that I could see. company-echo-metadata-frontend kept working.

@dgutov
Copy link
Member

dgutov commented Oct 6, 2023

I can confirm that the patch fixes it - after applying it, I can proceed after C-g. No other changes were needed.

Thanks for testing, I can apply and push it now.

Something else to consider: do you want these prompts? CIDER could consult some variable (e.g. non-essential) and skip the prompt when the doc-buffer display is initiated not by a user command, but by the automatic post-command action. Then it would either choose the first alternative among the set, or concatenate all of the possible docstrings in the buffer.

@vemv
Copy link
Author

vemv commented Oct 6, 2023

Cheers.

I'm pretty confident that we want these prompts. These Java docstrings can be wildly long and different across classes, so a guess can misinform the user, and a concatenation wouldn't be quite readable.

I appreciate the concern though. I created clojure-emacs/cider#3501 for making the experience more streamlined (only for the particular Company + IDO combination though) - feel free to check it out.

@dgutov
Copy link
Member

dgutov commented Oct 6, 2023

These Java docstrings can be wildly long and different across classes, so a guess can misinform the user, and a concatenation wouldn't be quite readable.

Makes sense. But in the linked issue you're referring to the same problem (flooring <down> being made impossible, or C-s, or etc).

What I'm suggesting is for cider to display the first alternative anyway, but you would be able to press C-h explicitly, then see the prompt and choose the right class. This is slightly less obvious for the user (not seeing the prompt originally), but it could OTOH work across completion systems, and you wouldn't worry about which bindings need the override.

Anyway, I'll follow the other discussion, let's see what you settle on.

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