[vscode] support TaskPresentationOptions close #82
Conversation
|
As a note: changelog will be updated when pushed to official repo. |
| terminal.busy = false; | ||
| terminal.scrollToBottom(); | ||
| if (showReuseMessage) { | ||
| terminal.writeLine('\x1b[1m\n\rTerminal will be reused by tasks. \x1b[0m\n'); | ||
| } | ||
| if (closeOnFinish) { |
There was a problem hiding this comment.
I don't like that the terminal is closed in a message that is names notifyTaskFinished. A reader would not expect that from the name. Why not just do this at the (single) call site?
There was a problem hiding this comment.
I vaguely remember that we reuse terminals for running tasks in some cases. If my recollection is correct, shouldn't we check if this is a shared terminal and keep it open in that case?
There was a problem hiding this comment.
Indeed, thanks for pointing out!
I renamed to a more generic term on handling the terminal on task exit, not only notifying the task was finished. That should be more readable from a developer point of view.
For the reuse terminal options, I tend to think that closing the terminal has a higher priority than sharing it with other tasks. I checked on VS Code, the behavior is coherent. Their close option prevails over the shared/dedicated option.
| @@ -93,7 +93,8 @@ export class TaskTerminalWidgetManager { | |||
| for (const terminal of this.getTaskTerminalWidgets()) { | |||
| if (terminal.taskId === finishedTaskId) { | |||
| const showReuseMessage = !!event.config && TaskOutputPresentation.shouldShowReuseMessage(event.config); | |||
| this.notifyTaskFinished(terminal, showReuseMessage); | |||
| const closeOnFinish = !!event.config && TaskOutputPresentation.shouldCloseTerminalOnFinish(event.config); | |||
There was a problem hiding this comment.
It seems to me that if closeOnFinish defaults to true in TaskOutputPresentation.shouldCloseTerminalOnFinish. Is that the right default?
There was a problem hiding this comment.
Yes, the default was supposed to be 'false' and was badly implemented. Fixed with new patch.
rschnekenbu
force-pushed
the
issues/12611
branch
2 times, most recently
from
3e60edd to
452ec93
Compare
|
Thanks a lot for your review, Thomas! I pushed a new version of the branch, rebased on latest master. That should address the comments |
* Adds close to the TaskPresentationOptions * Checks when task is finished if terminal shall be closed Contributed on behalf of STMicroelectronics
rschnekenbu
force-pushed
the
issues/12611
branch
from
452ec93 to
43b3de3
Compare
|
Thanks @tsmaeder for the review!
|
What it does
Adds the close property to TaskPresentationOptions
This enables the possibility to close the terminal once the task is finished.
Fixes eclipse-theia#12611
Contributed on behalf of ST Microelectronics
How to test
Copy the content of this archive at the root of your workspace:
workspace-test.zip. It contains several tasks that can be identified by the sample extension. In the extension, the build tasks are supposed to close the terminal after the task is finished (
closeset totrue) whereas the terminal should remain on completion for other tasks like test tasks.On
mastertheia, the terminal always remain after the tasks are finished (this is also the default behavior after the patch whencloseis not defined.). The extension is written so the tasks are configured differently if they are build tasks or any other kind of tasks. This does not do any difference onmaster, as the close property is not known.Switch to this PR and run the same tests. The terminal will close for the build tasks, and remain open for reuse for the other tasks like test tasks.
Review checklist
Reminder for reviewers