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

robe-completing-read for Module: is excessively interrupting #144

Open
vemv opened this issue Oct 20, 2023 · 20 comments
Open

robe-completing-read for Module: is excessively interrupting #144

vemv opened this issue Oct 20, 2023 · 20 comments

Comments

@vemv
Copy link

vemv commented Oct 20, 2023

Hi @dgutov ,

I'm using Robe, company-mode and IDO.

I have company-auto-update-doc set to t.

Under that setup, the Module: prompts are too high-friction. While using Company, maybe I intended to <down> my way to method foo, but method bar is before in the Company popup, and Robe asks me to disamgibuate the Module: . I cannot exit that until I C-g or choose an arbitrary module.

What I really want is to press <down> again and continue my way to foo.

The way we solved an analog problem in CIDER was to provide this macro:

https://github.com/clojure-emacs/cider/blob/f85d5c17043a430d0bb730c6637fba52f5a7f94f/cider-client.el#L480-L500

And then one would use:

(advice-add 'robe-completing-read
            :around
            (lambda (f &rest args)
              (cider--with-temporary-ido-keys "<up>" "<down>"
                                              (apply f args))))

(I actually used that CIDER macro as-is with robe-completing-read and it worked!)

I don't have a problem atm with this setup, but I figured it would be good to let you know / document a solution.

Cheers - V

@dgutov
Copy link
Owner

dgutov commented Oct 20, 2023

Hi!

I use Ivy for robe-completing-read-func myself, but otherwise totally understand your frustration: with ivy-posframe the disruption is even more severe. And overriding up/down (or C-n/C-p) wouldn't work even as a hack because those are used to select a completion with Ivy.

I wrote up some a couple of alternative approaches here, as you recall: company-mode/company-mode#1408 (comment)

To summarize:

a. The docstring is showed automatically (not after explicit C-h keypress), it could show the doc for the first of the matching modules. Pressing C-h after that will raise the prompt allowing you to choose a specific one (this will work through the non-essential variable).
b. Concatenate all the possible docstrings in one buffer, allowing the user to page through.

Here's a couple of new variations: c. the buffer text itself will mention that there are several matching with the given name, and you'd be able to page through them by tapping C-h multiple times. There could be a counter (e.g. 4/12) like with method overloads in eldoc. And/or additional key bindings (M-n/M-p would be handy, but I guess not by default) which would go back/forward. Maybe C-M-v/C-S-M-v could do double duty. The main drawback of this approach is no possibility of completion by typing something, only selection.

d. Like a, but when there are several available options, don't show any doc (so as not to create confusion) until the user types C-h explicitly. Then ask which one. This might fit the use of non-essential the best. OTOH, it's less compatible with company-quickhelp.

Looks like the previous time this came up was in company-mode/company-quickhelp#8, which settled on the first part of a.

I also tried to check out what VS Code does in such cases, and apparently Solargraph gives up and shows nothing in cases where it's not certain about the owner class. Which would correspond to d, though a little weaker.

@dgutov
Copy link
Owner

dgutov commented Oct 20, 2023

BTW, your current solution might be implemented slightly differently as well:

(defun my/completing-read-without-up-down (f &rest args)
  (let ((key-translation-map (define-keymap   ; new in Emacs 29
                               "<up>" "C-g"  ; maybe add C-n and C-p as well
                               "<down>" "C-g")))
  (condition-case nil
      (apply f args)
    (quit (car (nth 1 args))))))

(advice-add 'robe-completing-read :around 'my/completing-read-without-up-down)

That should work across completion systems, though with expected disruptive effect for Ivy and other vertical ones.

@vemv
Copy link
Author

vemv commented Oct 21, 2023

(I use Emacs 27 😅)

Thanks much for the summary.

Perhaps a variation would be:

  • If there's a single alternative, render its doc immediately.
  • If there are multiple alternatives, render nothing, up to 1 second (configurable). If 1 second has passed with no input, perform the completing-read.

This way, I can <down> <down> <down> ... without interruptions, but getting the usual experience when I need it.

It also seems a fairly intuituive UX, that requires no learning of extra commands. Users would be able to reuse their existing knowledge of this aspect of Company / completing-read.

Hopefully it wouldn't complicate the implementation code much? For one thing, it probably would involve that the completing-read is performed by Company, not by Robe or CIDER.

