Update message about task start/stop #5296
Conversation
vzhukovs
added
enhancement
| @@ -97,6 +97,9 @@ export interface TaskExitedEvent { | |||
| // Exactly one of code and signal will be set. | |||
| readonly code?: number; | |||
| readonly signal?: string; | |||
|
|
|||
| readonly label: string; | |||
There was a problem hiding this comment.
@vzhukovskii shouldn't the new label and processType properties be optional?
There was a problem hiding this comment.
Don't think so, in TaskConfiguration these fields are mandatory.
There was a problem hiding this comment.
I was just pointing out that making them mandatory means the update to the TaskExitedEvent is a breaking change.
Perhaps we can instead keep an optional config: TaskConfiguration property?
There was a problem hiding this comment.
Updated PR, now there is only optional config field.
There was a problem hiding this comment.
Thanks, do you mind squashing your commits into a single commit?
There was a problem hiding this comment.
Sure, I'm going to squash all changes into a single commit before merge. Just waiting for other feedbacks.
|
@elaihau please review |
|
Should not be there a terminal associated with each task where a user can track progress? Why do we need such notifications then? They are a bit annoying. |
|
@elaihau @RomanNikitenko i would prefer if we implement #5269 (comment) instead. It would align with VS Code instead of spamming with notification. |
| @@ -97,6 +97,8 @@ export interface TaskExitedEvent { | |||
| // Exactly one of code and signal will be set. | |||
| readonly code?: number; | |||
| readonly signal?: string; | |||
|
|
|||
| readonly config?: TaskConfiguration; | |||
There was a problem hiding this comment.
i know that having an optional property does not break API. but is it good to make config optional? all exited tasks have a TaskConfiguration, so it does not look optional conceptually. what do you think @akosyakov ?
@vince-fugnitto agreed with you, Anton. Also @vince-fugnitto suggested that we probably could move theia notifications, which for now show up at the top, to the bottom right to match the behaviour of VsCode. From there we could even allow users to view the notification history and perform a search. What do you think? @RomanNikitenko @akosyakov |
|
@akosyakov Looks like in this PR we don't change Theia behavior but change message in current notification. So can we go with it for now? |
I'm not sure if the comments have been addressed, @elaihau can you clarify? |
@vince-fugnitto thank you for pointing it out. @vparfonov The problem I found in my testing has not been fixed yet. |
Yes, thanks I understand now |
Signed-off-by: Vladyslav Zhukovskyi <vzhukovs@redhat.com>
vzhukovs
force-pushed
the
task_message_update
branch
from
fdcf442 to
6e93774
Compare
|
@akosyakov can I merge this PR? |
According to issue there is a changes proposal to update message that indicates about task start/stop operation. Changes replaces task number with human readable name.
Signed-off-by: Vladyslav Zhukovskyi vzhukovs@redhat.com