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

Support StepInTarget #90793

Closed
davidanthoff opened this issue Feb 16, 2020 · 18 comments
Closed

Support StepInTarget #90793

davidanthoff opened this issue Feb 16, 2020 · 18 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

@davidanthoff
Copy link
Contributor

This is really a combined question and feature request: my understanding is that currently VS Code doesn't use the StepInTarget stuff?

If so, it would be great if UI for that could be added, it seems super useful :)

@gjsjohnmurray
Copy link
Contributor

@weinand please comment on current status and possible plans

@weinand weinand self-assigned this Feb 17, 2020
@weinand weinand added debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality labels Feb 17, 2020
@weinand weinand added this to the On Deck milestone Feb 17, 2020
@weinand
Copy link
Contributor

weinand commented Feb 17, 2020

Surfacing Debug Adapter Protocol features in the VS Code debugger UI is a continuous goal. But it has lower priority and we do not yet know in which milestone we'll add the feature.

@weinand
Copy link
Contributor

weinand commented Apr 14, 2020

In April we will investigate how we could support this feature in VS Code.

Original feature request
Pull request for adding the feature to DAP.

@weinand weinand modified the milestones: On Deck, April 2020 Apr 14, 2020
@weinand
Copy link
Contributor

weinand commented Apr 27, 2020

After reviewing the original request and the VS UI I suggest the following implementation in VS Code:

The UX is a context menu item "Step Into" with submenus actions for all available targets. The "Step Into" menu item is only available if there is a current stack frame selected in the CALL STACK view (and there is a yellow or green execution highlight in the the editor).
If there are no stepIn targets for the line, "Step Into" is disabled.

How this looks in VS:

de15d060-3964-11e6-8602-db98629c67ab-1

How does the UI interact with the DAP:
Only if a DA has a true supportsStepInTargetsRequest capability, VS Code does the following:

  • in the stopped state VS Code issues the stepInTargets request to find the possible "stepIn targets" for the current frame. The "current frame" is determined by the current stack frame selected in the CALL STACK view.
  • If the stepInTargets request returns one or more targets, the targets show up as sub menu items of the "Step Into" menu. The target's label becomes the menu item's label and the target ID is used as the targetId argument for the stepIn request that is triggered if the menu item is selected. If stepInTargets does not returns any targets the "Step Into" menu should be disabled.

Ideally the stepInTargets request should be executed lazily, so that single stepping through some code does not trigger a stepInTargets request for every step. One approach to achieve this would be by running stepInTargets only if the context menu is about to open or when the "Step Into" menu item is selected and the submenu needs to be populated.

@isidorn does this make sense?

@weinand weinand modified the milestones: April 2020, May 2020 Apr 27, 2020
@isidorn
Copy link
Contributor

isidorn commented Apr 27, 2020

@weinand this overall makes sense. Some comments / feedback:

  • If supportsStepInTargetsRequest is false we show nothing in the context menu
  • Once context menu is contstructed we can not do anything lazily. We also can not prevent the showing of context menu while the stepInTargets are being fetched, so imho there is only one way to make this work:
    • "Step Into Specific" action is always there when adapter supports this, and on click we fetch stepInTargets if there are mulitple targets we show them via quick pick, if only one we just execute it

@davidanthoff
Copy link
Contributor Author

Looks great!

Two thoughts:

While the context menu thing is nice, it also is pretty difficult to access, I think: right click, then you need to move the mouse to the right item in the context menu etc. Could (as an additional option) the button for the step-in option in the debug toolbar be modified to look like a typical Word button:

image

where the right part of the button opens a menu that would show the step-in targets? That could obviously also be added in a later iteration.

The second point is re the distinction of individual step-in targets. Say you have a line of code like x = log(log(x)) + log(x). Yes, that particular example might not make that much sense, but the general idea is how would these different targets be distinguished in the UI? Maybe that is more a question of what the DA return as the label for different step-in targets? Just from a pure UI standpoint, I almost feel the best UI would be if the step-in targets could use some slight rich text stuff, for example that each step-in target would echo back the whole expression, but the function that one would step-into for a given target would be in bold, or some other color, or something like that.

