Fixes for lsp-ui-doc-glance

[Lenbok]

Basically lsp-ui-doc-glance was not behaving as expected, mostly due to a bunch of interacting edge cases.

For example: When using inline docs, lsp-ui-doc--glance-hide-frame was conditioned on (overlayp lsp-ui-doc--inline-ov), however due to overlay re-use that test always succeeds after the first doc is shown. So, when lsp-ui-doc-glance was used for a second time (or actually after any lsp-ui-doc had been shown previously), the post-command-hook was getting cleared before the document had even been displayed (since the request was sent asynchronously, but the post-command-hook executes right after lsp-ui-doc-glance returns).

For example: lsp-ui-doc--glance-hide-frame was only activated when the doc is visible. When you keyboard-quit the doc would hide without this being triggered, leaving the function hanging around on post-command-hook ready to nuke the next doc (whether that was invoked via lsp-ui-doc-show or mouse or cursor hover). (childframe and webkit only, but only due to the first bug above)

For example: invoking lsp-ui-doc-glance followed by lsp-ui-doc-show would hide the doc (childframe and webkit only, but only due to the first bug above)

I suspect also bugs related to unfocus frame when mixing glance vs show due to unfocus timer handling not being consistent across the both.

This PR:

• Makes lsp-ui-doc-show asynchronous, preventing emacs blocking.
• It makes lsp-ui-doc-glance equivalent to calling lsp-ui-doc-show plus asking for the doc to hide on the next command, meaning lsp-ui-doc-glance now appears immediately rather than having to wait for lsp-ui-doc-delay first.
• Since document display is asynchronous, we move the addition of next-command triggered hiding to after the doc is actually displayed.
• Unfocus frame timer handling moved to lsp-ui-doc--hide-frame so it affects all display methods, not just glance.
• The removal of the hide frame call from post-command-hook now happens when the doc actually hides rather than relying on special visibility tests.
• A couple of other typos / documentation fixes and improvements to re-use pre-existing functions for clearing timers.

Fixes #713
Fixes #695

[kiennq]

For some inexplicable reason, lsp-ui-doc-show calls lsp-ui-doc--callback synchronously, while lsp-ui-doc-glance went via a more tortuous asynchronous route (via lsp-ui-doc--make-request).

The async route is recommended since the sync call will hang Emacs if the request takes too much time.
The timer is needed since there may be a timer left from before when we try to hide the doc frame, but with a new request to show doc again, the left-over timer will take over and hide the doc frame immediately.
We want to avoid that and have to terminate any left over timer before starting showing doc on-demand.

Can you test with (setq lsp-ui-doc-show-with-cursor nil), trigger lsp-ui-doc-glance, focus on the doc frame then focus back to the document to see if the doc frame is dismissed automatically?

[Lenbok]

The async route is recommended since the sync call will hang Emacs if the request takes too much time.

OK, so rather than make lsp-ui-doc-glance synchronous, we should make lsp-ui-doc-show asynchronous? I had thought that the async aspect may have been to do with the mouse and cursor activation being triggered after delay, whereas lsp-ui-doc-show (and lsp-ui-doc-glance) are triggered by direct user interaction.

Can you test with (setq lsp-ui-doc-show-with-cursor nil), trigger lsp-ui-doc-glance, focus on the doc frame then focus back to the document to see if the doc frame is dismissed automatically?

