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

Ensure meaningful information is displayed via progress reporter #3082

Merged
merged 1 commit into from
Apr 25, 2023

Conversation

jdneo
Copy link
Collaborator

@jdneo jdneo commented Apr 21, 2023

In the progress report terminal, append detailed messages only when they are available. This is to avoid seeing tasks like:

xxx Task: status [0/0]

Signed-off-by: Sheng Chen <sheche@microsoft.com>
@jdneo jdneo added the bug label Apr 21, 2023
Copy link
Member

@rgrunber rgrunber left a comment

Choose a reason for hiding this comment

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

I don't think I came across instances where the report status was empty, but agree that this would probably fix the problem if it did occur.

taskMsg += `: ${report.status}`;
}
if (report.totalWork && report.workDone >= 0) {
taskMsg += ` [${report.workDone}/${report.totalWork}]`;
Copy link
Member

Choose a reason for hiding this comment

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

Are we ok with messages like this : Refreshing workspace 0% [0/1000] ?

servertasks-at-0

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

0 means the job is just started, so I think it should be fine.

@jdneo
Copy link
Collaborator Author

jdneo commented Apr 25, 2023

I don't think I came across instances where the report status was empty, but agree that this would probably fix the problem if it did occur.

If the progress report is provided by the progress monitor, then in most case it should be fine. The problem may occur when the related APIs are called manually, for example: https://github.com/eclipse/eclipse.jdt.ls/blob/06716e21d336f8acbb5c99d29e19f252cfffe83a/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/framework/protobuf/ProtobufSupport.java#L160-L163

The above case does not have problems, but if some of the fields are missing, then the display content at the client side looks wired. I didn't find any guidance about how the APIs should be called at the server side, I think the client side should do some check to ensure the display content looks in a good way.

@rgrunber rgrunber merged commit 2df5085 into redhat-developer:master Apr 25, 2023
2 checks passed
@jdneo jdneo deleted the cs/reporter branch April 26, 2023 01:35
@rgrunber rgrunber added this to the End April 2023 milestone Apr 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants