dgutov / robe

Code navigation, documentation lookup and completion for Ruby
584 stars 37 forks source link

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

Open vemv opened 1 year ago

vemv commented 1 year ago

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 commented 1 year ago

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: https://github.com/company-mode/company-mode/issues/1408#issuecomment-1750509843

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 https://github.com/company-mode/company-quickhelp/issues/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 commented 1 year ago

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 commented 1 year ago

(I use Emacs 27 😅)

Thanks much for the summary.

Perhaps a variation would be:

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 commented 1 year ago

(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 commented 1 year ago

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 commented 1 year ago

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... :upside_down_face:

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 commented 1 year ago

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:

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

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

dgutov commented 1 year ago

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

Some problems:

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 commented 1 year ago

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 commented 1 year ago

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: https://github.com/company-mode/company-mode/commit/7da7869 and https://github.com/dgutov/robe/commit/00edb2aafc6e1eb790b59b3e1ab4a221ae41a222.

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

(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 commented 1 year ago

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

vemv commented 1 year ago

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 commented 1 year ago

Right, forgot about that.

Could you try with the change I just pushed?

vemv commented 1 year ago

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:

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 commented 1 year ago

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 commented 1 year ago

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

vemv commented 1 year ago

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.