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

Document or add `xref-find-references` to `xref-prompt-for-identifier` #194

Closed
MaskRay opened this Issue Nov 24, 2017 · 15 comments

Comments

Projects
None yet
5 participants
@MaskRay
Member

MaskRay commented Nov 24, 2017

(defcustom xref-prompt-for-identifier '(not xref-find-definitions
                                            xref-find-definitions-other-window
                                            xref-find-definitions-other-frame)
  "When t, always prompt for the identifier name.

When nil, prompt only when there's no value at point we can use,
or when the command has been called with the prefix argument.

Otherwise, it's a list of xref commands which will prompt
anyway (the value at point, if any, will be used as the default).

If the list starts with `not', the meaning of the rest of the
elements is negated."
..........

By default, xref-find-definitions does not prompt but xref-find-references does. When point is at a symbol, the prompting behavior is undesired. Some language clients may make use of text properties to find references. If the pattern is taken from the prompt instead of (word-at-point) or such, there is no text property associated with the pattern and the language client may not be able to find references.

There are probably three approaches:

  • Do not suggest using xref-find-references and add a custom lsp-find-references
  • Add xref-find-references to xref-prompt-for-identifier
  • Document that xref-prompt-for-identifier should be changed
@MaskRay

This comment has been minimized.

Show comment
Hide comment
@MaskRay

MaskRay Nov 24, 2017

Member

I found another hack in my config and I think having a custom lsp-find-references is the best

    ;; This is required to make xref-find-references work in helm-mode.
    ;; In helm-mode, it gives a prompt and asks the identifier (which has no text property) and then passes it to lsp-mode, which requires the text property at point to locate the references.
    (setq xref-prompt-for-identifier '(not xref-find-definitions xref-find-definitions-other-window xref-find-definitions-other-frame xref-find-references spacemacs/jump-to-definition spacemacs/jump-to-reference))

    (setq xref-show-xrefs-function 'helm-xref-show-xrefs)
Member

MaskRay commented Nov 24, 2017

I found another hack in my config and I think having a custom lsp-find-references is the best

    ;; This is required to make xref-find-references work in helm-mode.
    ;; In helm-mode, it gives a prompt and asks the identifier (which has no text property) and then passes it to lsp-mode, which requires the text property at point to locate the references.
    (setq xref-prompt-for-identifier '(not xref-find-definitions xref-find-definitions-other-window xref-find-definitions-other-frame xref-find-references spacemacs/jump-to-definition spacemacs/jump-to-reference))

    (setq xref-show-xrefs-function 'helm-xref-show-xrefs)
@vibhavp

This comment has been minimized.

Show comment
Hide comment
@vibhavp

vibhavp Nov 26, 2017

Member

My plan with lsp-mode is to simply provide a framework for interacting with language servers. IMO, stuff like lsp-find-references would best fit in another package, like company-lsp.

Member

vibhavp commented Nov 26, 2017

My plan with lsp-mode is to simply provide a framework for interacting with language servers. IMO, stuff like lsp-find-references would best fit in another package, like company-lsp.

@MaskRay

This comment has been minimized.

Show comment
Hide comment
@MaskRay

MaskRay Nov 26, 2017

Member

Do you think a separate package called lsp-xref might be a good place to implement these features? Here is a list of features that I have seen or thought of that can improve the code navigation experience greatly.

I have been missing these features in C++ development for years and until recently with your lsp-mode
and https://github.com/jacobdufault/cquery project I feel Emacs have become on par with other IDEs. (GNU Global with helm-gtags or ggtags make it closer, but still not competent) I'd like to see these code navigation features being implemented in some package. And to me these features naturally fit into lsp-mode (the counterpart of lsp-mode in the neovim world LanguageClient-neovim is more or less doing the same thing).

Member

MaskRay commented Nov 26, 2017

Do you think a separate package called lsp-xref might be a good place to implement these features? Here is a list of features that I have seen or thought of that can improve the code navigation experience greatly.

I have been missing these features in C++ development for years and until recently with your lsp-mode
and https://github.com/jacobdufault/cquery project I feel Emacs have become on par with other IDEs. (GNU Global with helm-gtags or ggtags make it closer, but still not competent) I'd like to see these code navigation features being implemented in some package. And to me these features naturally fit into lsp-mode (the counterpart of lsp-mode in the neovim world LanguageClient-neovim is more or less doing the same thing).

@topisani

This comment has been minimized.

Show comment
Hide comment
@topisani

topisani Nov 26, 2017

Member

maybe we turn some of the cquery stuff into a generic lsp-ui package? i definately understand why @vibhavp wants to keep lsp-mode as mostly a library, i think thats the power of it.

Here are some of the things id like to put in cquery, which could go in a generic lsp-ui package:

codelens (WIP)
code actions (WIP)
peek, which will make for a great xref ui
popup docs on completion

Member

topisani commented Nov 26, 2017

maybe we turn some of the cquery stuff into a generic lsp-ui package? i definately understand why @vibhavp wants to keep lsp-mode as mostly a library, i think thats the power of it.

Here are some of the things id like to put in cquery, which could go in a generic lsp-ui package:

codelens (WIP)
code actions (WIP)
peek, which will make for a great xref ui
popup docs on completion

@sebastiencs

This comment has been minimized.

Show comment
Hide comment
@sebastiencs

sebastiencs Nov 26, 2017

Member

@topisani That would be great. It could be the right place for this ?

Member

sebastiencs commented Nov 26, 2017

@topisani That would be great. It could be the right place for this ?

@topisani

This comment has been minimized.

Show comment
Hide comment
@topisani

topisani Nov 26, 2017

Member

@sebastiencs oooh you are the sorcerer behind that... yeah definately, its awesome! I could set up a lsp-ui package for this stuff, the only thing is, id really like to have it MIT. I know that blocks it from ending up in emacs at some point, but i personally have very little motivation to get it there. I feel like it would be too high level to belong in core anyway.
Does anyone (@sebastiencs ?) have a problem with that?

Member

topisani commented Nov 26, 2017

@sebastiencs oooh you are the sorcerer behind that... yeah definately, its awesome! I could set up a lsp-ui package for this stuff, the only thing is, id really like to have it MIT. I know that blocks it from ending up in emacs at some point, but i personally have very little motivation to get it there. I feel like it would be too high level to belong in core anyway.
Does anyone (@sebastiencs ?) have a problem with that?

@terlar

This comment has been minimized.

Show comment
Hide comment
@terlar

terlar Nov 26, 2017

There is a package for peeking source-peek, perhaps it could be integrated with LSP support

terlar commented Nov 26, 2017

There is a package for peeking source-peek, perhaps it could be integrated with LSP support

@vibhavp

This comment has been minimized.

Show comment
Hide comment
@vibhavp

vibhavp Nov 26, 2017

Member

I could set up a lsp-ui package for this stuff, the only thing is, id really like to have it MIT. I know that blocks it from ending up in emacs at some point, but i personally have very little motivation to get it there. I feel like it would be too high level to belong in core anyway.

Yep! If anything, I'd prefer an ecosystem of source code navigation frontends which happen to use lsp-mode behind the scenes. The only thing that emacs would provide would be the library for communicating with language servers, and nothing else.

Member

vibhavp commented Nov 26, 2017

I could set up a lsp-ui package for this stuff, the only thing is, id really like to have it MIT. I know that blocks it from ending up in emacs at some point, but i personally have very little motivation to get it there. I feel like it would be too high level to belong in core anyway.

Yep! If anything, I'd prefer an ecosystem of source code navigation frontends which happen to use lsp-mode behind the scenes. The only thing that emacs would provide would be the library for communicating with language servers, and nothing else.

@MaskRay

This comment has been minimized.

Show comment
Hide comment
@MaskRay

MaskRay Nov 26, 2017

Member

Having a lsp-ui package for (definitions/references/codeLens/.....) looks great!

Member

MaskRay commented Nov 26, 2017

Having a lsp-ui package for (definitions/references/codeLens/.....) looks great!

@topisani

This comment has been minimized.

Show comment
Hide comment
@topisani

topisani Nov 26, 2017

Member

@MaskRay yeah, especially since a lot of it will not actually be specific to lsp mode, but some xref extensions etc

Member

topisani commented Nov 26, 2017

@MaskRay yeah, especially since a lot of it will not actually be specific to lsp mode, but some xref extensions etc

@sebastiencs

This comment has been minimized.

Show comment
Hide comment
@sebastiencs

sebastiencs Nov 26, 2017

Member

Sound good !

Does anyone (@sebastiencs ?) have a problem with that?

I am totally ok with that.
I don't think it need to be in emacs either

Member

sebastiencs commented Nov 26, 2017

Sound good !

Does anyone (@sebastiencs ?) have a problem with that?

I am totally ok with that.
I don't think it need to be in emacs either

@MaskRay

This comment has been minimized.

Show comment
Hide comment
@MaskRay

MaskRay Nov 26, 2017

Member

If all are fine with a separate package lsp-ui, could the owner please set up https://github.com/emacs-lsp/lsp-ui ?

Member

MaskRay commented Nov 26, 2017

If all are fine with a separate package lsp-ui, could the owner please set up https://github.com/emacs-lsp/lsp-ui ?

@vibhavp

This comment has been minimized.

Show comment
Hide comment
@vibhavp

vibhavp Nov 28, 2017

Member

Done: https://github.com/emacs-lsp/lsp-ui'

We could start by shifting flycheck support to lsp-ui

Member

vibhavp commented Nov 28, 2017

Done: https://github.com/emacs-lsp/lsp-ui'

We could start by shifting flycheck support to lsp-ui

@vibhavp

This comment has been minimized.

Show comment
Hide comment
@vibhavp

vibhavp Dec 8, 2017

Member

Now that lsp-ui exists, it should be okay to close this.

Member

vibhavp commented Dec 8, 2017

Now that lsp-ui exists, it should be okay to close this.

@vibhavp vibhavp closed this Dec 8, 2017

@MaskRay

This comment has been minimized.

Show comment
Hide comment
@MaskRay

MaskRay Dec 9, 2017

Member

Reported upstream https://debbugs.gnu.org/cgi/bugreport.cgi?bug=29619
If you feel the same pain, please report what you feel on that bug.

> Currently, xref-find-references prompts for the identifier to look up,
> because it is not listed in xref-prompt-for-identifier:
> 
> (defcustom xref-prompt-for-identifier '(not xref-find-definitions
>                                              xref-find-definitions-other-window
>                                              xref-find-definitions-other-frame)
> 
> It will be much core convenient to add xref-find-references in the list.

We've discussed it before, and for now have settled on this list of 
non-prompting functions.

But thank you for your report. We may change the default if more people 
feel the same.
Member

MaskRay commented Dec 9, 2017

Reported upstream https://debbugs.gnu.org/cgi/bugreport.cgi?bug=29619
If you feel the same pain, please report what you feel on that bug.

> Currently, xref-find-references prompts for the identifier to look up,
> because it is not listed in xref-prompt-for-identifier:
> 
> (defcustom xref-prompt-for-identifier '(not xref-find-definitions
>                                              xref-find-definitions-other-window
>                                              xref-find-definitions-other-frame)
> 
> It will be much core convenient to add xref-find-references in the list.

We've discussed it before, and for now have settled on this list of 
non-prompting functions.

But thank you for your report. We may change the default if more people 
feel the same.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment