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

Enable file links when task terminals complete #9695

Conversation

kenneth-marut-work
Copy link
Contributor

Signed-off-by: Kenneth Marut [email protected]

What it does

Fixes #9677 by ensuring that the terminal-linkmatcher-files's getValidate method does not fail when trying to retrieve the TerminalWidget's cwd field when a task is terminated. This is done by adding a lastCwd field on the terminalWidget which is set every time a taskTerminal is started or reused.

I've added this fix on the frontend although the underlying issue is due to the base-terminal-server's call to this.processManager.get(id) being prone to failure when tasks end (i.e. 'terminal "id" does not exist').

How to test

  • Before pulling this branch, open Theia's master branch and build.
  • Open Theia to a workspace with some files in it
  • Add a task in your workspace that looks something like:
{
  "version": "2.0.0",
  "tasks": [
    {
      "label": "list filepaths",
      "command": "find $(pwd)",
      "type": "shell"
    }
  ]
}
  • Run that task and observe that you cannot ctrl+click on the file paths

  • Run the command find $(pwd) manually in a separate Theia terminal and observe that you can click the paths

  • Exit Theia, and build this branch

  • Run through the same steps and observe that paths are now clickable even after the task ends.

Review checklist

Reminder for reviewers

@kenneth-marut-work kenneth-marut-work added terminal issues related to the terminal tasks issues related to the task system labels Jul 2, 2021
@kenneth-marut-work kenneth-marut-work force-pushed the bug/terminal-get-last-cwd branch from 010d877 to 96a1b61 Compare July 2, 2021 21:30
@rcla
Copy link

rcla commented Jul 2, 2021

@kenneth-marut-work

With the initial changes proposed by you, solve this problem that I have had: (Thanks)
Firing up a Task

However with the last proposed change "switch ordering of fallback"
The same problem appears again

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

I confirmed that it fixes the issue with file links in terminals when executing tasks 👍

@kenneth-marut-work
Copy link
Contributor Author

kenneth-marut-work commented Jul 6, 2021

@rcla

With the initial changes proposed by you, solve this problem that I have had: (Thanks)
Firing up a Task

However with the last proposed change "switch ordering of fallback"
The same problem appears again

Thanks for bringing attention to this issue. The behavior you've described does make sense with the ordering change since prior to the last commit, the backend method was never being called and thus never triggering the error (which is just hiding the underlying issue). Unfortunately, though the original commit did prevent the error from being thrown, it's probably not in the best interest to revert that change since I'm relying on the error being thrown to allow for the fallback inside the catch block.

Maybe a potential fix for the future is to make these errors less invasive and to use warnings instead, however I think it may be out of the scope of this MR since several features of the task/terminal system likely rely on these errors.

@colin-grant-work
Copy link
Contributor

Thanks for addressing my comments. I agree that this likely isn't the place to modify the behavior of the backend error logs: the faulty logic to which they point still exists. The code here allows the frontend to work around the possibility of backend failure, and that's a step forward.

@rcla
Copy link

rcla commented Jul 6, 2021

@kenneth-marut-work @colin-grant-work

Thanks for the explanations, now I understand what concerns the backend errors for the future in another PR.

@kenneth-marut-work kenneth-marut-work force-pushed the bug/terminal-get-last-cwd branch from 96a1b61 to e683989 Compare July 7, 2021 14:50
@kenneth-marut-work kenneth-marut-work force-pushed the bug/terminal-get-last-cwd branch from e683989 to 507dd34 Compare July 7, 2021 15:42
@kenneth-marut-work kenneth-marut-work merged commit dc40f6e into eclipse-theia:master Jul 7, 2021
@vince-fugnitto vince-fugnitto added this to the 1.16.0 milestone Jul 7, 2021
kenneth-marut-work added a commit to kenneth-marut-work/theia that referenced this pull request Jul 15, 2021
Fixes eclipse-theia#9743 by improving check for taskConfig.options.cwd from eclipse-theia#9695.
Also ensures that lastCwd field on terminal-widget is never undefined
as to ensure get cwd() will always return a URI.

Signed-off-by: Kenneth Marut <[email protected]>
tsmaeder pushed a commit that referenced this pull request Jul 16, 2021
Fixes #9743 by improving check for taskConfig.options.cwd from #9695.
Also ensures that lastCwd field on terminal-widget is never undefined
as to ensure get cwd() will always return a URI.

Signed-off-by: Kenneth Marut <[email protected]>
dna2github pushed a commit to dna2fork/theia that referenced this pull request Aug 25, 2021
Fixes eclipse-theia#9743 by improving check for taskConfig.options.cwd from eclipse-theia#9695.
Also ensures that lastCwd field on terminal-widget is never undefined
as to ensure get cwd() will always return a URI.

Signed-off-by: Kenneth Marut <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tasks issues related to the task system terminal issues related to the terminal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Link detection for files not working in task terminals
4 participants