Skip to content

Conversation

@yyoncho
Copy link
Member

@yyoncho yyoncho commented Jan 14, 2019

Fixes #361

The lenses support is similar to what VScode has:

  • Lenses are displayed above the line

  • Lenses on the same line are grouped and displayed above the first text item.

  • Extension could register custom lences provider and add custom lenses, e. g.
    JDT LS has extensions that display the tests hints.

  • The placement code was adapter from https://github.com/cpitclaudel/quick-peek

Tested against CCLS and JDT LS. CCLS has to register the proper handler(similar
to what ccls vscode extension does). Some other servers like rls do clain that
they support lenses but I was unable to trigger them.

Further improvements:

  • Provide different methods for redering methods (e. g. in the end of the line)
  • Keyboard support for the lenses
  • Test more servers

selection_077

lsp-mode.el Outdated

(defun lsp--lenses-display (lenses)
"Show LENSES."
(let ((overlays (->> lenses
Copy link
Member

Choose a reason for hiding this comment

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

Some new lines here to reduce indentation?

Copy link
Member Author

Choose a reason for hiding this comment

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

Restructured the code + deleted one variable which wasn't used.

lsp-mode.el Outdated
"Face used for highlighting symbols being written to."
:group 'lsp-faces)

(defcustom lsp-lenses-check-interval 0.1
Copy link
Member

Choose a reason for hiding this comment

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

Singular form may be good enough... codeLens is in its singular form.

Copy link
Member Author

Choose a reason for hiding this comment

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

Make sense, will change it everywhere.

lsp-mode.el Outdated
"Delete all lenses."
(interactive)
(let ((scroll-preserve-screen-position t))
(-each lsp--lenses-overlays 'delete-overlay)
Copy link
Member

Choose a reason for hiding this comment

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

Builtin mapc suffices.. No need for -each

lsp-mode.el Outdated
(goto-char (overlay-start ov))
(overlay-put ov
'before-string
(format "%s%s\n"
Copy link
Member

Choose a reason for hiding this comment

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

concat?

Copy link
Member Author

Choose a reason for hiding this comment

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

will do

lsp-mode.el Outdated
"Create iddle function for buffer BUFFER."
(when (or (not buffer) (eq (current-buffer) buffer))
(cond
((not (equal (buffer-modified-tick) lsp--lenses-modified-tick))
Copy link
Member

Choose a reason for hiding this comment

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

For numbers, which is more common, = or eql?

Copy link
Member Author

Choose a reason for hiding this comment

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

will change it to =

@yyoncho yyoncho force-pushed the lens branch 2 times, most recently from cec57ff to b5b7cfd Compare January 18, 2019 13:21
@yyoncho
Copy link
Member Author

yyoncho commented Jan 18, 2019

Addressed comments

lsp-mode.el Outdated
(defvar-local lsp--lens-idle-timer nil
"Pair of points which holds the last window location the lenses were loaded.")

(defvar-local lsp--lences-data nil
Copy link
Member

Choose a reason for hiding this comment

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

lsp--lens-data

@MaskRay
Copy link
Member

MaskRay commented Jan 18, 2019

Typo in description. clain -> claim

"was adapter" > "adapted"

... CCLS can be referred in its lower case form..

(or (car (cl-remove-if-not (lambda (ov) (lsp--lens-overlay-matches-pos ov pos)) lsp--lens-overlays))
(let* ((ov (save-excursion
(goto-char pos)
(make-overlay (point-at-bol) (1+ (point-at-eol))))))
Copy link
Member

Choose a reason for hiding this comment

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

Set front-advance but unset rear-advance

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you elaborate?

Copy link
Member

@MaskRay MaskRay Jan 18, 2019

Choose a reason for hiding this comment

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

(make-overlay (point-at-bol) (1+ (point-at-eol) nil 'front-advance)

Oh, this does not matter if the overlay is on its own line. It matters when the overlay is on the same line of the text.

lsp-mode.el Outdated
"Create iddle function for buffer BUFFER."
(when (or (not buffer) (eq (current-buffer) buffer))
(cond
((not (eq (buffer-modified-tick) lsp--lens-modified-tick))
Copy link
Member

Choose a reason for hiding this comment

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

/=

lsp-mode.el Outdated
(puthash "pending" t it)
(lsp-request-async "codeLens/resolve" it
(lambda (lens)
(when (equal tick (buffer-modified-tick))
Copy link
Member

Choose a reason for hiding this comment

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

=

lsp-mode.el Outdated
(lsp-request-async "textDocument/codeLens"
`(:textDocument (:uri ,(lsp--path-to-uri buffer-file-name)))
(lambda (lenses)
(when (equal tick (buffer-modified-tick))
Copy link
Member

Choose a reason for hiding this comment

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

=

lsp-mode.el Outdated
(-some-> lsp--lens-refresh-timer cancel-timer)

(setq-local lsp--lens-modified-tick (buffer-modified-tick))
(setq-local lsp--lens-page (list (window-start) (window-end)))
Copy link
Member

Choose a reason for hiding this comment

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

cons? And line 1013

lsp-mode.el Outdated
it
(lsp-request "codeLens/resolve" it)))
lsp--lens-display)))
(--each lsp--lens-overlays
Copy link
Member

Choose a reason for hiding this comment

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

The (--each ...) is redundant

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, copy/paste mistake, the only thing that is needed is the code in the let statement.

lsp-mode.el Outdated
(--each lsp--lens-overlays
(unless (-contains? overlays it)
(delete-overlay it)))
(setq-local lsp--lens-overlays overlays)))
Copy link
Member

Choose a reason for hiding this comment

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

setq-local is also redundant

@yyoncho yyoncho force-pushed the lens branch 3 times, most recently from 988ebf7 to 3447600 Compare January 18, 2019 17:18
@yyoncho
Copy link
Member Author

yyoncho commented Jan 18, 2019

Addressed everything except "Set front-advance but unset rear-advance" which is not clear to me.

lsp-mode.el Outdated
`buffer-modified-tick' at the time of receiving the updates the
updates must be discarted..
CALLBACK - the callback for the lenses."
(--each (-filter 'lsp--lens-backend-not-loaded? lenses)
Copy link
Member

Choose a reason for hiding this comment

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

#'

lsp-mode.el Outdated
(when (= tick (buffer-modified-tick))
(remhash "pending" it)
(puthash "command" (gethash "command" lens) it)
(when (-all? 'lsp--lens-backend-present? lenses)
Copy link
Member

Choose a reason for hiding this comment

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

#'

lsp-mode.el Outdated
(< pos (overlay-end ov))))

(defun lsp--lens-idle-function (&optional buffer)
"Create iddle function for buffer BUFFER."
Copy link
Member

Choose a reason for hiding this comment

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

typo. idle

Fixes emacs-lsp#361

The lenses support is similar to what VScode has:

- Lenses are displayed above the line
- Lenses on the same line are grouped and displayed above the first text item.
- Extension could register custom lences provider and add custom lenses, e. g.
JDT LS has extensions that display the tests hints.

- The placement code was adapted from https://github.com/cpitclaudel/quick-peek

Tested against CCLS and JDT LS. CCLS has to register the proper handler(similar
to what ccls vscode extension does). Some other servers like rls do claim that
they support lenses but I was unable to trigger them.

Further improvements:

* Provide different methods for redering methods (e. g. in the end of the line)
* Keyboard support for the lenses
* Test more servers
@yyoncho yyoncho merged commit d4c4e10 into emacs-lsp:master Jan 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants