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

Allow a stack frame with no source without saying "Unknown source" #20677

Closed
roblourens opened this issue Feb 15, 2017 · 20 comments
Closed

Allow a stack frame with no source without saying "Unknown source" #20677

roblourens opened this issue Feb 15, 2017 · 20 comments
Assignees
Milestone

Comments

@roblourens
Copy link
Member

From #5552, looking at using stack frames with no source as a label for groups of async frames.

Currently when there's no source, they say "Unknown source" on the right. I think that either that label shouldn't be shown, or I should be able to mark a stack frame to have it not shown.

Also these frames are automatically 'deemphasized'. I'm not sure whether I would want that style. It needs to be obvious that these frames are async, and the point of deemphasize is to make a frame easy to ignore. I think that decision should be left up to the debug adapter.

@isidorn
Copy link
Contributor

isidorn commented Feb 21, 2017

I agree that more of this should be moved to the debug adatper.
@roblourens is this something that is needed for feb, or should we tackle it in march?

@roblourens roblourens added this to the March 2017 milestone Feb 22, 2017
@roblourens
Copy link
Member Author

I won't do anything with it in Feb

@roblourens
Copy link
Member Author

Instead of changing what happens when there's a frame with no source, I think it would be better to introduce a 'presentationHint' like 'label', which @weinand proposed in the other issue. It does make sense to show "Unknown source" for a normal frame with no source - that might be an error. A label frame is a specific thing that will work differently than other frames.

@isidorn
Copy link
Contributor

isidorn commented Mar 10, 2017

I agree that we should introduce a new presentationHint.
Assigning this to @roblourens to add that presentationHint once that is supported please assign this issue back to me and I will pick it up on the vscode side.

Suggestion on how such a source should be shown are welcome here and in what way should it be specially treated.

fyi @weinand

@isidorn isidorn assigned roblourens and unassigned isidorn Mar 10, 2017
@roblourens
Copy link
Member Author

You're assigning to me to first have the debug adapter return the new presentationHint? Sure, I can do that today.

@weinand
Copy link
Contributor

weinand commented Mar 10, 2017

I'll add a 'frame' presentationHint to the DAP.

@isidorn
Copy link
Contributor

isidorn commented Mar 10, 2017

@roblourens so if the stackFrame returns no source I will behave the same as before.
When an adapter returns a source with presentationHint 'frame' in what special way would you like it to be styled? Or just give me an easy way to repro this and I can make it look good

@weinand
Copy link
Contributor

weinand commented Mar 10, 2017

I don't think that we should use the presentationHint on Source for this. It is more appropriate to have something like this on StackFrame. I'm investigating...

@isidorn
Copy link
Contributor

isidorn commented Mar 10, 2017

@weinand from my implementation point of view - I always expect a Source to be non null, so even if you introduce the presentaion hint on the stackFrame I will create a dummy source object with that presentation hint

@weinand
Copy link
Contributor

weinand commented Mar 10, 2017

@isidorn sure, you can create a dummy Source object as an implementation detail. But from the protocol perspective it sounds wrong to have the backend create dummy Source objects just to be able to specify the 'frame' presentationHint.

@roblourens
Copy link
Member Author

roblourens commented Mar 10, 2017

I agree, I forgot that the presentationHint isn't actually on the frame.

@roblourens roblourens assigned weinand and isidorn and unassigned roblourens Mar 11, 2017
@weinand
Copy link
Contributor

weinand commented Mar 14, 2017

I've created this "protocol addition" request: microsoft/vscode-debugadapter-node#106

@isidorn
Copy link
Contributor

isidorn commented Mar 14, 2017

Great.
@roblourens let me know once node2 supports this so I can pick it up on my side

@weinand
Copy link
Contributor

weinand commented Mar 14, 2017

@isidorn I've added an optional presentationHint attribute to type type StackFrame.
The only supported value for now is label.
It is available in the latest.

@weinand weinand removed their assignment Mar 14, 2017
@isidorn
Copy link
Contributor

isidorn commented Mar 14, 2017

This is how we style stack frames which have a presentationHint = label
Note that I have hacked what the backend to always return presentationHint = 'label' in order to get his picture

screen shot 2017-03-14 at 17 52 35

@roblourens
Copy link
Member Author

This is beautiful!
chrome-debug:

image

node unfortunately brings in some baggage, but this is still awesome:

image

Next up... collapse deemphasized frames? :)

@weinand
Copy link
Contributor

weinand commented Mar 14, 2017

@roblourens you could use a heuristic in the DA to hide those "well known" frames (independent from skipFiles)?
I don't think that anybody wants to ever debug the 5 deemphasized frames from above.

@roblourens
Copy link
Member Author

roblourens commented Mar 14, 2017

That's not a bad idea. I could show a frame that says '5 collapsed frames' in the deemphasized style (or even using 'label'). And there could be a context menu item to toggle it.

It even might not be a bad thing to do for all skipFiles, especially when using async/await, because it generates a lot of garbage frames in user scripts. Collapse them by default, toggling works on the whole callstack and persists while stepping.

Cool how much we can do with minimal added vscode UI.

@weinand
Copy link
Contributor

weinand commented Mar 14, 2017

Yes, we should consider to have a generic "collapse/expand skipped frames" feature.
I was suggestion the "heuristic" only as a temporary measure (which does not require anything on the UI side).

@isidorn
Copy link
Contributor

isidorn commented Mar 15, 2017

@roblourens great that you were able to connect all the dots! Maybe you could post a short gif showing of this niceness in the ux channel once you have it polished.

@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 18, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants