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

Consider TerminalLinkProvider contributions recursively #15177

Merged
merged 1 commit into from
Mar 19, 2025

Conversation

xpomul
Copy link
Contributor

@xpomul xpomul commented Mar 12, 2025

What it does

Fixes #15050 by considering the TerminalLinkProvider contributions also recursively (i.e., consider TerminalLinkProvider contributions bound in the root container).

For a thorough analysis of the issue see #15050 (comment)

How to test

See #15050 description.
In short:

  • npm run build:browser
  • npm run download:plugins
  • npm run start:browser
  • Start frontend and make sure a folder is opened
  • Command Palette: Terminal: Create new Terminal
  • In the terminal, execute ls -l
  • Try to hover over a file (e.g., package.json)

Without the fix, there is no link provided, with the fix, the link is there and clicking leads to the file being opened in an editor.

Notes:

  1. I am not 100% sure whether this is the correct fix. It could also be a bug that plugin-ext binds the TerminalServiceMainImpl in the child container, not the parent (root) container. So, maybe someone with knowledge in this area should have a second look. (Again, see the linked comment in the issue for details)
  2. I tried to write a unit test but failed because importing the terminal-link-provider from the test results in a phosphor error (ReferenceError: navigator is not defined). So I am not sure how to mock the UI dependencies correctly and I haven't found a similar example/test case in the code base...

Review checklist

Reminder for reviewers

@JonasHelming JonasHelming requested a review from sgraband March 19, 2025 15:33
Copy link
Contributor

@sgraband sgraband left a comment

Choose a reason for hiding this comment

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

LGTM and works 👍 Thank you very much, this bug was quite annoying.

@sgraband sgraband merged commit 47450a1 into eclipse-theia:master Mar 19, 2025
10 of 11 checks passed
@github-actions github-actions bot added this to the 1.60.0 milestone Mar 19, 2025
@mcg1103
Copy link

mcg1103 commented Mar 19, 2025

@xpomul just tested and it is working for me as well. Thanks for looking into this and fixing.

@carstenrd
Copy link

Thank you for the fix, much appricated!

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.

Terminal File Links Not Working in Theia 1.58.x
4 participants