search

emacs-lsp / lsp-ui

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

lsp-ui-doc frame is not hidden after the doc frame is unfocused #726

Closed kiennq closed 2 years ago

kiennq commented 2 years ago

Seems to be regressed by #711, see https://github.com/emacs-lsp/lsp-ui/pull/711#issuecomment-1207260887

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

I can't reproduce your problem with needing to press tab twice, maybe it's related to tab being used for auto completion? In my setup I have bound C-z l to lsp-ui-doc-focus-frame and it works fine on the first invocation.

The behavior on focus exit may be have changed, but I don't necessarily think it is a bug. I think that it's perfectly acceptable to cancel the auto-dismiss of the frame if the user has been explicitly interacting with it -- it then acts like a non-glance frame that you can focus in and out of as you wish. If you want q to always close the doc, it would be better to bind it to a function that explicitly does that (although it seems that lsp-ui-doc-hide does not work when the focus is inside the doc frame).

Lenbok commented 2 years ago

(BTW, are you triggering the doc to appear via lsp-ui-doc-glance or is it via auto appearing with mouse or cursor hover?)

kiennq commented 2 years ago

The behavior on focus exit may be have changed, but I don't necessarily think it is a bug. I think that it's perfectly acceptable to cancel the auto-dismiss of the frame if the user has been explicitly interacting with it -- it then acts like a non-glance frame that you can focus in and out of as you wish. If you want q to always close the doc, it would be better to bind it to a function that explicitly does that (although it seems that lsp-ui-doc-hide does not work when the focus is inside the doc frame).

There is already a function for that, lsp-ui-doc-unfocus-frame that will also hide, and dismiss the doc frame before. Your PR changed that behavior with no obvious gain and the workaround is not even available yet.

(BTW, are you triggering the doc to appear via lsp-ui-doc-glance or is it via auto appearing with mouse or cursor hover?)

I trigger it via Shift + K (you can see the key typed).

Lenbok commented 2 years ago

I would not expect lsp-ui-doc-unfocus-frame to also hide the frame (and that's not what its docs say), so how would anyone know if that is considered changed behavior? (Other than the catch-all https://xkcd.com/1172/)

Even so, consistent behavior would be that the next command after lsp-ui-doc-unfocus-frame would dismiss it. I.e. both focus and unfocus actions would not be counted as the "next command" that glance uses to dismiss, and you could focus/unfocus multiple times.

The simple fix (keeping current/documented behavior of unfocus) would seem to be ensure that lsp-ui-doc-hide works from within the doc frame. For the alternative behavior, perhaps a more direct solution would be for lsp-ui-doc-focus-frame to record whether the doc is scheduled for dismissal (by the presence of lsp-ui-doc--hide-frame on the post-command-hook), and if so lsp-ui-doc-unfocus-frame could re-enable it when it returns focus back to the main buffer?

(Your embedded screencast is unclear to me, due to speed, auto-looping with no indication of position, and it does not show what keys are bound to what actions. Do you know of better screencast tools that at least allow control of playback speed?)

kiennq commented 2 years ago

Even so, consistent behavior would be that the next command after lsp-ui-doc-unfocus-frame would dismiss it. I.e. both focus and unfocus actions would not be counted as the "next command" that glance uses to dismiss, and you could focus/unfocus multiple times.

You're correct about the part that you can't hide the frame that you're having focus into, else the focus will move away and you will have no idea where it is. That's also working in our favor prior to your change. Now, they're not counted as "next command"??? That doesn't seem right to me. If they're counted as the next commands, then unfocus should hide the frame (the focus in can't do that so we should hide the frame when possible, unless we cancel it).

kiennq commented 2 years ago

(Your embedded screencast is unclear to me, due to speed, auto-looping with no indication of position, and it does not show what keys are bound to what actions. Do you know of better screencast tools that at least allow control of playback speed?)

That's a gif, I don't know any tool allow that though

Lenbok commented 2 years ago

@kiennq Try #729, it lets you bind to lsp-ui-doc-hide which will let you close the doc when focus is in the doc (regardless of whether it came from glance, so an overall improvement I think).

(Aside, yesterday I found https://github.com/tarsius/keycast which will display both keys and bound actions, and also a log view, which might help make clearer casts)

kiennq commented 2 years ago

Thanks, that change seems working for me. Regardless of that, I wonder why #727 is not working for you. I can't seem to repro the problem. Can you show me some screencasts in your case?

The changes in #711 and #729 move the post command hook all the way to lsp-ui-doc--callback instead of just containing it in functions related to lsp-ui-doc-glance, thus more risks of regression.