Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
kiennq opened this issue Aug 7, 2022 · 8 comments · Fixed by #729 · May be fixed by #727
Closed

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

kiennq opened this issue Aug 7, 2022 · 8 comments · Fixed by #729 · May be fixed by #727

Comments

@kiennq
Copy link
Member

kiennq commented Aug 7, 2022

Seems to be regressed by #711, see #711 (comment)

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
Copy link
Contributor

Lenbok commented Aug 7, 2022

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
Copy link
Contributor

Lenbok commented Aug 7, 2022

(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
Copy link
Member Author

kiennq commented Aug 7, 2022

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
Copy link
Contributor

Lenbok commented Aug 7, 2022

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
Copy link
Member Author

kiennq commented Aug 7, 2022

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
Copy link
Member Author

kiennq commented Aug 7, 2022

(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
Copy link
Contributor

Lenbok commented Aug 8, 2022

@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
Copy link
Member Author

kiennq commented Aug 9, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants