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.

Sign up for GitHub

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

Add posframe integration to lsp-ui-doc #480

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Add posframe integration to lsp-ui-doc #480

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8a8ac9e
Add posframe integration to lsp-ui-doc
ericdallo Aug 1, 2020
ab97add
Fix hide window when buffer didn't change
ericdallo Aug 1, 2020
9c5b01f
Avoid mouse focus
ericdallo Aug 1, 2020
0728d41
Fix performace following https://github.com/emacs-lsp/lsp-ui/pull/465…
ericdallo Aug 1, 2020
96e79b3
Fix tests
ericdallo Aug 1, 2020
1068163
Make variable global
ericdallo Aug 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cask
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
(depends-on "lsp-mode")
(depends-on "markdown-mode")
(depends-on "rustic")
(depends-on "posframe")

(package-file "lsp-ui.el")
(files "*.el" "lsp-ui-doc.html")
201 changes: 104 additions & 97 deletions lsp-ui-doc.el
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
(require 'goto-addr)
(require 'markdown-mode)
(require 'cl-lib)
(require 'posframe)

(when (featurep 'xwidget-internal)
(require 'xwidget))
Expand Down Expand Up @@ -144,29 +145,16 @@ Only the `background' is used in this face."
:group 'lsp-ui-doc)

(defvar lsp-ui-doc-frame-parameters
'((left . -1)
(no-focus-on-map . t)
(min-width . 0)
(width . 0)
(min-height . 0)
(height . 0)
(internal-border-width . 1)
'((no-focus-on-map . t)
(vertical-scroll-bars . nil)
(horizontal-scroll-bars . nil)
(right-fringe . 0)
(menu-bar-lines . 0)
(tool-bar-lines . 0)
(line-spacing . 0)
(unsplittable . t)
(undecorated . t)
(top . -1)
(visibility . nil)
(mouse-wheel-frame . nil)
(no-other-frame . t)
(no-accept-focus . nil)
(focus-follows-mouse . nil)
(mouse-autoselect-window . nil)
(inhibit-double-buffering . t)
(drag-internal-border . t)
(no-special-glyphs . t)
(desktop-dont-save . t))
(cursor-type . box)
(drag-internal-border . t))
"Frame parameters used to create the frame.")

(defvar lsp-ui-doc-render-function nil
Expand Down Expand Up @@ -198,6 +186,8 @@ Because some variables are buffer local.")
(defvar-local lsp-ui-doc--bounds nil)
(defvar-local lsp-ui-doc--timer nil)

(defvar-local lsp-ui-doc--inline-width nil)

(defconst lsp-ui-doc--buffer-prefix " *lsp-ui-doc-")

(defmacro lsp-ui-doc--with-buffer (&rest body)
Expand Down Expand Up @@ -365,10 +355,7 @@ We don't extract the string that `lps-line' is already displaying."
(when (overlayp lsp-ui-doc--inline-ov)
(delete-overlay lsp-ui-doc--inline-ov))
(when (lsp-ui-doc--get-frame)
(unless lsp-ui-doc-use-webkit
(lsp-ui-doc--with-buffer
(erase-buffer)))
(make-frame-invisible (lsp-ui-doc--get-frame))))
(posframe-hide (lsp-ui-doc--make-buffer-name))))

(defun lsp-ui-doc--buffer-width ()
"Calcul the max width of the buffer."
Expand Down Expand Up @@ -403,19 +390,6 @@ We don't extract the string that `lps-line' is already displaying."
(xwidget-resize (lsp-ui-doc--webkit-get-xwidget) offset-width offset-height))
(lsp-ui-doc--move-frame (lsp-ui-doc--get-frame)))

(defun lsp-ui-doc--resize-buffer ()
"If the buffer's width is larger than the current frame, resize it."
(if lsp-ui-doc-use-webkit
(lsp-ui-doc--webkit-execute-script
"[document.querySelector('#lsp-ui-webkit').offsetWidth, document.querySelector('#lsp-ui-webkit').offsetHeight];"
'lsp-ui-doc--webkit-resize-callback)

(let* ((frame-width (frame-width))
(fill-column (min lsp-ui-doc-max-width (- frame-width 5))))
(when (> (lsp-ui-doc--buffer-width) (min lsp-ui-doc-max-width frame-width))
(lsp-ui-doc--with-buffer
(fill-region (point-min) (point-max)))))))

(defun lsp-ui-doc--mv-at-point (frame width height start-x start-y)
"Move FRAME to be where the point is.
WIDTH is the child frame width.
Expand Down Expand Up @@ -459,12 +433,11 @@ FRAME just below the symbol at point."
(if (eq lsp-ui-doc-position 'at-point)
(lsp-ui-doc--mv-at-point frame width height left top)
(set-frame-position frame
(max (- frame-right width 10 (frame-char-width)) 10)
(max (- frame-right width) 0)
(pcase lsp-ui-doc-position
('top (+ top 10))
('top top)
('bottom (- (lsp-ui-doc--line-height 'mode-line)
height
10)))))))
height)))))))

(defun lsp-ui-doc--visit-file (filename)
"Visit FILENAME in the parent frame."
Expand Down Expand Up @@ -495,26 +468,31 @@ FN is the function to call on click."
(lsp-ui-doc--put-click (match-beginning 0) (match-end 0)
'browse-url-at-mouse)))))

(defun lsp-ui-doc--render-buffer (string symbol)
"Set the buffer with STRING."
(defvar lsp-ui-doc--render-string nil
"The string to render in the documentation popup.")
(defvar lsp-ui-doc--render-symbol nil
"The symbol to render documentation for.")

(defun lsp-ui-doc--render-buffer ()
"Set the buffer with `lsp-ui-doc--render-string'."
(lsp-ui-doc--with-buffer
(if lsp-ui-doc-use-webkit
(progn
(lsp-ui-doc--webkit-execute-script
(format
"renderMarkdown('%s', '%s');"
symbol
(url-hexify-string string))
lsp-ui-doc--render-symbol
(url-hexify-string lsp-ui-doc--render-string))
'lsp-ui-doc--webkit-resize-callback))
(erase-buffer)
(let ((inline-p (lsp-ui-doc--inline-p)))
(insert (concat (unless inline-p (propertize "\n" 'face '(:height 0.2)))
(s-trim string)
(s-trim lsp-ui-doc--render-string)
(unless inline-p (propertize "\n\n" 'face '(:height 0.3))))))
(lsp-ui-doc--make-clickable-link))
(setq-local face-remapping-alist `((header-line lsp-ui-doc-header)))
(setq-local window-min-height 1)
(setq header-line-format (when lsp-ui-doc-header (concat " " symbol))
(setq header-line-format (when lsp-ui-doc-header (concat " " lsp-ui-doc--render-symbol))
mode-line-format nil
cursor-type nil)))

Expand All @@ -532,8 +510,6 @@ FN is the function to call on click."
(setq start (text-property-not-all 0 (length string) 'invisible nil string)))
string))

(defvar-local lsp-ui-doc--inline-width nil)

(defun lsp-ui-doc--inline-window-width nil
(- (min (window-text-width)
(window-body-width))
Expand Down Expand Up @@ -619,49 +595,65 @@ HEIGHT is the documentation number of lines."
(defun lsp-ui-doc--inline-p ()
"Return non-nil when the documentation should be display without a child frame."
(or (not lsp-ui-doc-use-childframe)
(not (display-graphic-p))
(not (posframe-workable-p))
(not (fboundp 'display-buffer-in-child-frame))))

(defun lsp-ui-doc--display (symbol string)
"Display the documentation."
"Display the documentation for SYMBOL and STRING."
(setq lsp-ui-doc--render-symbol symbol
lsp-ui-doc--render-string string)
(when (and lsp-ui-doc-use-webkit (not (featurep 'xwidget-internal)))
(setq lsp-ui-doc-use-webkit nil))
(if (or (null string) (string-empty-p string))
(lsp-ui-doc--hide-frame)
(lsp-ui-doc--render-buffer string symbol)
(lsp-ui-doc--render-buffer)
(if (lsp-ui-doc--inline-p)
(lsp-ui-doc--inline)
(unless (lsp-ui-doc--get-frame)
(lsp-ui-doc--set-frame (lsp-ui-doc--make-frame)))
(unless lsp-ui-doc-use-webkit
(lsp-ui-doc--resize-buffer)
(lsp-ui-doc--move-frame (lsp-ui-doc--get-frame)))
(lsp-ui-doc--set-frame (lsp-ui-doc--make-frame)))
(unless (frame-visible-p (lsp-ui-doc--get-frame))
(make-frame-visible (lsp-ui-doc--get-frame))))))

(defun lsp-ui-doc--posframe-poshandler-point-top-left-corner (info)
"Place the posframe at the top-left corner of the point without covering it.
The structure of INFO is defined in the documentation of `posframe-show'."
(let* ((frame (plist-get info :posframe))
(height (frame-pixel-height frame)))
(posframe-poshandler-point-bottom-left-corner info (- height))))

(defvar lsp-ui-doc--original-buffer-with-frame nil)

(defun lsp-ui-doc--make-frame ()
"Create the child frame and return it."
(lsp-ui-doc--delete-frame)
(let* ((after-make-frame-functions nil)
(before-make-frame-hook nil)
(name-buffer (lsp-ui-doc--make-buffer-name))
(buffer (get-buffer name-buffer))
(let* ((before-make-frame-hook nil)
(buffer-name (lsp-ui-doc--make-buffer-name))
(buffer (get-buffer-create buffer-name))
(params (append lsp-ui-doc-frame-parameters
`((name . "")
(default-minibuffer-frame . ,(selected-frame))
(minibuffer . ,(minibuffer-window))
(left-fringe . ,(frame-char-width))
(background-color . ,(face-background 'lsp-ui-doc-background nil t)))))
(window (display-buffer-in-child-frame
buffer
`((child-frame-parameters . ,params))))
(frame (window-frame window)))
(minibuffer . ,(minibuffer-window)))))
(position (pcase (list lsp-ui-doc-position lsp-ui-doc-alignment)
('(top frame) #'posframe-poshandler-frame-top-right-corner)
('(top window) #'posframe-poshandler-window-top-right-corner)
('(bottom frame) #'posframe-poshandler-frame-bottom-right-corner)
('(bottom window) #'posframe-poshandler-window-bottom-right-corner)
('(at-point frame) #'lsp-ui-doc--posframe-poshandler-point-top-left-corner)
('(at-point window) #'lsp-ui-doc--posframe-poshandler-point-top-left-corner)))
(frame (posframe-show buffer
:width lsp-ui-doc-max-width
:height lsp-ui-doc-max-height
:poshandler position
:internal-border-width 1
:internal-border-color lsp-ui-doc-border
:left-fringe t
:right-fringe t
:background-color (face-background 'lsp-ui-doc-background nil t)
:override-parameters params))
(window (frame-root-window frame)))
(setq lsp-ui-doc--original-buffer-with-frame (current-buffer))
(with-current-buffer buffer
(lsp-ui-doc-frame-mode 1))
(set-frame-parameter nil 'lsp-ui-doc-buffer buffer)
(set-window-dedicated-p window t)
(redirect-frame-focus frame (frame-parent frame))
(set-face-background 'internal-border lsp-ui-doc-border frame)
(lsp-ui-doc-frame-mode 1)
(visual-line-mode 1))
(set-face-background 'fringe nil frame)
(run-hook-with-args 'lsp-ui-doc-frame-hook frame window)
(when lsp-ui-doc-use-webkit
Expand All @@ -670,6 +662,8 @@ HEIGHT is the documentation number of lines."
(interactive)

(let ((xwidget-event-type (nth 1 last-input-event)))
(when (eq xwidget-event-type 'load-changed)
(lsp-ui-doc--render-buffer))
;; (when (eq xwidget-event-type 'load-changed)
;; (lsp-ui-doc--move-frame (lsp-ui-doc--get-frame)))

Expand Down Expand Up @@ -727,7 +721,7 @@ BUFFER is the buffer where the request has been made."
(defun lsp-ui-doc--delete-frame ()
"Delete the child frame if it exists."
(-when-let (frame (lsp-ui-doc--get-frame))
(delete-frame frame)
(posframe-delete-frame (lsp-ui-doc--make-buffer-name))
(lsp-ui-doc--set-frame nil)))

(defun lsp-ui-doc--visible-p ()
Expand Down Expand Up @@ -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 ()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
lsp-ui-doc-posframe-bug1

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it necessary to hide when window-configuration-change-hook?
company-posframe doesn't do it neither

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it necessary to hide when window-configuration-change-hook?
company-posframe doesn't do it neither

comapany-posframe is managed by company itself so there is no need company-posframe to handle that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"Check if buffer has changed before hide hook."
(when (or (null lsp-ui-doc--original-buffer-with-frame)
(not (eq (current-buffer) lsp-ui-doc--original-buffer-with-frame)))
(lsp-ui-doc--hide-frame)))

(add-hook 'window-configuration-change-hook #'lsp-ui-doc--hide-checking-buffer)

(advice-add #'keyboard-quit :before #'lsp-ui-doc--hide-frame)

Expand All @@ -766,6 +767,27 @@ before, or if the new window is the minibuffer."
(and (buffer-live-p it) it)
(kill-buffer it)))

(define-minor-mode lsp-ui-doc-frame-mode
"Marker mode to add additional key bind for lsp-ui-doc-frame."
:init-value nil
:lighter ""
:group lsp-ui-doc
:keymap `(([?q] . lsp-ui-doc-unfocus-frame)))

(defun lsp-ui-doc-focus-frame ()
"Focus into lsp-ui-doc-frame."
(interactive)
(when (lsp-ui-doc--frame-visible-p)
(lsp-ui-doc--with-buffer
(setq cursor-type t))
(select-frame-set-input-focus (lsp-ui-doc--get-frame))))

(defun lsp-ui-doc-unfocus-frame ()
"Unfocus from lsp-ui-doc-frame."
(interactive)
(when-let ((frame (frame-parent (lsp-ui-doc--get-frame))))
(select-frame-set-input-focus frame)))

(define-minor-mode lsp-ui-doc-mode
"Minor mode for showing hover information in child frame."
:init-value nil
Expand All @@ -783,11 +805,17 @@ before, or if the new window is the minibuffer."
(cl-callf copy-tree frameset-filter-alist)
(push '(lsp-ui-doc-frame . :never) frameset-filter-alist)))
(add-hook 'post-command-hook 'lsp-ui-doc--make-request nil t)
(add-hook 'delete-frame-functions 'lsp-ui-doc--on-delete nil t))
(add-hook 'delete-frame-functions 'lsp-ui-doc--on-delete nil t)
(advice-add #'posframe--redirect-posframe-focus
:before-until (lambda (&rest _)
lsp-ui-doc-frame-mode)
'((name . lsp-ui-doc--dont-redirect-posframe))))
(t
(lsp-ui-doc-hide)
(remove-hook 'post-command-hook 'lsp-ui-doc--make-request t)
(remove-hook 'delete-frame-functions 'lsp-ui-doc--on-delete t))))
(remove-hook 'delete-frame-functions 'lsp-ui-doc--on-delete t)
(advice-remove #'posframe--redirect-posframe-focus
'lsp-ui-doc--dont-redirect-posframe))))

(defun lsp-ui-doc-enable (enable)
"Enable/disable ‘lsp-ui-doc-mode’.
Expand Down Expand Up @@ -825,26 +853,5 @@ It is supposed to be called from `lsp-ui--toggle'"
(cancel-timer lsp-ui-doc--unfocus-frame-timer))
(add-hook 'post-command-hook 'lsp-ui-doc--glance-hide-frame))

(define-minor-mode lsp-ui-doc-frame-mode
"Marker mode to add additional key bind for lsp-ui-doc-frame."
:init-value nil
:lighter ""
:group lsp-ui-doc
:keymap `(([?q] . lsp-ui-doc-unfocus-frame)))

(defun lsp-ui-doc-focus-frame ()
"Focus into lsp-ui-doc-frame."
(interactive)
(when (lsp-ui-doc--frame-visible-p)
(lsp-ui-doc--with-buffer
(setq cursor-type t))
(select-frame-set-input-focus (lsp-ui-doc--get-frame))))

(defun lsp-ui-doc-unfocus-frame ()
"Unfocus from lsp-ui-doc-frame."
(interactive)
(when-let ((frame (frame-parent (lsp-ui-doc--get-frame))))
(select-frame-set-input-focus frame)))

(provide 'lsp-ui-doc)
;;; lsp-ui-doc.el ends here
2 changes: 1 addition & 1 deletion lsp-ui.el
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
;; Author: Sebastien Chapuis <[email protected]>, Fangrui Song <[email protected]>
;; Keywords: languages, tools
;; URL: https://github.com/emacs-lsp/lsp-ui
;; Package-Requires: ((emacs "26.1") (dash "2.14") (dash-functional "1.2.0") (lsp-mode "6.0") (markdown-mode "2.3"))
;; Package-Requires: ((emacs "26.1") (dash "2.14") (dash-functional "1.2.0") (lsp-mode "6.0") (markdown-mode "2.3") (posframe "0.7.0"))
;; Version: 7.0

;;; License
Expand Down
1 change: 1 addition & 0 deletions test/test-helper.el
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
(file-name-as-directory (f-parent (f-parent (f-this-file)))))

(require 'lsp-mode)
(require 'lsp-modeline)
(require 'lsp-rust)
(require 'lsp-ui)
(require 'flycheck)
Expand Down