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.

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

Implement code lens #71

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@mkcms
Copy link
Collaborator

mkcms commented Aug 12, 2018

This is a work in progress code lens implementation.

What's currently missing is executing commands.

@joaotavora

This comment has been minimized.

Copy link
Owner

joaotavora commented Aug 12, 2018

When I read the spec, I don't understand very well what code lens are. Can you explain in your words, maybe with examples? I'd like to find the best available Emacs abstraction to represent them

@mkcms

This comment has been minimized.

Copy link
Collaborator

mkcms commented Aug 12, 2018

They're clickable overlays that are placed on symbols. The eclipse.jdt.ls language servers puts them on function and class names.

They provide information about a symbol and action to perform on this symbol. E.g. the eclipse server puts on each symbol the number of references to it, and a clickable action "find references".

@mkcms mkcms force-pushed the mkcms:code-lens branch 2 times, most recently from c83f48a to b556bdf Aug 15, 2018

@mkcms

This comment has been minimized.

Copy link
Collaborator

mkcms commented Aug 18, 2018

I rewrote everything after I realized that the previous version had some flaws:

  1. Unlike code lens in other editors, the information they provided in the old version was never visible unless you hovered over the overlays.
  2. It assumed that the server will put them in the right place, without having duplicates in the same place.
  3. The code lens looked ugly when they spanned multiple lines.

I tried vscode to see how the code lens are implemented there. The current version is very similar, although unlike in vscode, the code lens are not visible at all times. Currently you have to toggle them with the command eglot-code-lens.

This command will make the buffer read only and enable a new minor mode which provides keybindings p, n to go to previous, next lens, RET to act on lens at point, q to exit the minor mode and clear the lenses.
The lenses are displayed like in vscode, above the line in which they start. If there are multiple lenses on the same line, they are placed one next to another. The title of their command is what's visible in the buffer.

Tell me what you think. If you think that a minor mode is too much, I believe I could also use a transient map, or simply read the events (go to next/previous lens, act on it) from the minibuffer. That will make the command self-contained.

@mkcms

This comment has been minimized.

Copy link
Collaborator

mkcms commented Aug 18, 2018

If you want to test this, I recommend cquery and eclipse.jdt.ls servers.

@cmccloud

This comment has been minimized.

Copy link

cmccloud commented Aug 18, 2018

@joaotavora, for what it's worth, this vscode blog post on code lenses helped me to get a better understanding of what they are and how they can be used.

@joaotavora

This comment has been minimized.

Copy link
Owner

joaotavora commented Aug 18, 2018

@joaotavora, for what it's worth, this vscode blog post on code lenses helped me to get a better understanding of what they are and how they can be used.

Thanks. From what I understand they're very valuable for editors which don't have the capabilities of Emacs. For example, in emacs you can M-x mark-defun and M-x vc-region-history to elegantly get to the history of a function. And finding references is already supported by xref-find-references.

But that doesn't mean this shouldn't be supported. I'm evaluating @mkcms proposal. I mostly need to sit down and configure Java in a medium or small sized project so I can start testing all these features, which seem to be big in that language.

@cmccloud

This comment has been minimized.

Copy link

cmccloud commented Aug 18, 2018

I agree with you. My own take away from the article was the importance of distinguishing between the links (overlays), actions, and the displaying of actions. I see lenses (the overlays) mostly as a discoverability tool, but would almost certainly leave them disabled in my own configuration, provided that I could independently access whatever actions are made available by the code lens. They could still be very useful for being made aware of obscure or less commonly used capabilities.

Similarly, finding a reference via an action might still use xref, but display the result differently, maybe in an undecorated child frame, for example.

@mkcms

This comment has been minimized.

Copy link
Collaborator

mkcms commented Aug 18, 2018

would almost certainly leave them disabled in my own configuration

Unlike in vscode, this implementation is not automatic. You only see the lenses when you explicitly toggle them with eglot-code-lens command.

@cmccloud

This comment has been minimized.

Copy link

cmccloud commented Aug 18, 2018

Unlike in vscode, this implementation is not automatic. You only see the lenses when you explicitly toggle them with eglot-code-lens command.

I definitely took note of that, and do think it's the right approach.

@mkcms mkcms force-pushed the mkcms:code-lens branch from b556bdf to e6b876a Aug 19, 2018

@david-christiansen

This comment has been minimized.

Copy link

david-christiansen commented Aug 24, 2018

For displaying a list of commands based on the text properties and/or overlays at a particular buffer position, it might be worth considering using prop-menu. We use this in idris-mode and a couple of others for displaying commands available at a particular buffer position. It's both keyboard (via completing-read) and mouse (via a menu) accessible.

Something like this could be used to have the commands always be available without putting the buffer in a read-only mode, but without the visual noise of some other approaches.

@david-christiansen

This comment has been minimized.

Copy link

david-christiansen commented Aug 24, 2018

One more potential gain from prop-menu is that it would let the "code actions" menu easily coexist with the "code lens" menu.

@mkcms

This comment has been minimized.

