Add posframe integration to lsp-ui-doc #480
Conversation
|
@Sorixelle created this PR trying to fix some issues, we should merge that when we confirm that the issues reported before are fixed. |
| @@ -755,7 +749,14 @@ before, or if the new window is the minibuffer." | |||
| (advice-add #'select-window :around #'lsp-ui-doc-hide-frame-on-window-change) | |||
|
|
|||
| (advice-add 'load-theme :before (lambda (&rest _) (lsp-ui-doc--delete-frame))) | |||
| (add-hook 'window-configuration-change-hook #'lsp-ui-doc--hide-frame) | |||
|
|
|||
| (defun lsp-ui-doc--hide-checking-buffer () | |||
There was a problem hiding this comment.
Why we only hide frame when the current buffer is different from buffer where posframe is invoked?
Another issue is that lsp-ui-doc--original-buffer-with-frame is local variable, thus if it's ever set, it will only contain the value of its buffer, so the second check of (not (eq (current-buffer) lsp-ui-doc--original-buffer-with-frame)) is likely always failed.
There was a problem hiding this comment.
You are right I forgot undo the defvar-local that I was testing.
Why we only hide frame when the current buffer is different from buffer where posframe is invoked?
Because if we hide every time window-configuration-change-hook is triggered, the frame hides when the window size change, e.g when mini-buffer size change:
There was a problem hiding this comment.
Is it necessary to hide when window-configuration-change-hook?
company-posframe doesn't do it neither
There was a problem hiding this comment.
Is it necessary to hide when
window-configuration-change-hook?
company-posframedoesn't do it neither
comapany-posframe is managed by company itself so there is no need company-posframe to handle that.
There was a problem hiding this comment.
So is it necessary to hide when window-configuration-change-hook when in other buffer?
From what I can see, before may be we worried about changing window size will make the mini frame not fully displayable, especially when not in at-point mode. Thus it would make sense to hide the frame.
However, with this change, if the current buffer is still in focus, even with window configuration run it will not hide the frame. Given that window configuration can be easily changed due to mini buffer, I think that we don't need to hide doc frame in this hook anymore.
There was a problem hiding this comment.
From what I can see, before may be we worried about changing window size will make the mini frame not fully displayable, especially when not in at-point mode. Thus it would make sense to hide the frame.
I hope that this is not going to happen or at least it won't be a huge concern. IMO the primary reason for hiding the popup should be buffer change and/or no hover info at the point. Even if we decide to handle that case it should not be unconditional like we currently do, but it should be like "1. subscribe for window-configuration-change. 2. Hide lsp-ui-doc if there is no enough space to show the popup" . The problem with current implementation is that it hides the popup on pretty much every frame resizing(e. g. dap-controls-mode, lv/signature, multiline modeline).
There was a problem hiding this comment.
I agree.
Here is the commit to add the hook 2214908.
I've tried it without window-configuration-change-hook and found no problem. Maybe we don't need that hook at all.
|
Merge ? |
|
I think that all that complained in #465 should test that. (@seagle0128 was one of them) |
|
Yes, I tested it on NixOS (bspwm window manager), but it would be better to test on macOS and gnome that had issues. |
|
It works fine on gnome2. |
|
I tested on macOS. It's not working well. The screenshot is below.
The original is |
|
Can this one be merged yet? |
|
We need to fix @seagle0128 bug or ask for more people to test it, I think |
Another try to #459 which was reverted