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 139 forks source link

lsp-ui-doc: make cursor shows on keyboard focus #504

Closed kiennq closed 3 years ago

kiennq commented 4 years ago

Fix issue mentioned in https://github.com/emacs-lsp/lsp-ui/issues/414#issuecomment-691512719

kiennq commented 4 years ago

@sebastiencs

sebastiencs commented 4 years ago

Thanks @kiennq, I think removing this line fixes your issue: https://github.com/emacs-lsp/lsp-ui/blob/7873160d3ad33081bf6dc25a68430e83133d880e/lsp-ui-doc.el#L804

I don't remember why I added it and it seems that removing it doesn't cause undesired behavior. It's a simpler solution that modifying the parameter cursor-type as you propose.

Can you confirm ?

kiennq commented 4 years ago

Well, remove the frame parameter at that time also fine. But when you focus the doc frame by mouse, the cursor will show up, having the cursor-type there will help hiding that. Why using keyboard to navigate doc frame need cursor, using mouse doesn't require that (rather should not display text cursor in that case).

sebastiencs commented 4 years ago

You are right, modifying the frame parameter gives us more control. I will merge the PR. Can you just reset the variable (not frame parameter) cursor-type to nil in lsp-ui-doc-unfocus-frame.

As a side note, it should not be necessary anymore to click with the mouse on the frame to be able to scroll it. Just moving the mouse pointer over the frame, and using the mouse wheel will scroll the documentation. (Without any click). Can you observe the same behavior ? It depends on the windows manager and I didn't get any feedback about that change

kiennq commented 4 years ago

Can you just reset the variable (not frame parameter) cursor-type to nil in lsp-ui-doc-unfocus-frame.

While it requires both local variable cursor-type and frame parameter cursor-type to showing the cursor, just setting frame parameter one to nil only will hide it. That's why the cursor is not showing up in the old code.

I think we can just stop assigning local cursor-type, to save a few CPU cycles. But that can be done later. For now I reset the variable as you suggested.

As a side note, it should not be necessary anymore to click with the mouse on the frame to be able to scroll it. Just moving the mouse pointer over the frame, and using the mouse wheel will scroll the documentation. (Without any click). Can you observe the same behavior ? It depends on the windows manager and I didn't get any feedback about that change

No, I didn't observer that behavior. I'm on Windows though.

Another thing is that the lsp-ui-doc--no-focus is reset only when lsp-ui-doc--from-mouse, is that okay? Does that affect the case where I focus on doc frame once and unfocus it (by keyboard), then later whenever doc frame show up, mouse will be captured inside it?

sebastiencs commented 3 years ago

I think we can just stop assigning local cursor-type, to save a few CPU cycles. But that can be done later.

Agreed, if setting the frame parameter is enough there is no need to set the variable.

No, I didn't observer that behavior. I'm on Windows though.

Ah, windows is a bit particular I guess

Another thing is that the lsp-ui-doc--no-focus is reset only when lsp-ui-doc--from-mouse, is that okay?

mmh I don't think it's intended. It shouldn't depend on lsp-ui-doc--from-mouse. Can you change that as well ?

kiennq commented 3 years ago

Can you change that as well ?

Done

sebastiencs commented 3 years ago

@kiennq I just test your branch and I still see the cursor after lsp-ui-doc-unfocus-frame, or just by scrolling with the mouse wheel. Looks like the variable cursor-type is still necessary

kiennq commented 3 years ago

@sebastiencs Can you check it with latest one? The cursor-type is not set to t in doc frame anymore

sebastiencs commented 3 years ago

Thanks @kiennq, it's working now