I use TTY emacs, where neither childframe nor webkit work. Does the overlay version even have the concept of focus? (I can't work out how to set the focus there if it does)

[kiennq]

The async route is recommended since the sync call will hang Emacs if the request takes too much time.

OK, so rather than make lsp-ui-doc-glance synchronous, we should make lsp-ui-doc-show asynchronous? I had thought that the async aspect may have been to do with the mouse and cursor activation being triggered after delay, whereas lsp-ui-doc-show (and lsp-ui-doc-glance) are triggered by direct user interaction.

I think they serve different purpose with glance as a asynchronous version of lsp-ui-doc-show that less punishing even when the user invokes it by themselves.

Can you test with (setq lsp-ui-doc-show-with-cursor nil), trigger lsp-ui-doc-glance, focus on the doc frame then focus back to the document to see if the doc frame is dismissed automatically?

I use TTY emacs, where neither childframe nor webkit work. Does the overlay version even have the concept of focus? (I can't work out how to set the focus there if it does)

The overlay seems doesn't support focus so I'm not sure how to test on that as well. Can you test with GUI Emacs instead?

[Lenbok]

I think they serve different purpose with glance as a asynchronous version of lsp-ui-doc-show that less punishing even when the user invokes it by themselves.

I do not agree -- the only difference AFAIK between lsp-ui-doc-show and lsp-ui-doc-glance is that the latter will auto-dismiss after next (out of focus) command? Since they're both explicitly initiated by the user they have the same level of "punishing". It just makes the code harder to follow if they take different paths for no clear reason.

I have experimented some more using the code at current master (370022b), putting some message statements into when lsp-ui-doc--glance-hide-frame is called, and also when the code makes it past the overlayp / lsp-ui-doc--frame-visible-p condition...

When using inline display (TTY), the first time glance is invoked upon starting emacs, it auto hides when expected (although the hide frame function is called immediately, it does not proceed past the condition). But from then on, it triggers auto hide immediately (when post-command-hook runs), before the async documentation retrieval has even returned the doc content! When the async retrieval comes back, the documentation then shows and never hides (since the hook has already been cleared by then). This is the behaviour I observed when I reported #695

When using childframe (GUI), the auto hiding seems to work every time.

This suggests that the condition in:
https://github.com/emacs-lsp/lsp-ui/blob/master/lsp-ui-doc.el#L1170-L1171
is not working for inline docs. Looking now it seems due to lsp-ui-doc--inline-ov not being reset in lsp-ui-doc--hide-frame. So a minimal fix to #695 is to address that.

However, in terms of testing the focus behaviour with childframe (still on master), I'm not sure how you are normally supposed to get focus into the docs from lsp-ui-doc-glance though, as moving the mouse triggers the window to hide. If I try M-x the window hides. If I directly bind a key to lsp-ui-doc-focus and try that after bringing up the docs with glance, the docs hide. So it seems to me master is already broken in terms of handling doc focus from lsp-ui-doc-glance.

I did find another couple of bugs when using childframe (they might need testing in a minimal emacs environment):

• if the mouse pointer happens to be in the area where the child frame is to appear, lsp-ui-doc-glance never shows the doc at all.
• when you bring up the docs via lsp-ui-doc-show, then set focus to the docs, then M-x the UI locks up.

[kiennq]

I do not agree -- the only difference AFAIK between lsp-ui-doc-show and lsp-ui-doc-glance is that the latter will auto-dismiss after next (out of focus) command? Since they're both explicitly initiated by the user they have the same level of "punishing". It just makes the code harder to follow if they take different paths for no clear reason.

I think the difference here is that the lsp-ui-doc-glance is async while lsp-ui-doc-show is locking Emacs.
Locking up editor on any occasion is never a good thing so I'm in favor of the current implementation for lsp-ui-doc-glance, which follows the one that runs when you set lsp-ui-doc-show-with-cursor to t.

Looking now it seems due to lsp-ui-doc--inline-ov not being reset in lsp-ui-doc--hide-frame. So a minimal fix to #695 is to address that.

The new fix is to reset the overlay, which can be a perf problem since we have to recreate a new overlay every time.
Is the proposed fix not working on your end? It does working in my test env though.

However, in terms of testing the focus behaviour with childframe (still on master), I'm not sure how you are normally supposed to get focus into the docs from lsp-ui-doc-glance though, as moving the mouse triggers the window to hide. If I try M-x the window hides. If I directly bind a key to lsp-ui-doc-focus and try that after bringing up the docs with glance, the docs hide. So it seems to me master is already broken in terms of handling doc focus from lsp-ui-doc-glance.

I did find another couple of bugs when using childframe (they might need testing in a minimal emacs environment):

• if the mouse pointer happens to be in the area where the child frame is to appear, lsp-ui-doc-glance never shows the doc at all.
• when you bring up the docs via lsp-ui-doc-show, then set focus to the docs, then M-x the UI locks up.

I bind lsp-ui-doc-focus-frame to focus onto the child frame.
Your experience is very different from mine, what's your env?

The problem with the mouse pointer in the area where the child frame is to appear happens for me as well, I guess that's one of the quirks of mixing mouse + keyboard input with child frame.
We do have PR for switching to use posframe but it seems not yet merged. @ericdallo
You can use the mouse to trigger the docs to show and then move to focus onto the doc frame, the code path is very similar to what lsp-ui-doc-glance do, both use lsp-ui-doc--make-request

[ericdallo]

@kiennq yeah, that PR is really old and stale, but it still makes sense, feel free to re-try from master

[Lenbok]

@kiennq

The new fix is to reset the overlay, which can be a perf problem since we have to recreate a new overlay every time.
Is the proposed fix not working on your end? It does working in my test env though.

I had actually missed your proposed fix before I made mine. I think it's speculation as to whether there is any performance difference either way (moving an overlay may be more expensive in some cases than creating a new one), but took your suggestion anyway, as it allowed a couple of other small tidy-ups.

I also went back to part of my first version in making lsp-ui-doc-glance go via lsp-ui-doc-show -- this is because the original version would only display after lsp-ui-doc-delay, which doesn't make any sense. For safety and clarity (and correctness if called when the doc is already showing) I also ensured the hide function is only added to post-command-hook after the current command finishes.

[kiennq]

I also went back to part of my first version in making lsp-ui-doc-glance go via lsp-ui-doc-show -- this is because the original version would only display after lsp-ui-doc-delay, which doesn't make any sense. For safety and clarity (and correctness if called when the doc is already showing) I also ensured the hide function is only added to post-command-hook after the current command finishes.

Thank you @Lenbok for working on addressing the comments.

Can we bind the lsp-ui-doc-delay to 0 instead of going to lsp-ui-doc-show?
Same argument as before, hanging Emacs while waiting for a glance document seems not good behavior.

[Lenbok]

Can we bind the lsp-ui-doc-delay to 0 instead of going to lsp-ui-doc-show?
Same argument as before, hanging Emacs while waiting for a glance document seems not good behavior.

I really don't like that idea. I think it is significantly clearer with the code the way it is here (just look at all the shenanigans lsp-ui-doc--make-request does that is completely irrelevant to lsp-ui-doc-glance and lsp-ui-doc-show), and this is not any worse than what lsp-ui-doc-show is already doing.

If you want to make lsp-ui-doc-show async, perhaps that could be a separate PR.

[kiennq]

If you want to make lsp-ui-doc-show async, perhaps that could be a separate PR.

It's already async before this PR.
I think we should make incremental changes, not intentional regression like this.

[Lenbok]

Let's reword that. They are both asynchronous under the hood (that's what lsp-request actually does) so they are interruptible rather than "hanging emacs" - it's just that they both show the documentation immediately as part of the command. Which is exactly what the user would expect for both of those commands. So it's not a regression, it's an improvement.

(Whereas mouse and cursor hovers can appear while the user is generally moving/mousing around and is likely to continue moving/mousing. It's a totally different use case and makes sense for those to be a different code path).

[kiennq]

Let's reword that. They are both asynchronous under the hood (that's what lsp-request actually does) so they are interruptible rather than "hanging emacs" - it's just that they both show the documentation immediately as part of the command. Which is exactly what the user would expect for both of those commands.

lsp-request is waiting for results from LSP server, while blocking Emacs from inputting and redisplay, overall, it makes Emacs hang. You can interrupt it with Ctrl+G but it may cause the internal not properly cleaned up.
If you want the sync version that can be interrupted by input, lsp-request-while-no-input is the way to go.

On the other hand, the part about showing documentation immediately can be solved by binding lsp-ui-doc-delay to 0 when calling lsp-ui-doc--make-request. It doesn't block Emacs input and redisplay, that means even with language server takes a long time to return the docs, Emacs will not go into hang state where you can't move cursor, switch windows. etc...

[Lenbok]

lsp-request is waiting for results from LSP server, while blocking Emacs from inputting and redisplay, overall, it makes Emacs hang. You can interrupt it with Ctrl+G but it may cause the internal not properly cleaned up.
If you want the sync version that can be interrupted by input, lsp-request-while-no-input is the way to go.

We don't want it to be interrupted by input - if the user has initiated either of these two functions they expect to get the result back right then and aren't planning on doing anything else in the interim. In my experience showing the doc takes no appreciable time. If they change their mind, there is C-g. If the lsp server takes too long, there's already a timeout. You're basically trying to argue that no interactive function in the whole lsp-mode/lsp-ui code base should directly call lsp-request. That seems like nonsense to me, and looking at the code base it's also not true, I can see dozens of instances that follow this pattern.

[kiennq]

We don't want it to be interrupted by input - if the user has initiated either of these two functions they expect to get the result back right then and aren't planning on doing anything else in the interim

I don't agree with this.
And actually, that is not related to the problem you want to fix in this bug.

If you think the new behavior is better, please make a new PR for that while keeping the minimal fix for this problem.
That would be better to move the discussion and have this ready for merging.

[Lenbok]

Separate PR created from the first commit on this branch.

[Lenbok]

@kiennq Please re-review this PR.

PR description at the top has been updated. This new strategy addresses several interrelated issues, meaning it now makes sense to keep in a single PR. Previous review comments are addressed:

• All doc showing is asynchronous
• Overlays are re-used.

(It does not address the other possible performance issue we discovered related to inline + display-line-numbers-mode, the impact was not always present in our benchmarking, only small on a per-doc basis anyway, and can easily be done separately if desired)

I have tested inline and childframe docs every way I can think of and have not found anything undesirable.

[Lenbok]

@kiennq would you (or anyone else) be able to review?

[Lenbok]

Anyone able to review?

[brotzeit]

I've tested it and it seems to work as expected. Sorry for the delay but lsp-ui can be really annoying with bugs...

[Lenbok]

Thanks @brotzeit !

[kiennq]

This PR seems to break the auto dismiss of the doc frame after it has been focused in (for scrolling and reading on the docs) and out.
I've bond tab to lsp-ui-doc-focus-frame and q to lsp-ui-doc-unfocus-frame.
Here is the current behavior

As you can see, it's broken and I have to press Tab twice for the focus to be moved to the doc frame.
@Lenbok @brotzeit

[brotzeit]

Sorry...

That's the reason why I'm so hesitant to merge pull requests. I'm always missing something when I'm testing changes.