joaotavora / sly

Sylvester the Cat's Common Lisp IDE
1.26k stars 142 forks source link

Fix that the company overlay does not disappear frequently when typing #468

Open Dima-369 opened 2 years ago

Dima-369 commented 2 years ago

First of all I have tested this in slime and it also shows the same behavior apparently, so this might not be sly/slime related but maybe even a company issue?

Nonetheless, I find it quite annoying when typing quickly as I keep thinking that there are no candidates anymore when the popup disappears but then it appears again which can happen multiple times on a long word.

Here is a video (right side shows Emacs Lisp where it works but I suspect it works so well because the is no async capf function):

https://user-images.githubusercontent.com/15002298/136171095-f3dfa7cd-43e5-4bbe-9dc2-e04cee828dc9.mov


Does anyone know of a fix for this? I would expect the company popup to stay open and only disappear once there are no candidates anymore. I don't see a problem with the company popup even showing the old candidates until updated.

joaotavora commented 2 years ago

I don't see anything wrong with your example: it works just like that by design. Explanation: with Emacs lisp, the completions are very close by to the display facility (which is company), thus we are able to show them faster and without flickering. With Common Lisp they are not as close, they are computed in aa asynchronous process and shot back to Emacs via a process pipe, which is much slower than accessing memory. If the input is still valid by the time they arrive, the company tooltip shows.

To get rid of the "flickering", one would have to block user input, so you wouldn't be able to type "defparameter" as fast as you do.

BTW, I'm very curious of your claim about SLIME showing the same behaviour as SLY. Can you post a second video showing the same comparison between SLY and SLIME (this is just for my curiosity, it has nothing to do with the issue).

joaotavora commented 2 years ago

I don't see a problem with the company popup even showing the old candidates until updated.

Just caught this sentence here which I hadn't paid attention to. I do see a problem with that: it's misleading information right? But maybe you can figure out some knob which lets you opt into this behaviour. I don't have much time to dig in right now, sorry :-/

Dima-369 commented 2 years ago

BTW, I'm very curious of your claim about SLIME showing the same behaviour as SLY. Can you post a second video showing the same comparison between SLY and SLIME (this is just for my curiosity, it has nothing to do with the issue).

Aha, I misremembered! The company popup does not disappear except when typing out a full word and then backspacing (whereas in Emacs Lisp the popup stays as seen in the first video):

Also excuse the lengthy video, I was a bit baffled that it worked so I tried it many times 😉

https://user-images.githubusercontent.com/15002298/136176238-64c0260b-a17d-4dd6-a3e9-72ba9b1b4d6e.mov

I do see a problem with that: it's misleading information right?

I am used to Jetbrains IDEs and from my feeling the completion popup there behaves the same with it showing the old popup data but it usually updates so quickly that it is absolutely no issue. But then again, I think that Jetbrains IDEs run the completion in the same process and index the entire project beforehand (like LSP) so it might be 'easier' to quickly return completion candidates (like for Emacs Lisp).

If I could choose between a persistent popup showing outdated information sometimes and a popup which disappears often, I'd take the persistent popup, but that's just me :)

I don't have much time to dig in right now, sorry :-/

Hey, no problem, Sly is already very good, thank you for what you have done so far :)

joaotavora commented 2 years ago

You'll notice that SLIME offers many less completions than SLY. SLIME is using basic prefix completion, whereas SLY is using "flex" completion, which searches for all candidates that have the pattern as a subsequence. You can downgrade SLY to prefix completion, if you want. Have a look at the manual, or at the code, I don't know that by heart.

monnier commented 2 years ago

If I could choose between a persistent popup showing outdated information sometimes and a popup which disappears often, I'd take the persistent popup, but that's just me :)

IIUC this is largely an issue with Company rather than with SLY/SLIME, so please mention it there. You might also suggest finding a compromise, such as greying out the completions when they're out of date (instead of removing the popup altogether).

    Stefan
Dima-369 commented 2 years ago

Created an issue: https://github.com/company-mode/company-mode/issues/1235

dgutov commented 2 years ago

Hi all!

IIUC the problem is the following:

This means you have to type the next char (it's already been typed, okay: we're in this situation because of new input) and wait for company-idle-delay seconds for the popup to show up again.

