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

align how tasks are displayed as quick open items #7392

Merged
merged 1 commit into from
Mar 24, 2020

Conversation

elaihau
Copy link
Contributor

@elaihau elaihau commented Mar 22, 2020

  • In a multi root workspace, detected tasks are displayed in different
    ways across Run task, Restart running task, Terminate task,
    and Show running tasks. This change aligns the display of the task
    quick open items.

  • fixed Align task items  #6821

Signed-off-by: Liang Huang [email protected]

How to test

  1. Open a workspace that has multiple roots.
  2. Define a detected task, e.g., a npm script in your package.json
  "scripts": {
    "test": "echo start && sleep 1000 && echo end"
  },
  1. Start the detected task defined in Step 2
  2. Check how the task defined in Step 2 is displayed in the following quick open menus:

Terminal -> Run task, Terminal -> Restart running task, Terminal -> Terminate task,
and Terminal -> Show running tasks.

They should all be displayed as npm: test <- subfolder name / path> [root folder name]

Peek 2020-03-22 11-49

Review checklist

Reminder for reviewers

@elaihau elaihau force-pushed the Liang/alignTaskOpenItem branch from 789245b to 2043d91 Compare March 22, 2020 15:54
@vince-fugnitto vince-fugnitto added tasks issues related to the task system enhancement issues that are enhancements to current functionality - nice to haves labels Mar 23, 2020
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.

The changes look good to me, and they work as intended for a multi-root workspace.

I am able to see the source, the subfolder name (if present), and the root name when performing:

  • run task
  • show running task
  • restart running task
  • terminate task

I did notice that attach task did not have the same formatting as others however, but I'm unsure if it is in the scope of this pull-request:

Screen Shot 2020-03-23 at 10 14 51 AM

@elaihau elaihau force-pushed the Liang/alignTaskOpenItem branch from 2043d91 to 1817687 Compare March 23, 2020 20:48
@elaihau
Copy link
Contributor Author

elaihau commented Mar 23, 2020

The changes look good to me, and they work as intended for a multi-root workspace.

I am able to see the source, the subfolder name (if present), and the root name when performing:

  • run task
  • show running task
  • restart running task
  • terminate task

I did notice that attach task did not have the same formatting as others however, but I'm unsure if it is in the scope of this pull-request:

Thank you for the review @vince-fugnitto !

The "attach task" menu items should definitely be updated as well.
I updated the pull request.

}
if (task.terminalId) {
this.taskService.attach(task.terminalId, task.taskId);
}
Copy link
Contributor

@RomanNikitenko RomanNikitenko Mar 24, 2020

Choose a reason for hiding this comment

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

btw, the question is not related to your PR, but maybe you have an answer:
To be honest, I didn't use this action and I don't see Terminal -> Attach Task action in VS Code.
Tried to use the action in Theia:

  • quick-open items are created for running tasks with terminalId, so, I guess, a terminal widget already open for such tasks
  • it creates one more terminal for a running task

What's the use case for using this action?

thanks in advance!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is used in 2 cases in my opinion

  1. the user closed the browser for some reason, when s/he reopens the workspace s/he wants to reconnect to the task
  2. the user wants someone that opens the same workspace to see the task output

Copy link
Contributor

@RomanNikitenko RomanNikitenko Mar 24, 2020

Choose a reason for hiding this comment

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

Why I asked:
it's strange for me that it creates one more terminal:

  • we give terminalId, taskId
  • attach method asks widgetManager get or create widget.

So, it's expected to get existed widget, but it creates a new one.

OK, anyway it's not related to the current PR.
thank you for the answer!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why I asked:
it's strange for me that it creates one more terminal:

  • we give terminalId, taskId
  • attach method asks widgetManager get or create widget.

So, it's expected to get existed widget, but it creates a new one.

yea i filed an issue for the problem you describe: #7407

i think:

  1. we should set focus to the teminal widget if there is already a terminal widget
  2. we should create a new widget if we couldn't find that widget - this is likely to happen, as the localstorage could possibly be cleared, or users use a different machine to access the same workspace

@RomanNikitenko RomanNikitenko self-requested a review March 24, 2020 16:57
Copy link
Contributor

@RomanNikitenko RomanNikitenko left a comment

Choose a reason for hiding this comment

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

Tested for tsc detected and configured tasks for single and multi root workspace as well:

  • labels for items for Terminate Task, Show Running Tasks, Restart Running Task, Attach Task actions are aligned
  • I don't see any regression

thanks @elaihau!

- In a multi root workspace, detected tasks are displayed in different
ways across `Run task`, `Restart running task`, `Terminate task`,
and `Show running tasks`. This change aligns the display of the task
quick open items.
- fixed #6821

Signed-off-by: Liang Huang <[email protected]>
@elaihau elaihau force-pushed the Liang/alignTaskOpenItem branch from 1817687 to ceb7a31 Compare March 24, 2020 17:06
@elaihau elaihau merged commit 8152c55 into master Mar 24, 2020
@elaihau elaihau deleted the Liang/alignTaskOpenItem branch March 24, 2020 23:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement issues that are enhancements to current functionality - nice to haves tasks issues related to the task system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Align task items
3 participants