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

Resolve links to workspace files in terminal #13498

Merged
merged 2 commits into from
Mar 26, 2024

Conversation

AlexandraBuzila
Copy link
Contributor

What it does

Try and provide links for paths pointing to workspace files in the terminal. The links are created by searching for matching files based on the provided path.

Fixes #13047

Contributed on behalf of STMicroelectronics

How to test

Open the theia workspace and echo some partial paths in the terminal. All paths that can be resolved to a workspace file should have valid links that can be used to open the files from the terminal, e.g.:

  • ../../packages/terminal/src/browser/terminal-file-link-provider.ts:17:20
  • src/browser/terminal-file-link-provider.ts

Review checklist

Reminder for reviewers

Copy link
Contributor

@planger planger left a comment

Choose a reason for hiding this comment

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

Thank you @AlexandraBuzila! This definitely improves the file linking in the terminal and the code looks fine to me.

The only thing I miss is that simple file names don't work (e.g. package.json). They only work if they start with ./ as in ./package.json. Is that something we could fix easily? This seems to work in VS Code.

Probably beyond the scope of this PR, but VS Code is a bit more clever when choosing the CWD. VS Code seems to remember the CWD of each command output in the terminal history and keeps it, whereas we switch for the entire terminal. This is nice of course, if you ran a build and then cd'd into another folder, it keeps the correct linking. If this is easy to do (which I don't expect), it may be worth adding. Otherwise, using the current CWD should be good enough for now.

I have added to OPTIONAL remarks inline. If you disagree, feel free to ignore them.

searchTerm = searchTerm.replace(/^(\.+[\\/])+/, '');
// try and find a matching file in the workspace
const files = (await this.searchService.find(searchTerm, {
rootUris: [cwd.toString()],
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder whether it makes sense to add the workspace root(s) here too as a fallback, if no matches are found in CWD?

Try and provide links for workspace files in the terminal, by searching
for matching files.

Contributed on behalf of STMicroelectronics
- match files without a starting separator, e.g. 'yarn.lock:25:2'
@AlexandraBuzila
Copy link
Contributor Author

Thank you @planger for the review!
I pushed a new commit with added support for links to paths that don't have any separators. Could you recheck the code, please?

I also had a look at improving the support for CWD and it wasn't so straightforward. It might be worth investigating this in a separate ticket.

Copy link
Contributor

@planger planger left a comment

Choose a reason for hiding this comment

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

Thank you for the update @AlexandraBuzila! Works great and code looks good. Thank you!

@planger planger merged commit ddb91b9 into eclipse-theia:master Mar 26, 2024
14 checks passed
@planger planger added this to the 1.48.0 milestone Mar 26, 2024
@msujew msujew mentioned this pull request May 29, 2024
1 task
@AlexandraBuzila AlexandraBuzila deleted the fix/terminal-links branch May 29, 2024 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Path that resolve to files in the workspace are not navigatable
2 participants