Give more flexibility for tasks related sub classes

[RomanNikitenko]

What it does

1. Replace private modifier by protected for TaskTerminalWidgetManager and TaskService methods.
2. Add separated method as ability to create task terminal from TaskService for example
3. Some changes for TaskTerminalWidgetOpenerOptions
   I didn't find where required property taskId is used, instead we everywhere use task config.
   So, I moved taskId to optional property and added taskConfig (optional as well)

How to test

I don't think these changes can break something, but you could try:

1. Run any configured task
2. Run any detected task

Review checklist

• as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• as a reviewer, I agree to behave in accordance with the review guidelines

Signed-off-by: Roman Nikitenko rnikiten@redhat.com

[vince-fugnitto]

The changes make sense to me, thank you for the refactoring!

[tsmaeder]

I was just wondering what use case you need the protected methods for: in general, we should be sparse with what we make API.

[RomanNikitenko]

@tsmaeder
it's related to logic: tasks vs remote tasks

[elaihau]

3. Some changes for TaskTerminalWidgetOpenerOptions
   I didn't find where required property taskId is used, instead we everywhere use task config.
   So, I moved taskId to optional property and added taskConfig (optional as well)

Thank you for your work @RomanNikitenko !

2 questions:

• Since taskInfo is an object that has a taskId property, could we simply remove taskId from the interface TaskTerminalWidgetOpenerOptions ?

• Could you please give us a brief introduction of remote task?

Thanks!

[RomanNikitenko]

@elaihau

Since taskInfo is an object that has a taskId property, could we simply remove taskId from the interface TaskTerminalWidgetOpenerOptions ?

taskInfo contains both taskId and config and both are required.
AFAIK we everywhere use task config and according to this and my changes in task-terminal-widget-manager.ts config is really required.
About taskId: I didn't find where it is used.

So it make sense to me to have taskConfig as required property and additional optional properties.
The only reason why I moved taskId to optional property is: I don't know if someone uses it, so to avoid breaking changes.

@akosyakov @elaihau
please let me know your opinion - should we have:

• taskConfig as required property and taskInfo as optional one
  or
• taskConfig and taskInfo as optional
  or
• taskConfig, taskInfo taskIs as optional to avoid breaking changes

@elaihau

Could you please give us a brief introduction of remote task?

We have ability to run tasks in separate containers. For example, user can have editor and java (contains java related tools) containers. So it makes sense to run some specific task in the java container and it's impossible to run this task in editor container because this one doesn't contain some specific tools.
For remote tasks we use remote terminals, that's why I need some flexibility to manage which type of a terminal is created at running task.

[elaihau]

I would vote
taskConfig and taskInfo as optional

I don't see taskId being referenced either. I shouldn't have added it to the interface in the first place. Sorry.

[RomanNikitenko]

@elaihau

I would vote
taskConfig and taskInfo as optional

thank you for the feedback, I removed taskId from TaskTerminalWidgetOpenerOptions.
I'm going to merge my PR tomorrow.

I shouldn't have added it to the interface in the first place. Sorry.

I think you don't need to worry about it. You put much effort into this project and contributed a lot of functionality related to tasks (thanks!) 👍.
I believe it's OK to reconsider some approaches and improve some places from time to time, because a project like a life-form - lives and develops :-)