Copy link
Collaborator

mkcms commented Aug 27, 2018

@david-christiansen Nice package. Would it be possible to have support for it and at the same time not actually require it?

@david-christiansen

This comment has been minimized.

Copy link

david-christiansen commented Aug 27, 2018

Would something like this be too ugly?

(with-eval-after-load prop-menu
    (setq-local prop-menu-item-functions '(eglot-prop-menu-items-function))
    (define-key eglot-mode-map (kbd "C-c C-SPC") 'prop-menu-by-completing-read)
    (define-key eglot-mode-map (kbd "<mouse-3>") 'prop-menu-show-menu))

If the concern for avoiding a dependency is the eventual desire to put eglot into ELPA or Emacs proper, I'm happy to assign copyright to the FSF for the library.

@mkcms

This comment has been minimized.

Copy link
Collaborator

mkcms commented Nov 14, 2018

@joaotavora Should I continue this develop this branch, or do you think code lens shouldn't be included in eglot? I personally find them distracting (when they're present all the time), but when browsing code they have some use. I know that ccls, cquery and eclipse.jdt.ls servers all have code lens support.

@joaotavora

This comment has been minimized.

Copy link
Owner

joaotavora commented Dec 3, 2018

I'll be looking at this in the near future. What servers currently support codelens? EclipseJDT and some other one?

@mkcms

This comment has been minimized.

Copy link
Collaborator

mkcms commented Dec 3, 2018

What servers currently support codelens? EclipseJDT and some other one?

Like I said above, ccls and cquery :-)

@joaotavora

This comment has been minimized.

Copy link
Owner

joaotavora commented Dec 3, 2018

Like I said above, ccls and cquery :-)

sorry 😊

First stab at implementing code lens
* eglot.el (eglot-code-lens): New face.
(eglot--lens-mode): New minor mode.
(eglot-lens-act, eglot-next-lens, eglot-previous-lens
eglot-code-lens): New commands.

@joaotavora joaotavora force-pushed the mkcms:code-lens branch from e6b876a to 2595fed Dec 12, 2018

@joaotavora

This comment has been minimized.

Copy link
Owner

joaotavora commented Dec 12, 2018

Hey, I finally tried this with ccls and it's quite interesting. I rebased to current master and pushed to your branch. I'm going to try a flymake-based approach to cut down on the needed code and push that to a separate branch.

@mkcms

This comment has been minimized.

Copy link
Collaborator

mkcms commented Dec 12, 2018

I'm going to try a flymake-based approach

Ha, that's very interesting. I've never even considered that. Can flymake be modified to display some special diagnostics different than others?

@joaotavora

This comment has been minimized.

Copy link
Owner

joaotavora commented Dec 12, 2018

Ha, that's very interesting. I've never even considered that. Can flymake be modified to display some special diagnostics different than others?

Not yet :-)

joaotavora added a commit that referenced this pull request Dec 13, 2018

Per #71: Add rudiments of new stab at CodeLens using Flymake
* eglot.el (eglot--lsp-interface-alist): New CodeLens interface.
(eglot--current-flymake-report-fn)
(eglot--unreported-diagnostics): Move up here.
(eglot--report-diagnostics): New helper.
(eglot-handle-notification textDocument/publishDiagnostics): Use
it.
(eglot--post-self-insert-hook, eglot--pre-command-hook)
(eglot--before-change): Fix docstring.
(eglot-lens-act, eglot-next-lens, eglot-previous-lens): Unimplement.
(eglot-code-lens): Implement using new Flymake.
(eglot--eclipse-jdt-contact): Fix docstring.
@joaotavora

This comment has been minimized.

Copy link
Owner

joaotavora commented Dec 13, 2018

Just pushed a new Flymake to Emacs master, should be on GNU ELPA soon. Use it to test the new approach (which is very broken still).

@mkcms

This comment has been minimized.

Copy link
Collaborator

mkcms commented Dec 13, 2018

Use it to test the new approach (which is very broken still).

It's pretty cool. Although it messes up my display, because it "splits" the line in two to show the lens there. This happens when the end of a lens region is not the end of some line.

@Sarcasm

This comment has been minimized.

Copy link

Sarcasm commented Dec 13, 2018

Out of curiosity, would you mind sharing a screenshot of what it looks like?

@mkcms

This comment has been minimized.

Copy link
Collaborator

mkcms commented Dec 13, 2018

@Sarcasm

Out of curiosity, would you mind sharing a screenshot of what it looks like?

Here's screenshot of my version. @joaotavora's looks a bit different because of the display issue I described above.

screenshot from 2018-12-13 21-37-34

@MaskRay

This comment has been minimized.

Copy link
Contributor

MaskRay commented Dec 14, 2018

The screenshot looks great, however, the code lenses occupy some vertical space. Putting code lenses at line ends is better IMHO. I've experimented with the idea before but my elisp-fu isn't strong enough to tame display properties https://www.reddit.com/r/emacs/comments/9k3rf6/put_a_codelens_overlay_sticking_in_the_end_of_the/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment