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.

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

Use F3 to switch between "Open Definition" and "Open Declaration", e.g. for switching between header and cpp files in C++ #1104

Open
travkin79 opened this issue Sep 11, 2024 · 4 comments

Comments

@travkin79
Copy link
Contributor

travkin79 commented Sep 11, 2024

I'd like to slightly change how LSP4E collects and uses definitions, declarations, implementations and type definitions for a cursor position or for a selection in an LSP-based text editor. My goal is the following: For C++ editors (in CDT LSP), by pressing F3 a user should jump to the definition or to the declaration of the element under cursor and by pressing F3 multiple times just jump from a method's declaration (usually in a header file) to its definition (usually in a cpp file) and vice versa. In general, F3 should not just pick the first element from the list of all definitions, declarations, type definitions, and implementations, but at least select one of these that the cursor is not already placed upon.

At the moment, pressing F3 always opens the first definition (or declaration or type definition or implementation) found, even if the cursor is already placed on that definition.

My proposal is to detect the element that the cursor is placed upon and place that element (a definition or declaration) to the end of the list of all definitions and declarations, then followed by the type definitions and implementations.

I.e., the only thing that would change is the order of the definitions, declarations, type definitions and implementations in the list that is used to pick the element to jump to (the first element from that list is used as target). The same list is used when using Ctrl + click to select an element to jump to. Since we wouldn't remove any element from that list, that change should work for both use cases.

This issue might be related to issue #332.

@mickaelistria
Copy link
Contributor

My proposal is to detect the element that the cursor is placed upon and place that element (a definition or declaration) to the end of the list of all definitions and declarations, then followed by the type definitions and implementations.

While I think this might be a good idea here and there, I'm afraid it will not be an expected behavior for the target audience: Jumping to implementation when triggering the "Go to definition" operation would seem rather unexpected. At least in Eclipse IDE with JDT, it would really feel strange and could be perceived as a bug.

@travkin79
Copy link
Contributor Author

travkin79 commented Sep 11, 2024

Hi @mickaelistria,

Jumping to implementation when triggering the "Go to definition" operation would seem rather unexpected.

I think, you probably got me wrong.

Today, LSP4E creates a list of all potential link targets for the element under cursor. That list is built as follows (see org.eclipse.lsp4e.operations.declaration.OpenDeclarationHyperlinkDetector):

  1. list of all found definitions for current cursor position
  2. list of all found declarations for current cursor position
  3. list of all found type definitions for current cursor position
  4. list of all found implementations for current cursor position

These lists are combined in one list containing all the elements. When hitting F3, LSP4E selects the first element from that list. Sometimes it selects the element the the cursor is already on.

My proposal is to change the order as follows:

  1. list of all found definitions for current cursor position except of those that the cursor is on
  2. list of all found declarations for current cursor position except of those that the cursor is on
  3. list of all found definitions for current cursor position where the cursor is on (usually only one or no element)
  4. list of all found declarations for current cursor position where the cursor is on (usually only one or no element)
  5. list of all found type definitions for current cursor position
  6. list of all found implementations for current cursor position

As you see, the implementations and type definitions are already contained in the list of all results. I didn't plan to change that. I only propose to move the definitions and declarations that are known to be under the cursor to the end of all definitions and declarations. This way, if a user has a cursor on a method definition in a cpp file, he/she would jump to the corresponding declaration in a hpp file and vice versa, without loosing the remaining link targets or their order when using the same code for Ctrl + click navigation.

@mickaelistria
Copy link
Contributor

When hitting F3, LSP4E selects the first element from that list.

IIRC, it's supposed to always put definition first.

@travkin79
Copy link
Contributor Author

travkin79 commented Sep 11, 2024

Dear @mickaelistria,

If (for some reason) it's desired to always put all definitions first, do you have an idea how LSP4E could allow e.g. CDT LSP to change that particular behavior in such a way that (similar to CDT) F3 jumps from declaration to definition and vice versa?

Besides, my assumption was that removing the definition / declaration that the cursor is already on is not desired. Is that correct? Otherwise, it might be an option to just remove these from the result list, without changing any order.

I also checked the hyperLinkDetector extension point (org.eclipse.ui.workbench.texteditor.hyperlinkDetectors). It seems that the hyperLinkDetector provided by LSP4E cannot be replaced with another hyperLinkDetector, e.g. from CDT LSP. Thus, it seems to be difficult to change the default behavior from LSP4E.

Another idea:
In case of F3 being pressed, a method findFirstHyperlink(IRegion) is called in code from org.eclipse.ui.internal.genericeditor.ExtensionBasedTextEditor. Could we replace the following code in this method

boolean canShowMultipleHyperlinks = fHyperlinkPresenter.canShowMultipleHyperlinks();
IHyperlink[] hyperlinks = detector.detectHyperlinks(this, region, canShowMultipleHyperlinks);

with this? (see this code in ExtensionBasedTextEditor)

IHyperlink[] hyperlinks = detector.detectHyperlinks(this, region, false);

In findFirstHyperlink(IRegion), we request only one hyper link anyway. This way, the hyperlink detector, e.g. in LSP4E, could decide which of the hyperlink candidates it returns and could return the definition if the cursor is on the declaration and vice versa.

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

No branches or pull requests

2 participants