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

The default label for multiple declarations is not useful #973

Closed
ylussaud opened this issue Apr 24, 2024 · 11 comments
Closed

The default label for multiple declarations is not useful #973

ylussaud opened this issue Apr 24, 2024 · 11 comments

Comments

@ylussaud
Copy link
Contributor

The computed label for multiple declarations is the name and the path of the resource with is not really useful as an end user.

It would be better to display the line declaring the symbol so the used can select the relevant entry. This could be done at least for IFile from the workspace since we can have the encoding and the stream to read its content.

ylussaud added a commit to ylussaud/lsp4e that referenced this issue Apr 24, 2024
ylussaud added a commit to ylussaud/lsp4e that referenced this issue Apr 24, 2024
@mickaelistria
Copy link
Contributor

I'm not sure I understand what you're talking about. Can you please submit some screenshot of the particular UI element being discussed?

@rubenporras
Copy link
Contributor

Yes, an screenshot with and without your PR would be very helpful. I know how it currently looks like, but I am not sure about how it would look like after

ylussaud added a commit to ylussaud/lsp4e that referenced this issue Apr 25, 2024
@ylussaud
Copy link
Contributor Author

Before the patch:
image
After the patch:
image
I updated my commit...

@rubenporras
Copy link
Contributor

I am unsure on this change. I see how in some cases the declaration line might be useful, but the URI might be as well quite relevant. Would the problem be solved if the dropdown menu that shows the URI would be wider?

@mickaelistria
Copy link
Contributor

Showing the line content can be helpful, but it IMO usually seems more important to show the target location as well (and not only its content). Also not that in some cases, the first line of the declaration might not be so interesting. It could for example be the start of a comment. The line content might be a nice addition as a suffix, but we'd need more space.
I think we can maybe save some space by replacing the "Open" by an arrow emoji ➡️ , Also we might be able to only show some relative paths when they're shorter; then we could have more room to append line content.

@ylussaud
Copy link
Contributor Author

In my case all declarations are in the same file so there is no way to distinguish a declaration from its URI. Maybe this choice should be done by the language implementation via an extension point ? This extension point could include other labels as well.

@mickaelistria
Copy link
Contributor

choice should be done by the language implementation via an extension point ?

I fear it's a dangerous to have extension points for it, as at such pace, we'd start maintaining dozens of extension points for everything. How does VSCode behave in this case? Maybe adding a "label" to LocationLink in the spec would be the best way forward.

@ylussaud
Copy link
Contributor Author

I understand, in the mean time could:

Declaration - <resource name> - <line content>
Declaration - body.mtl - [template public generateClassifier(cls : ecore::EClassifier)]

be an option ?

@rubenporras
Copy link
Contributor

At least for our use cases, the file name would not be enough, the folders are also relevant.

I wonder, for the cases where the URI is not enough, should one not use type hierarchy? Would that work for you?

@ylussaud
Copy link
Contributor Author

ylussaud commented Apr 26, 2024

No those are declarations not related to types in the language. I'll try to find a workaround...

@ylussaud
Copy link
Contributor Author

I provided my own org.eclipse.ui.workbench.texteditor.hyperlinkDetectors as a workaround. I still have the hyperlink detector from LSP4E but I can live with that for the moment.
For more details one can check this commit.

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

3 participants