Skip to content

lsp-rename: improve UI #2317

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

Merged
merged 17 commits into from
Nov 20, 2020
Merged

lsp-rename: improve UI #2317

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
44accbb
`lsp-rename': check if supported
nbfalcon Nov 9, 2020
ef951ec
`lsp-rename': support ommitted :placeholder
nbfalcon Nov 9, 2020
66adf55
Address PR review
nbfalcon Nov 9, 2020
39acd1f
`lsp-rename': use built-in error functions
nbfalcon Nov 9, 2020
f353c9f
Fix renaming without prepareRename support
nbfalcon Nov 9, 2020
0e1193e
`lsp-rename': move `user-error' into `interactive'
nbfalcon Nov 9, 2020
0e44da7
Revert unrelated changes
nbfalcon Nov 10, 2020
c5ad2f2
`lsp-rename': highlight renaming
nbfalcon Nov 10, 2020
98590a0
`lsp-rename': add tests
nbfalcon Nov 10, 2020
87f9313
`lsp--read-rename': use a variable to hold overlay
nbfalcon Nov 10, 2020
3f941f4
`lsp-rename-face': add missing :group
nbfalcon Nov 10, 2020
fb9c150
`lsp-rename': tests: check overlays
nbfalcon Nov 10, 2020
3507a96
`lsp-rename': make prepareRename configurable
nbfalcon Nov 11, 2020
7e92089
Address nit: `-doto'
nbfalcon Nov 14, 2020
778c9e9
Use a consistent placeholder face
nbfalcon Nov 17, 2020
126f74e
`lsp-face-rename': underline instead of highlight
nbfalcon Nov 17, 2020
ae280ae
Make `lsp-rename-history' public
nbfalcon Nov 20, 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
95 changes: 75 additions & 20 deletions lsp-mode.el
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5653,30 +5653,85 @@ perform the request synchronously."
(seq-map #'lsp--symbol-information-to-xref
(lsp-request "workspace/symbol" `(:query ,pattern))))

(defcustom lsp-rename-use-prepare t
"Whether `lsp-rename' should do a prepareRename first.
For some language servers, textDocument/prepareRename might be
too slow, in which case this variable may be set to nil.
`lsp-rename' will then use `thing-at-point' `symbol' to determine
the symbol to rename at point.")

(defun lsp--get-symbol-to-rename ()
"Get symbol to rename and placeholder at point."
(if (let ((rename-provider (or (lsp--capability :renameProvider)
(-some-> (lsp--registered-capability "textDocument/rename")
(lsp--registered-capability-options)))))
(lsp:rename-options-prepare-provider? rename-provider))
(-when-let (response (lsp-request "textDocument/prepareRename"
(lsp--text-document-position-params)))
(-let* (((start . end) (lsp--range-to-region
(if (lsp-range? response)
response
(lsp:prepare-rename-result-range response))))
(symbol (buffer-substring-no-properties start end))
(placeholder (lsp:prepare-rename-result-placeholder response)))
(cons symbol (or placeholder symbol))))
(let ((symbol (thing-at-point 'symbol t)))
(cons symbol symbol))))
"Get a symbol to rename and placeholder at point.
Returns a cons ((START . END) . PLACEHOLDER?), and nil if
renaming is generally supported but cannot be done at point.
START and END are the bounds of the identifiers being renamed,
while PLACEHOLDER?, is either nil or a string suggested by the
language server as the initial input of a new-name prompt."
(unless (lsp-feature? "textDocument/rename")
(error "The connected server(s) doesn't support renaming"))
(if (and lsp-rename-use-prepare (lsp-feature? "textDocument/prepareRename"))
(when-let ((response
(lsp-request "textDocument/prepareRename"
(lsp--text-document-position-params))))
(let* ((bounds (lsp--range-to-region
(if (lsp-range? response)
response
(lsp:prepare-rename-result-range response))))
(placeholder
(and (not (lsp-range? response))
(lsp:prepare-rename-result-placeholder response))))
(cons bounds placeholder)))
(when-let ((bounds (bounds-of-thing-at-point 'symbol)))
(cons bounds nil))))

(defface lsp-face-rename '((t :underline t))
"Face used to highlight the identifier being renamed.
Renaming can be done using `lsp-rename'."
:group 'lsp-faces)

(defface lsp-rename-placeholder-face '((t :inherit font-lock-variable-name-face))
"Face used to display the rename placeholder in.
When calling `lsp-rename' interactively, this will be the face of
the new name."
:group 'lsp-faces)

(defvar lsp-rename-history '()
"History for `lsp--read-rename'.")

(defun lsp--read-rename (at-point)
"Read a new name for a `lsp-rename' at `point' from the user.
AT-POINT shall be a structure as returned by
`lsp--get-symbol-to-rename'.
Copy link
Member

Choose a reason for hiding this comment

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

It would be better to specify what the structure look like instead of just saying it will be the return value from lsp--get-symbol-to-rename

Copy link
Member Author

Choose a reason for hiding this comment

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

The structure is defined in lsp--get-symbol-to-rename's docstring.


Returns a string, which should be the new name for the identifier
at point. If renaming cannot be done at point (as determined from
AT-POINT), throw a `user-error'.

This function is for use in `lsp-rename' only, and shall not be
relied upon."
(unless at-point
(user-error "`lsp-rename' is invalid here"))
(-let* ((((start . end) . placeholder?) at-point)
;; Do the `buffer-substring' first to not include `lsp-face-rename'
(rename-me (buffer-substring start end))
(placeholder (or placeholder? rename-me))
(placeholder (propertize placeholder 'face 'lsp-rename-placeholder-face))
Copy link
Member

Choose a reason for hiding this comment

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

I would vote for using different faces for placeholder and for the thing to rename. Using bold face seems to be a good candidate for one of them (to avoid being too colorful).

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you propose overriding the face of the thing to be renamed the same way as is done for the placeholder? What we might also want to explore is adding a face to it, using add-face-text-property. I'll post some screenshots of various approaches soon.

Copy link
Member

Choose a reason for hiding this comment

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

I am proposing using 2 different faces like this:

(let ((placeholder "foo")
      (rename-me "foo"))
  (read-string (format "Rename %s to: " (propertize rename-me 'face 'bold)) 
               (propertize placeholder 'face 'font-lock-variable-name-face)
               'lsp-rename-history))


overlay)
;; We need unwind protect, as the user might cancel here, causing the
;; overlay to linger.
(unwind-protect
(progn
(setq overlay (make-overlay start end))
(overlay-put overlay 'face 'lsp-face-rename)

(read-string (format "Rename %s to: " rename-me) placeholder
'lsp-rename-history))
(and overlay (delete-overlay overlay)))))

(defun lsp-rename (newname)
Copy link
Member

Choose a reason for hiding this comment

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

Why don't we just check the capability in this command (public) instead of changing internal one?
I don't think guarding every internal one is desirable/good from perf POV. Just guard once in public one should be enough, shouldn't it?

Copy link
Member Author

Choose a reason for hiding this comment

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

If we guard only in lsp-rename, the user would be prompted to enter a new name even though renaming is unsupported.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, it's fast enough:

(with-current-buffer "main.c"
  (benchmark-run 1000 (ignore (lsp-feature? "textDocument/rename"))))

yields 0.037 seconds.

Calling it only twice gives me 2ms timings, which is less than a 120FPS monitor's refresh latency.

Copy link
Member

Choose a reason for hiding this comment

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

If we guard only in lsp-rename, the user would be prompted to enter a new name even though renaming is unsupported.

see #2317 (comment)
yyoncho@443f801 .

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it better to move the check outside of lsp--get-symbol-to-rename though?

Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to do that. lsp--get-symbol-to-rename is internal function, which should only be called once the condition (has capability for rename) has been cleared.
That would be a good convention to follow, else we will have to add the check for every function, which can be overwhelming.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's actually called only once though, in lsp--get-symbol-to-rename. We needn't add checks in every function, because the server will respond with a proper "not implemented" error if it isn't supported. Here, we cannot rely on that though, since we mustn't prompt the user for a name if the rename wouldn't work anyway.

Copy link
Member

Choose a reason for hiding this comment

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

I think you don't get my point. Let say if the lsp--get-symbol-to-rename is gone/refactored, which can happen since it's internal function, now we will need to find a new place to put the capacity checking.
On other hand, lsp-rename is public function, and is unlikely to change without a breaking change, put the capacity guard there is the best, since user has to use that function to actually go through rename.
Is there any specific concern why the capacity guard has to be put in lsp--get-symbol-to-rename?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could add an additional lsp-feature? check to lsp-rename. However, the check in lsp--get-symbol-to-rename must stay: the interactive block is executed before the body, meaning the user would get prompted for the symbol to rename even though rename might not be possible at all or at point.

Copy link
Member

Choose a reason for hiding this comment

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

You can put the check in (interactive) form too. Like what @yyoncho suggested.
If you hate that, you can even make (interactive '(nil)) and check for newName outside of interactive block and call to lsp--get-symbol-to-rename.

"Rename the symbol (and all references to it) under point to NEWNAME."
(interactive (list (-when-let ((symbol . placeholder) (lsp--get-symbol-to-rename))
(read-string (format "Rename %s to: " symbol) placeholder nil symbol))))
(unless newname
(user-error "A rename is not valid at this position"))
(interactive (list (lsp--read-rename (lsp--get-symbol-to-rename))))
(when-let ((edits (lsp-request "textDocument/rename"
`( :textDocument ,(lsp--text-document-identifier)
:position ,(lsp--cur-position)
Expand Down
92 changes: 92 additions & 0 deletions test/lsp-methods-test.el
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,3 +302,95 @@ private void extracted() {
:kind "rename")]))
(should-not (f-exists? old-file-name))
(should (f-exists? new-file-name))))

;;; `lsp-rename'
(defmacro lsp-test--simulated-input (keys &rest body)
"Execute body, while simulating the pressing KEYS.
KEYS is passed to `execute-kbd-macro', after being run trough
`kbd'. Returns the result of the last BODY form."
(declare (indent 1))
`(let (result)
(execute-kbd-macro (kbd ,keys) 1 (lambda () (setq result (progn ,@body))))
result))

(defun lsp-test--rename-overlays? (pos)
"Return non-nil if there are `lsp-rename' overlays at POS.
POS is a point in the current buffer."
(--any? (equal (overlay-get it 'face) 'lsp-face-rename)
(overlays-at pos)))

(ert-deftest lsp--read-rename ()
"Ensure that `lsp--read-rename' works.
If AT-POINT is nil, it throws a `user-error'.
If a placeholder is given, it shall be the default value,

otherwise the bounds are to be used.

Rename overlays are removed afterwards, even if the user presses
C-g."
(should-error (lsp--read-rename nil) :type 'user-error)
(with-temp-buffer
(insert "identifier")
(should (string= "identifier"
(lsp-test--simulated-input "RET"
(lsp--read-rename '((1 . 11) . nil)))))
(should (string= "ident"
(lsp-test--simulated-input "RET"
(lsp--read-rename '((1 . 10) . "ident")))))
(goto-char 1)
(condition-case nil
(cl-letf (((symbol-function #'read-string)
(lambda (&rest _)
;; NOTE: BEGIN and END means a range [BEGIN;END[, so at
;; point 10, there shouldn't be an overlay anymore. This is
;; consistent with of `bounds-thing-at-point', and it
;; worked during manual testing.
(should (lsp-test--rename-overlays? 1))
(should (lsp-test--rename-overlays? 9))
(should-not (lsp-test--rename-overlays? 10)))))
(lsp--read-rename '((1 . 10) . "id")))
(quit))
;; but not after `lsp--read-rename'
(should-not (lsp-test--rename-overlays? 1))
(should-not (lsp-test--rename-overlays? 9))
(should-not (lsp-test--rename-overlays? 10))))

(ert-deftest lsp--get-symbol-to-rename ()
"Test `lsp--get-symbol-to-rename'.
It should error if renaming cannot be done, make use of
prepareRename as much as possible, with or without bounds, and it
should work without the latter."
;; We don't support rename
(cl-letf (((symbol-function #'lsp-feature?) #'ignore))
(should-error (lsp--get-symbol-to-rename) :type 'error))
(cl-letf (((symbol-function #'lsp--text-document-position-params) #'ignore)
((symbol-function #'lsp--range-to-region) #'identity))
(with-temp-buffer
(insert "identifier")
(goto-char 1)
;; We do support rename, but no prepareRename
(cl-letf (((symbol-function #'lsp-feature?)
(lambda (f) (member f '("textDocument/rename")))))
(should (equal (cons (bounds-of-thing-at-point 'symbol) nil)
(lsp--get-symbol-to-rename)))
(goto-char (point-max))
(insert " ")
;; we are not on an identifier
(should (equal nil (lsp--get-symbol-to-rename))))
;; Do the following tests with an identifier at point
(goto-char 1)
(cl-letf (((symbol-function #'lsp-feature?)
(lambda (f) (member f '("textDocument/rename"
"textDocument/prepareRename")))))
(cl-letf (((symbol-function #'lsp-request)
(lambda (&rest _) (lsp-make-prepare-rename-result
:range '(1 . 12)
:placeholder nil))))
(should (equal '((1 . 12) . nil) (lsp--get-symbol-to-rename))))
(cl-letf (((symbol-function #'lsp-request)
(lambda (&rest _) (lsp-make-prepare-rename-result
:range '(1 . 12)
:placeholder "_"))))
(should (equal '((1 . 12) . "_") (lsp--get-symbol-to-rename))))))))

;;; lsp-methods-test.el ends here