The way I'd imagine is by providing something like :company-doc-buffer that was named instead e.g. :company-doc-buffer-alternatives. It would return multiple buffers instead of just one.

It would be a non-breaking API change: backends would be free to provide one or the other key (maybe both, in case the user was running an old Company)


Edit: as part of that would-be design, :company-doc-buffer-alternatives would have to return lazy recipes for doc buffers, not fully-rendered doc buffers. Because e.g. CIDER performs one costly network request per doc buffer.

@dgutov
Copy link
Owner

dgutov commented Oct 21, 2023

(I use Emacs 27 😅)

Ah, okay. Only define-keymap is new in 29. In earlier versions you would use the more manual way to construct the keymap -- (let ((map (make-sparse-keymap)) (define-key map ...) .... That still makes for a shorter implementation.

If there are multiple alternatives, render nothing, up to 1 second (configurable). If 1 second has passed with no input, perform the completing-read.

I'm not in love with this approach myself, for two reasons: a) Ivy's prompt is disruptive for the completion popup, whether it's immediate or after 1 second, b) there is no guarantee that you're not going to want to change selection right after that 1 second timeout fires sometime. The odds of such conflict will be lower, but unexpected problems often feel more annoying than the ones you anticipate.

So I'm reasonably certain you won't like this approach in end after trying it. But maybe I'm wrong. What we could do, probably, is split the change into two parts: changes to Company and Robe which implement d and a customization (or maybe an experimental patch) on top of it that calls doc-buffer over an interval with a timer in a way that causes completing-read to be used. That won't require a new backend command, just setting and using the variable non-essential.

@vemv
Copy link
Author

vemv commented Oct 21, 2023

It sounds reasonable to me.

Although, under the would-be :company-doc-buffer-alternatives approach, given that Company alone would be the responsible for prompting, then we could ditch completing-read entirely, and offer instead a 'native' approach, i.e. use the Company popup itself to let users choose.

Otherwise, mixing Company with <ido|ivy|etc> is not exactly a consistent UI, and even if it can work (it mostly works for me), it feels hacky.

(I'm agnostic as for how Company would offer a 'native' UI. I'm sure that it could evolve / have a few alternatives)

An additional win with :company-doc-buffer-alternatives is that Corfu could also consume it (just like it consumes :company* stuff today), avoiding mixing Corfu with <ido|ivy|etc>. I hear that other CIDER+Corfu users have a completely broken experience if completing-read pops up.

@dgutov
Copy link
Owner

dgutov commented Oct 21, 2023

Although, under the would-be :company-doc-buffer-alternatives approach, given that Company alone would be the responsible for prompting, then we could ditch completing-read entirely, and offer instead a 'native' approach, i.e. use the Company popup itself to let users choose.

That would be an advantage, yes. But one of these solutions I could implement in 10 minutes, and the other... 🙃

How would it look anyway? A pop up beside a pop up (harder)? Or a replacement popup (easier, but a lot more confusing from the user's POV)?

I hear that other CIDER+Corfu users have a completely broken experience if completing-read pops up.

Indeed. Ido/IComplete should be the least confusing of the bunch in this context because of the horizontal rendering.

@vemv
Copy link
Author

vemv commented Oct 21, 2023

How would it look anyway? A pop up beside a pop up (harder)? Or a replacement popup (easier, but a lot more confusing from the user's POV)?

company-active-map currently has <left> and <right> available, correct?

So, if there were multiple options during a completion:

  • Company could say Showing doc for: Foo | Bar | Baz in the echo area
    • (Foo being rendered more prominently, to make it evident that it's the current one)
  • I could press <right>, to indicate that I want Bar's doc instead
  • Company would update the doc buffer to reflect the new selection
  • Company would now say Showing doc for: Bar | Baz | Foo in the echo area

The interface would be vaguely similar to IDO, except that:

  • Hitting RET is not needed anymore
  • It's a non-blocking choice: I can press <right> just like I can press <down> to disregard the whole thing
  • I can retract my choice, and explore other choices
    • i.e. currently one has to make one definitive choice
    • Maybe I don't know in advance what the right choice is? (e.g. I'm exploring a new domain)

The completion popup would not be altered in any way, and it would remain open during the whole process, as usual.

@dgutov
Copy link
Owner

dgutov commented Oct 21, 2023

All right. It does sound nice, for the cases it would work in.

Some problems:

  • The "main" bindings for "select next/previous" are C-n and C-p, not and . So we'd need some accessible touch typing keys as well. Maybe C-f and C-b will work, but that does mean taking over more "normal" buffer navigation bindings. Right now I can exit completion also by pressing C-f (and the arrow keys' users can do that with <right>).
  • Echo area is also where the meta frontends lives. So that means overriding it, it somehow having to combine the outputs.
  • Like with the solutions b and c I suggested, choosing the module using typing will be impossible (unless we add one more binding and some additional UI for textual search?...). From my experience with Ruby, sometimes the lists of modules to choose from will be pretty long (20-30 entries, easily), so I've been often typing the module name in such cases). Just try looking up the doc for each in an empty project: 36 alternatives.

I can retract my choice, and explore other choices
i.e. currently one has to make one definitive choice
Maybe I don't know in advance what the right choice is? (e.g. I'm exploring a new domain)

Note that solutions a and d would also have that advantage. Although your proposed UI would make going between the choices much snappier (when there are not many of them).

@vemv
Copy link
Author

vemv commented Oct 21, 2023

36 alternatives.

A consideration is that Robe completions seem remarkably snappy. Maybe I can go from 1 to 36 just by keeping the key held for a couple seconds?

Other than Ruby, probably there aren't a lot of ecosystems where choices are so abundant.

I can think of the edge case (.toString x) in Clojure which has many completions (basically one per class; .toString is special since it comes from Object). But one can also make it accurate by modifying the code to be (.toString ^Foo x) which is a win.

Probably all ecosystems are like that - they have type hints or some other mechanism that can narrow the choices in advance.

Either way, thanks much for considering my suggestion carefully. If the endgame is that CIDER can get rid of completing-read(simplifying the life for our Company and Corfu users alike), I would not mind about this or that detail.

dgutov added a commit that referenced this issue Oct 21, 2023
Allowing external iteration through method overrides.

#144
dgutov added a commit to company-mode/company-mode that referenced this issue Oct 21, 2023
@dgutov
Copy link
Owner

dgutov commented Oct 21, 2023

Maybe I can go from 1 to 36 just by keeping the key held for a couple seconds?

Most likely, yes. You just wouldn't be able to do what I'd usually done: start typing part of the class name (if you know it yourself already) and press RET before the list is even displayed.

Other than Ruby, probably there aren't a lot of ecosystems where choices are so abundant.
I can think of the edge case (.toString x) in Clojure which has many completions (basically one per class; .toString is special since it comes from Object).

One language that comes to mind is Java :-) But I suppose we only need to consider the dynamic ones, for practical purposes (even though choosing between the subclass methods of a given interface could be useful in some circumstances).

In Clojure you have IFn, for example (with many types that implement invoke), though I'm not sure it's relevant to those not working on the language implementation or stdlib.

Anyway, this got me curious, so here's a thing to try: company-mode/company-mode@7da7869 and 00edb2a.

It looks like this: Screencast from 2023-10-22 02-10-36.webm

These little numbers in the bottom left are the indicator of the multiple available choices. And they're displayed through the metadata frontend.

It's not exactly like Ido, but could be, if we decide on how to display the non-selected method specs. But this section also could be moved to the doc buffer itself (the beginning), or to somewhere in the popup -- then the compactness would have its benefits.

dgutov added a commit to company-mode/company-mode that referenced this issue Oct 22, 2023
@dgutov
Copy link
Owner

dgutov commented Oct 22, 2023

also could be moved to the doc buffer itself (the beginning)

Or just added. Like this:

Screenshot from 2023-10-22 23-02-39

@vemv
Copy link
Author

vemv commented Oct 23, 2023

Hi @dgutov, those are some neat diffs and demo!

It all makes sense to me.

If that helps, I can implement the same would-be interface in CIDER and let you know how it works for that use case.

Are the branches stable as of today?

@dgutov
Copy link
Owner

dgutov commented Oct 23, 2023

(Note that the video is a little outdated, as per the screenshot.)

Glad you like it, and yes, more feedback on the UI and the approach would be good, including from CIDER. I'm more or less sold on the UI myself, but I haven't yet tried using it seriously in any bigger projects. And CIDER may face a peculiarity where the class names (fully qualified) are pretty long, so some shortening method could help.

I can keep the branches stable, no problem. The implementation might actually change a fair bit later, but only after the UI is decided on.

@dgutov
Copy link
Owner

dgutov commented Oct 23, 2023

You can base off the branches candidate-variants in both repos.

@vemv
Copy link
Author

vemv commented Oct 25, 2023

I have this drafted, but it looks we'd need company-capf.el to be adapted as well (since that's the main 'API' used by CIDER).

If it helps, here's how my tentative usage is looking like:

https://github.com/clojure-emacs/cider/compare/company-variants?expand=1

Thanks - V

dgutov added a commit to company-mode/company-mode that referenced this issue Oct 25, 2023
@dgutov
Copy link
Owner

dgutov commented Oct 25, 2023

Right, forgot about that.

Could you try with the change I just pushed?

@vemv
Copy link
Author

vemv commented Oct 25, 2023

Alright, I got it working! I updated the branch above accordingly.

It's looking very nice - thanks, as always.

I haven't implemented an indicator showing the other disambiguation candidates.

Do you have a strong opinion as to whether it's backends like CIDER or Company itself that should do that?

For me, leaving it to Company seems the right thing - Company has the variants + the index, i.e. all the ingredients to render it into the echo area.

Maybe, Company can let users choose:

  • render it to the echo area
    • the "merge" is rather simple: (concat x "\n\n" variants-info)
  • let CIDER/etc render it as part of the doc buffer
    • for this case, I'd suggest that Company passes an already "computed" string e.g. Candidate 1/20 foo | bar | baz
    • this string would be blank if the user indicated that he prefers using the echo area.

My overall thinking if that parts like the CIDER doc buffers should be as 'logicless' as possible - it's not their concern to calculate completion state.