When completion responds faster than the user types, it's not a problem. When completion is slower but uses built-in cache (meaning, prefix matching and not company-capf) -- not a problem either. But it is a problem with disabled caching and slow enough servers. Or faster typists.

If we had a proper async convention in place for capf API, we could deal with interrupting user input in some more constructive fashion.

As it is, though, try what lsp-mode folks did (they also use the "faux async" approach, due to having to go through capf too): when the user input interrupts completion response, return something non-nil: perhaps the previous completion result.

Relevant discussions:

https://github.com/emacs-lsp/lsp-mode/issues/1894 https://github.com/emacs-lsp/lsp-mode/pull/2148 https://github.com/emacs-lsp/lsp-mode/issues/2202

joaotavora commented 2 years ago

As it is, though, try what lsp-mode folks did (they also use the "faux async" approach, due to having to go through capf too): when the user input interrupts completion response, return something non-nil: perhaps the previous completion result.

"Something non-nil" sounds suboptimal. IMO we should search for ways to let company know that things have been interrupted and we're searching for a new set, and then company would do whatever is appropriate to keep the overlay visible but maybe greying it out a little, like Stefan suggested.

But that's a bit strange, too. I'm still missing informatino: doesn't company already know that "the previous completion has been interrupted and were searching for a new set"?? You seem to be aware of this fact when you write "it's already been typed, okay: we're in this situation because of new input".

So I don't see why company can't remember the last "visual" thing that it displayed and then, instead of removing it from the screen in the company-cancel branch, keep it around greyed out. Or briefly remove it and then quickly resurrect it, again greyed out, while waiting for the results of the next search.

dgutov commented 2 years ago

"Something non-nil" sounds suboptimal.

Just like the whole faux async story. But it works. On the flip side, you get to decide the list of completions the popup will show after such interrupted computation.

IMO we should search for ways to let company know that things have been interrupted and we're searching for a new set, and then company would do whatever is appropriate to keep the overlay visible but maybe greying it out a little, like Stefan suggested.

That's what I have been talking about throughout the recent years when saying we need a proper asynchronous completion API. But the completion backend shouldn't decide whether it's been "interrupted" either. And whether "we're searching for a new set" or not, is also not its decision.

You seem to be aware of this fact when you write "it's already been typed, okay: we're in this situation because of new input".

I'm aware of it because I know the whole scenario (it's described in the first comment, with a video and all). The framework doesn't have all this information.

We could add some sort of additional heuristic like "if input is pending when backend returns its completions, then prooobably it has been interrupted and the completions are not valid". Together with a nil check, it might even work okay 98% of the time, but on the flip side some fast typists will experience the popup staying around longer than it should.

So it's a band-aid solution as well, and even less precise one than "return previous completions".

So I don't see why company can't remember the last "visual" thing that it displayed and then, instead of removing it from the screen in the company-cancel branch, keep it around greyed out.

We're basically doing this since https://github.com/company-mode/company-mode/commit/ba67321645547362525d7930d6c05442ea48ac4f.

Except for the greying out part: I don't want to cause any seizures.

joaotavora commented 2 years ago

"Something non-nil" sounds suboptimal.

Just like the whole faux async story.

(Sidenote: I don't know if you just like to type French words or make gratutious acrymony. Either way there's nothing "fake" in this scheme, it's simply an an imperative interface to an asynchronous mechanism. I don't call your API-revamping ideas "conard API", for one)

But it works. On the flip side, you get to decide the list of completions the popup will show after such interrupted computation.

I don't see any need to do that decision.

IMO we should search for ways to let company know that things have been interrupted and we're searching for a new set, and then company would do whatever is appropriate to keep the overlay visible but maybe greying it out a little, like Stefan suggested.

That's what I have been talking about throughout the recent years when saying we need a proper asynchronous completion API.

If overhauling the whole API is the only solution you can think of for the problem of two pieces of software communicating a binary value, I think company is going to live with this problem for a long time.

But the completion backend shouldn't decide whether it's been "interrupted" either.

I don't see any extreme harm in it knowing that, but it doesn't have to know, it can just call a function that abstracts that detail away. In the Eglot case jsonrpc-request is that function.

