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

'Run Selected Text' is not working when a task is stopped #7323

Closed
RomanNikitenko opened this issue Mar 11, 2020 · 10 comments · Fixed by #7453
Closed

'Run Selected Text' is not working when a task is stopped #7323

RomanNikitenko opened this issue Mar 11, 2020 · 10 comments · Fixed by #7453
Assignees
Labels
bug bugs found in the application help wanted issues meant to be picked up, require help tasks issues related to the task system terminal issues related to the terminal

Comments

@RomanNikitenko
Copy link
Contributor

RomanNikitenko commented Mar 11, 2020

Description

'Run Selected Text' action is not working when current terminal widget is completed task widget.

Reproduction Steps

  1. Run any task
  2. Stop the task
  3. Select a command and use Run Selected Text action

runSelected

OS and Theia version:
Ubuntu 18.04.1
Theia: ced3f90

Diagnostics:
I guess Run Selected Text action tries to use stopped task as current terminal widget.

@akosyakov akosyakov added bug bugs found in the application help wanted issues meant to be picked up, require help tasks issues related to the task system terminal issues related to the terminal labels Mar 12, 2020
@elaihau
Copy link
Contributor

elaihau commented Mar 17, 2020

I did some investigation, and found an intesting behavior in vs code: "Run Selected text" simply echos the selected text in the current terminal which has one task actively running, instead of running the selected text as a command (See the attached GIF). Is it the correct behavior ?

In my opninion, the "selected text" should always be run in an interactive terminal, no exceptions. If there is a task running in the current terminal, we should find an interactive terminal, or create an interactive terminal to execute the selected text as a command.

What do you think? @RomanNikitenko @vince-fugnitto @akosyakov @eclipse-theia/task-extension

Peek 2020-03-16 22-41

@vince-fugnitto
Copy link
Member

In my opinion, the "selected text" should always be run in an interactive terminal, no exceptions.

I agree, I find it odd that vscode would use a busy terminal to display output of “run selected text”. In my opinion, the output would be lost in all the noise of a task which is currently under execution. I think we should opt for the simplest solution which is to look for an idle interactive terminal to run the text or create a terminal if none exist.

@RomanNikitenko
Copy link
Contributor Author

In my opninion, the "selected text" should always be run in an interactive terminal, no exceptions. If there is a task running in the current terminal, we should find an interactive terminal, or create an interactive terminal to execute the selected text as a command.

Agree.
For 'Run Selected Text' we should use:

  • an idle terminal, if it exists
  • create a terminal if it does not exist

@elaihau
Copy link
Contributor

elaihau commented Mar 18, 2020

would this solution work for you?

  • "Run selected text" runs the selected string as a command in interactive terminals
  • If no terminals are interactive, run it in a new interactive terminal. Please note, in the GIF below, the terminal that ran "long running task" wasn't an interactive terminal and therefore we created a new one.

Peek 2020-03-17 21-41

@akosyakov
Copy link
Member

What it a definition of the idle terminal If i don't know about tasks?

@elaihau
Copy link
Contributor

elaihau commented Mar 18, 2020

What it a definition of the idle terminal If i don't know about tasks?

@akosyakov
as you know, "idle" is not a property of the terminal. so we have no idea if a terminal is idle.

that's why I proposed the solution of "only running selected text in an interactive terminal"

and we could use terminalWidget.hasRunningProcesses() to check if an interactive terminal is busy.

What do you think?

@akosyakov
Copy link
Member

and we could use terminalWidget.hasRunningProcesses() to check if an interactive terminal is busy.

Terminal which does not run any task also has a running process (shell). How is it related to terminal be interactive? How do you define terminal interactive? That it can accept a user input? How could you check that a terminal process can accept a user input?

@akosyakov
Copy link
Member

akosyakov commented Mar 19, 2020

  • I don't really think terminal can tell a user whether it is interactive or not. Whether script running in a terminal can accept user input or not it is a property of a script, not a terminal process. And since terminals are only operating process streams it is abstracted away from concrete scripts. I would say it is responsibility of a terminal client to know whether something running in the terminal is interactive or not, i.e. a task can be interactive.
  • I am not sure what interactive is correct term even for tasks. How do you know whether a concrete task needs user input or not?
  • So if we cannot use interactive, we are back to idle/busy concept. For this concept similarly as for interactive we cannot know it for terminal, but for a task is more clear. A task is either still running or finished.
  • We have busy concept already for other widgets in Theia: monaco editors and trees. In the editor the peek widget can be busy for example while resolving references and in the tree a concrete node while it is resolving child nodes. In both cases there is API allowing to change busy state, so maybe we add something similar to terminal:
export interface TerminalWidget {
   busy: boolean;
   readonly onDidChangeBusy: Event<boolean>;
}
  • Whenever a task extension is running a task in the terminal it should set busy state and then it is done remove it.
  • Having busy state we can implement this issue following such logic:
    • if the current terminal is not busy, run command in it
    • it it is busy, create a new terminal
      • I don't think revealing some random hidden terminal to run a task is a good idea.
  • I think 'Run Selected Text' is not working when a task is stopped #7323 (comment) is most appropriate.

@akosyakov
Copy link
Member

akosyakov commented Mar 19, 2020

Or maybe we can introduce something like TerminalType with System and User kind, defaults to User. System kind means that a terminal was created by a product for some reasons and cannot be used to run random user scripts, e.g. for tasks. Then for this issue we will check whether a current terminal is User terminal and if not create a new User terminal. In both cases terminals can be interactive, i.e. if a task requires some input from a user.

I actually like it more, since it looks like terminals running task should disallow running a random user program and busy won't help with it. But System kind can resolve both issues.

@akosyakov
Copy link
Member

terminal.kind will be introduced in #7260. After it merged, this PR should be fixable by respecting only user terminals.

elaihau added a commit to elaihau/theia that referenced this issue Mar 26, 2020
- The selected texts are only run in a user terminal.
  If the terminal widget that has the focus
  1) is not a user terminal, or
  2) is a user terminal that has an actively running command,
  Theia should create a new user terminal and run the selected texts in
that new terminal.

- fixes eclipse-theia#7323

Signed-off-by: Liang Huang <lhuang4@ualberta.ca>
elaihau added a commit that referenced this issue Mar 30, 2020
- The selected texts are only run in a user terminal.
  If the terminal widget that has the focus
  1) is not a user terminal, or
  2) is a user terminal that has an actively running command,
  Theia should create a new user terminal and run the selected texts in that new terminal.

- fixes #7323

Signed-off-by: Liang Huang <lhuang4@ualberta.ca>
elaihau added a commit that referenced this issue Mar 30, 2020
- The selected texts are only run in a user terminal.
  If the terminal widget that has the focus
  1) is not a user terminal, or
  2) is a user terminal that has an actively running command,
  Theia should create a new user terminal and run the selected texts in that new terminal.

- fixes #7323

Signed-off-by: Liang Huang <lhuang4@ualberta.ca>
elaihau added a commit that referenced this issue Apr 6, 2020
- The selected texts are only run in a user terminal.
  If the terminal widget that has the focus
  1) is not a user terminal, or
  2) is a user terminal that has an actively running command,
  Theia should create a new user terminal and run the selected texts in that new terminal.

- fixes #7323

Signed-off-by: Liang Huang <lhuang4@ualberta.ca>
elaihau added a commit that referenced this issue Apr 6, 2020
- The selected texts are only run in a user terminal.
  If the terminal widget that has the focus
  1) is not a user terminal, or
  2) is a user terminal that has an actively running command,
  Theia should create a new user terminal and run the selected texts in that new terminal.

- fixes #7323

Signed-off-by: Liang Huang <lhuang4@ualberta.ca>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug bugs found in the application help wanted issues meant to be picked up, require help tasks issues related to the task system terminal issues related to the terminal
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants