search

emacs-lsp / lsp-ui

UI integrations for lsp-mode
https://emacs-lsp.github.io/lsp-ui
GNU General Public License v3.0
1.03k stars 141 forks source link

Fixes for lsp-ui-doc-glance #711

Closed Lenbok closed 1 year ago

Lenbok commented 2 years ago

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:

Fixes #713 Fixes #695

kiennq commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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):

kiennq commented 2 years ago

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? 614a55c2-f6c0-4c45-95a4-3ebbc960f137

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 commented 2 years ago

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

Lenbok commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Separate PR created from the first commit on this branch.

Lenbok commented 2 years ago

@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:

(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 commented 2 years ago

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

Lenbok commented 1 year ago

Anyone able to review?

brotzeit commented 1 year ago

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

Thanks @brotzeit !

kiennq commented 1 year ago

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 79ab2167-e8cf-48b2-9455-1470c43be3aa

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

Sorry...

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