Here's an idea that doesn't require revamping APIs.

What if, along with returning nil the table (or, if you prefer, the async abstraction being aware of interruptions), set a value somewhere that company or other frontends could consult and more easily decide if there had been an interruption.

We could add some sort of additional heuristic like "if input is pending when backend returns its completions, then prooobably it has been interrupted and the completions are not valid". Together with a nil check, it might even work okay 98% of the time, but on the flip side some fast typists will experience the popup staying around longer than it should.

98%? Why not 98.23%? Can you describe the scenario of the fast typists? What would they see exactly? (and anyway 98% sounds pretty good to me).

Except for the greying out part: I don't want to cause any seizures.

Was Stefan's idea. Doesn't seem so silly to me, especially for slow/bloated Common Lisp images.

dgutov commented 2 years ago

(Sidenote: I don't know if you just like to type French words or make gratutious acrymony. Either way there's nothing "fake" in this scheme, it's simply an an imperative interface to an asynchronous mechanism. I don't call your API-revamping ideas "conard API", for one)

French words are fun, but "acrimony" is not too bad either.

I need a phrase to describe this general class of approaches, and you didn't suggest any alternatives. You really won't like any of the full-English alternatives I would choose.

Your "imperative interface" (as opposed to what? functional? declarative?) to async is synchronizing. And opaque. And I can't believe how much time I have wasted already explaining such obvious things.

I don't see any need to do that decision.

I figured deciding things on your own is right up your alley.

If overhauling the whole API is the only solution you can think of for the problem of two pieces of software communicating a binary value, I think company is going to live with this problem for a long time.

Apparently so. An async interface doesn't require a full overhaul, but doing even a localized improvement in that area requires the stakeholders to be able to communicate.

Company doesn't have that problem, though. Eglot does.

What if, along with returning nil the table (or, if you prefer, the async abstraction being aware of interruptions), set a value somewhere that company or other frontends could consult and more easily decide if there had been an interruption.

Who would set it? When would it be cleared? Would it work for multiple completion tables combined into a composite completion table?

Anyway, it doesn't work for me because Company has a mode of operation when we don't want completion calculations to abort on input.

98%? Why not 98.23%?

It's 97.45678%, actually.

Can you describe the scenario of the fast typists?

I don't see the point. Either you figure it out on your own, or you won't listen to my explanation anyway

(and anyway 98% sounds pretty good to me).

The first suggestion results in 100%.

Was Stefan's idea. Doesn't seem so silly to me, especially for slow/bloated Common Lisp images.

You're welcome to try implementing that idea and enjoy the resulting blinking in person.

joaotavora commented 2 years ago

Who would set it?

The async abstraction that you insist on demonizing.

When would it be cleared?

The next capf request with a new string, or a the new call to the async abstraction.

Would it work for multiple completion tables combined into a composite completion table?

No idea, I just program for CAPF, instead of obsessing to replace it to some fancy shmancy "completely obvious" thing.

dgutov commented 2 years ago

The async abtraction that you insist on demonizing.

No such thing.

I just program for CAPF, instead of obsessing to replace it to some fancy shmancy "completely obvious" thing.

I'm referring to a feature that's already more-or-less possible to do with CAPF (the normal, synchronous tables, at least), and which is required to be able to deprecate company-backends.

joaotavora commented 2 years ago

I'm sorry, Dmitry, I don't think this is getting anywhere. If you put your 98% heuristic approach somewhere in a branch, let us know here. Maybe I can test it it to make it 100%. Thanks.

joaotavora commented 2 years ago

On a ligther note, this reminds me of how much better CL is as a language then Emacs Lisp. In CL this would e a signal+a restart. Interested outside observers down stack -- like the completion frontend -- could optionally act on the signal. Dynamic variables can do the same, but not as pretty.

joaotavora commented 2 years ago

Emacs Lisp can throw. No idea what that's going to do to concurrent requests, though.

throw has nothing to do with CL restarts.

Alright. This has gone too far. We need a cool down period.

joaotavora commented 2 years ago

@Gira-X I apologize for the sorry spectacle that the last few interactions. To summarize,

In the very short time being no solution in sight, though.