@isidorn
Copy link
Contributor

isidorn commented Apr 27, 2020

@davidanthoff we can also add the command to the quick pick so that users can always trigger it via a keybdining. I do not like the idea of adding it to the debug toolbar since I think it will add unnecessry complexity, however if you feel that is the right way you can always contribute to it from your extensions since the debug toolbar is open to contributions.

@weinand
Copy link
Contributor

weinand commented Apr 27, 2020

@isidorn, @davidanthoff thanks for the feedback.

@isidorn:

  • supportsStepInTargetsRequest: yes, as usual: if capability is false or omitted, we don't support the feature and it does not show up in the UI.
  • "Once context menu is constructed we can not do anything lazily." When/how often is the context menu constructed? If the context menu is constructed at least once after a new "stopped" event, we are able to run the stepInTargets request and construct the corresponding menu from it, right?
  • "QuickPick" proposal: IMO using Quickpick doesn't work because it results in a bad interaction: a user executes "StepInto" and then sometimes nothing happens because a QuickPick shows up in some other location on the screen and waits for his input.

@davidanthoff:

  • yes, I fully agree that having the "Step Into" targets in a drop-down of the "StepIn" button in th debug bar would be great. But since I don't think that our UI widgets already support this, I did not consider this UI for our first implementation of the "Step Into" feature. We will consider this in the next round.
  • "distinction of individual step-in targets if they have the same label": this is a valid point and we have the option to add additional information to the StepInTarget to support a more rich UI. For now a DA could just name labels "1: log", "2: log" etc.

@weinand
Copy link
Contributor

weinand commented Apr 27, 2020

One additional observation: it does not make a lot of sense if the stepInTargets request returns only one target...

@isidorn
Copy link
Contributor

isidorn commented Apr 27, 2020

@weinand context menu is constructed once the user clicks. However we have to register the actions and entries staticily. Since the entries are open ended due to stepInTargets this does not really work. The only way in our UX to show open ended things is quick pick afaik.

@gjsjohnmurray
Copy link
Contributor

Interesting use-case for dynamically computed context menus/submenus. Maybe add your support to my proposal at #9827 (comment)?

@weinand
Copy link
Contributor

weinand commented Apr 27, 2020

The "Refactor" menu seems to do exactly what we need:

2020-04-27_15-50-16 (1)

It is similar to a QuickPick but it is more closely located where it is used.

@isidorn
Copy link
Contributor

isidorn commented Apr 27, 2020

@weinand yes this looks like a good fit, good idea.
@gjsjohnmurray thanks for the pointer, will check it out.

@weinand
Copy link
Contributor

weinand commented May 13, 2020

@isidorn I've added support to Mock Debug for:

  • StepIn and StepOut: StepIn moves execution one character to the right, StepIn to the left
  • StepInTargets: every word in the stopped line is considered one stack frame (as already done for the call stack); StepInTargets returns targets for every character of the stack frame with the given frameId. Running StepIn with a targetId moves execution to the corresponding character position of the frame.

@isidorn
Copy link
Contributor

isidorn commented May 14, 2020

We now support step into targets. Here's how the UX looks
Note that I do not massage the Label which I think is the correct thing to do, just looks weird with Mock debug due to short label. Try it out and let me know what you think

We only show this action in the context menu when the debug supports step into targets capability and only when debug session is stopped.

Screenshot 2020-05-14 at 12 44 55

Screenshot 2020-05-14 at 12 44 59

@isidorn
Copy link
Contributor

isidorn commented May 14, 2020

I plan to write a test plan item so we can test this out.
It would be cool if some debug adapter supported this apart from Mock debug. So @davidanthoff let us know if you start supporting this.

@isidorn isidorn mentioned this issue May 14, 2020
2 tasks
@davidanthoff
Copy link
Contributor Author

Awesome, we'll try to test this out for Julia!

@isidorn
Copy link
Contributor

isidorn commented May 15, 2020

Great, let us know how it goes.

@isidorn isidorn added the on-release-notes Issue/pull request mentioned in release notes label Jun 4, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Jun 28, 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