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

Render progress for DAP progress events #92253

Closed
isidorn opened this issue Mar 9, 2020 · 20 comments
Closed

Render progress for DAP progress events #92253

isidorn opened this issue Mar 9, 2020 · 20 comments
Assignees
Labels
debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality on-release-notes Issue/pull request mentioned in release notes on-testplan
Milestone

Comments

@isidorn
Copy link
Contributor

isidorn commented Mar 9, 2020

Related debug adapter protocol issue microsoft/debug-adapter-protocol#92

fyi @weinand

@isidorn isidorn added feature-request Request for new features or functionality debug Debug viewlet, configurations, breakpoints, adapter issues labels Mar 9, 2020
@isidorn isidorn added this to the March 2020 milestone Mar 9, 2020
@isidorn isidorn self-assigned this Mar 9, 2020
@weinand
Copy link
Contributor

weinand commented Mar 12, 2020

@isidorn I have added experimental DAP progress support to debugProtocol.d.ts.

I suggest that we start prototyping the debug progress UI for the use case "progress independent from a specific request". This is the important use case that @chuckries and @DanTup are looking for.

I will start this effort by adding this use case to Mock Debug.

@isidorn
Copy link
Contributor Author

isidorn commented Mar 13, 2020

@weinand sounds good. Let me know when Mock Debug has adopted this.
Progress independent from a request: I suggest to render in as the spinning bar at the top of the Debug Viewlet.
Alternatives are:

  • status bar entry (personally dislike this)
  • progress badge in debug activity bar
  • global workbench progress (too global imho)

@weinand
Copy link
Contributor

weinand commented Mar 13, 2020

@isidorn Yes, let's start by showing "unspecific progress" at the top of the Debug Viewlet.
If we will start showing the percentage we could show it in the status bar.

A shortcoming of the "top of the Debug Viewlet" location is that the user won't be able to cancel it, right?

@isidorn
Copy link
Contributor Author

isidorn commented Mar 13, 2020

@weinand while we render progress we could also render a X or some other cnacel action in the debug viewlet title area. Pressing that would send a cancel. Check out search for long running search operations does somethign similar.

@weinand
Copy link
Contributor

weinand commented Mar 13, 2020

@isidorn I've added progress support to Mock Debug via this commit.
Running "progress" in the REPL starts a sequence of 100 progress events over a period of 50 seconds (but only if the client frontend sends a supportsProgressReporting capability).
The progress is cancellable.

@weinand
Copy link
Contributor

weinand commented Mar 16, 2020

@isidorn some random notes:

  • yes, debug should have a similar cancelling UI as search (Mock Debug should have everything in place to support cancel)
  • Mock debug provides the (mandatory) "title" attribute and the optional "message" attribute. We'll have to decide how they are displayed.
  • overlapping progress: you can produce concurrent progress sequences with Mock debug by running multiple "progress" commands in the REPL within 50 seconds. I see two strategies for how to deal with concurrent progress sequences:
    • we could stack them and show only the top (newest) one (cleanest)
    • we could maintain just one progress and every new progress overwrites the current (kludge)
  • next stage of the prototype: what to do with request-specific-progress. For now I suggest that we treat it identical as non-request-specific progress.

@isidorn
Copy link
Contributor Author

isidorn commented Mar 16, 2020

@weinand thanks for adding the progess support for Mock debug. I plan to jump on this today.

  1. Cool, will add a cancel title action
  2. Maybe we show a hidden notification for every progress, and once the user clicks on the notification center he can se the title and the message
  3. Idealy we would show progress as long as there is any progress event. Canceling cancels the last one
  4. Yeah for now we will just render in the debug viewlet. Once there are specific use cases we can think more

@isidorn
Copy link
Contributor Author

isidorn commented Mar 18, 2020

I have pushed the inital support for this.
When the progress is reported we show the progress bar in the debug viewlet and the progress can be canceled. Note the following:

  • We use a "stop" icon which I think works for the ocassion, attached is an icon that search uses, but I do not think it would be necessery to "scope the icon to debug", since it is already in the viewlet fyi @misolori maybe instead of the X in the circle we should use the do not park sign that search uses
  • When the user presses the Cancel we cancel the last progress that arrived
  • We show progress as long as any progress is left
  • If a session goes down we stop all the progress associated with that session
  • Tooltip of the Cancel action shows the title of the progress

What is not covered by this work and we can tackle as seperate issues once clients adopt this:

  • We ignore the ProgressUpdateEvent since VS Code progress UI does not support reporting progress status
  • We do not show the message of the progress (could be done as a hidden progress notification)
  • We only show progress in the debug viewlet, we do not show any scoped progress

Note that I had to do a slight change in mock debug such that it no longer waits after a progress has been canceled microsoft/vscode-mock-debug@29ec169
For reference here's the Cancel progress icon search is using
Screenshot 2020-03-18 at 10 29 26

Try it out and let me know what you think.

progress

@isidorn
Copy link
Contributor Author

isidorn commented Mar 18, 2020

Based on the great feedback from @bpasero we have decided to show progress via progress notifications.
Since this is similar to the save participant. On top of that show progress bar in debug viewlet (but not the cancel button).
Good part of this approach is that notifications can show the message and have multiple progress managment.
Once we have view cancelalble progress in workbench think of adopting.
Progress notifications will show the progress in status bar even when they are hidden.

@isidorn
Copy link
Contributor Author

isidorn commented Mar 18, 2020

We have done the changes and now the progress cancelation is much easier to manage by users imho.

There is still some work in progress:

Try it out and let me know what you think.

progress

isidorn added a commit that referenced this issue Mar 18, 2020
isidorn added a commit that referenced this issue Mar 19, 2020
@isidorn
Copy link
Contributor Author

isidorn commented Mar 19, 2020

@weinand I have pushed a commit such that we now also respect the progressUpdate events.

@gjsjohnmurray
Copy link
Contributor

Nice! How about also setting options.source to something, so the message gets a [source] prefix that lets the user know where the message is coming from?

@bpasero
Copy link
Member

bpasero commented Mar 19, 2020

That should be the extension I guess that sets the progress, I like that 👍

@weinand
Copy link
Contributor

weinand commented Mar 19, 2020

@gjsjohnmurray excellent suggestion.

@isidorn Since the DAP does not know anything about extensions, we cannot put something in the spec that says that the extension's name has to go into the progress' "title" attribute.

I suggest that we let VS Code add the extension name by using a format string like "${debugExtensionName}: ${progressTitle}"

@isidorn
Copy link
Contributor Author

isidorn commented Mar 19, 2020

@weinand @gjsjohnmurray makes sense, good sugggestion.
Though I just used the ${debugExtensionName} since the progressTitle is already set on the message so to avoid duplication.

isidorn added a commit that referenced this issue Mar 19, 2020
@weinand
Copy link
Contributor

weinand commented Mar 19, 2020

The "source" in the notification works nicely:

2020-03-19_17-54-29

Since we should not use too much space in the status bar, we could show the "source" when hovering over the status item?
2020-03-19_17-57-10

@isidorn
Copy link
Contributor Author

isidorn commented Mar 19, 2020

I already talked to @bpasero about this and I think we should not show the message in the status bar. Only the title. This way it is way too wide.
On hover we should show the message or the source.

@gjsjohnmurray
Copy link
Contributor

On hover we should show the message or the source.

Or perhaps {message} [{source}]

For example progress: 53 [Mock Debug]

@weinand
Copy link
Contributor

weinand commented Mar 19, 2020

In the status item we could start with the progress "title", and if there is a progress "message", we would overwrite the title with the message (so we only show one thing).
On hover we could show the "title" and the "source" as @gjsjohnmurray suggested.

@bpasero
Copy link
Member

bpasero commented Mar 19, 2020

The status bar is capable of showing title and message at the same time as far as I can tell. It would simply show it as title: message and I think it is good to show both and not just the message because there is a chance that it becomes harder for the user to understand what is going on.

@isidorn isidorn added the on-release-notes Issue/pull request mentioned in release notes label Apr 3, 2020
@github-actions github-actions bot locked and limited conversation to collaborators May 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality on-release-notes Issue/pull request mentioned in release notes on-testplan
Projects
None yet
Development

No branches or pull requests

4 participants