@dgutov
Copy link
Owner

dgutov commented Oct 25, 2023

For me, leaving it to Company seems the right thing - Company has the variants + the index, i.e. all the ingredients to render it into the echo area.

I couldn't find a good way to do that that would work for all users, hence the idea to leave it to the backends (in meta and doc-buffer). It's not a done decision -- but I was hoping to make it a part of the experiment.

Maybe, Company can let users choose:

Sure, but we'd still have to decide on the choices.

render it to the echo area

By default, it's taken by the metadata frontend. But some users disable it (to keep it for Eldoc). So the echo area is not ideal as an everyone's solution.

When the user just wants to disable metadata, we could provide a different frontend without it (which would still show the variants), but avoiding overriding Eldoc's outputs seems more difficult. I haven't looked into this issue properly enough to say how hard it is, though.

the "merge" is rather simple: (concat x "\n\n" variants-info)

And seeing echo area grow past 1 line isn't to everyone's liking as well: e.g. for Eldoc there is a setting eldoc-echo-area-use-multiline-p.

for this case, I'd suggest that Company passes an already "computed" string e.g. Candidate 1/20 foo | bar | baz

That's also a possibility, but for now I'd rather you try implementing this on your side, and try optimizing it for your data, how it looks best. Code reuse shouldn't be a concern for now: you can copy-paste some code from Robe.

this string would be blank if the user indicated that he prefers using the echo area

I take it you didn't like my X/Y metadata frontend approach either?

My overall thinking if that parts like the CIDER doc buffers should be as 'logicless' as possible - it's not their concern to calculate completion state.

I was curious about the other direction as well: for example, in Robe the robe-doc command (outside of completion) might also print in the beginning that the method in question is defined in multiple modules, allowing to switch between them. And that sort of usage might make it more natural to rather move the state management into the backend instead (or not; depending on whether it would be easier to copy the extra 10-20 lines of code).

@dgutov
Copy link
Owner

dgutov commented Oct 25, 2023

Anyway, if you wanted to implement a couple of different variants and present them for a wider discussion, that would be even better.

@vemv
Copy link
Author

vemv commented Nov 13, 2023

I haven't forgotten about this one.

Things are looking reasonable with Robe and CIDER alike - nice work with Robe!

Will give further input asap - my personal backlog is clearing up finally.

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