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

[debug] Add "debug" progress indicator #6009

Merged
merged 1 commit into from
Aug 26, 2019
Merged

Conversation

AlexTugarev
Copy link
Contributor

@AlexTugarev AlexTugarev commented Aug 21, 2019

What it does

Adds a progress bar to the debug view.

2019-08-23 10 34 47

Fixes #5988

How to test

  • start a debugging session and see a progress indicator

Review checklist

Reminder for reviewers

Fixes #5988

Signed-off-by: Alex Tugarev <alex.tugarev@typefox.io>
@akosyakov akosyakov added debug issues that related to debug functionality notifications issues related to notifications labels Aug 23, 2019
@akosyakov
Copy link
Member

@AlexTugarev is it ready for a review?

@AlexTugarev AlexTugarev marked this pull request as ready for review August 23, 2019 08:35
@westbury
Copy link
Contributor

Can we make this progress support progress percentage? We have debug adaptors that report the percent in the OutputEvent:

new OutputEvent('progress', 'telemetry', {
            percent,
            message
        }

We know the progress percent because we get this from the process when downloading the image.

Currently this ends up at logOutput in DebugConsoleSession. It would be nice if this percent could be shown in this progress monitor.

@akosyakov
Copy link
Member

akosyakov commented Aug 23, 2019

@westbury Could you please accept an invitation to the organization? That i can assign you for a review, also to core team since you are the committer now.

Copy link
Contributor

@tolusha tolusha left a comment

Choose a reason for hiding this comment

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

I built and run the project.
Everything were working as expected.

@AlexTugarev
Copy link
Contributor Author

@westbury can you elaborate on the intent of this information? it looks like you're talking about telemetry data. well, that would not be the idea of this PR to bring progress indication as it's done in vscode; shame on me I didn't mentioned that before!

@AlexTugarev AlexTugarev added the vscode issues related to VSCode compatibility label Aug 23, 2019
@thegecko
Copy link
Member

thegecko commented Aug 23, 2019

@AlexTugarev

that would not be the idea of this PR to bring progress indication

Perhaps the PR should be rename activity indication 😄
I think the point here is that it's possible to have sources of richer progress data and this PR doesn't currently allow exposing this richer data in the UI.

It's just a suggestion, the addition as it stands is an improvement.

@AlexTugarev
Copy link
Contributor Author

AlexTugarev commented Aug 23, 2019

@thegecko, the term "progress indicator" is used for indeterminate indicators e.g. in vscode and material design as well. I'd like to stick with that.

I think the point here is that it's possible to have sources of richer progress data and this PR doesn't currently allow exposing this richer data in the UI.

The progress bars integrated into scm/git, search, and debug as proposed in this PR, are linked to a "progress location" with the id debug which allows different tasks to indicate progress, cf. currently the single call site https://github.com/theia-ide/theia/blob/bb37bd1c15ffd5165ceaafa6937c38e499d1853c/packages/debug/src/browser/debug-session-manager.ts#L146
Other components may report progress to this location as well. I think it fits quite well to render an indeterminate indicator, especially when a user might not understand where the length of a process comes from.

Generally the ProgressService does support reporting of progress updates as it is done already for the window (aka status bar) location, cf. https://github.com/theia-ide/theia/blob/master/packages/core/src/browser/progress-status-bar-item.ts#L58. So, it depends on what you would like to render in the first place. The progress bar can aggregate multiple tasks to show an indeterminate progress indicator. I'm fully open to the idea of falling back to the current behavior, but showing a determinate progress there if there is only one progress source and it's reporting updates.

@akosyakov
Copy link
Member

@AlexTugarev Are you going to merge it?

@AlexTugarev
Copy link
Contributor Author

@westbury and @thegecko I like the idea to enhance it as suggested and created #6043 as a followup.

@AlexTugarev AlexTugarev merged commit 6c4fa9c into master Aug 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
debug issues that related to debug functionality notifications issues related to notifications vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show progress in Debug view
5 participants