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 collapsing stack frames #50334

Closed
DanTup opened this issue May 23, 2018 · 20 comments
Closed

Support collapsing stack frames #50334

DanTup opened this issue May 23, 2018 · 20 comments
Assignees
Labels
debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality on-testplan release-notes Release notes issues
Milestone

Comments

@DanTup
Copy link
Contributor

DanTup commented May 23, 2018

Sometimes call stacks in the debugger contain a lot of frames from code that isn't the users (for example SDK code):

#0      Object._noSuchMethod (dart:core-patch/object_patch.dart:42)
#1      Object.noSuchMethod (dart:core-patch/object_patch.dart:46)
#2      HotRunner.validateReloadReport (package:flutter_tools/src/run_hot.dart:407)
#3      HotRunner._reloadSources.<anonymous closure> (package:flutter_tools/src/run_hot.dart:528)
#4      _rootRunUnary (dart:async/zone.dart:1128)
#5      _CustomZone.runUnary (dart:async/zone.dart:1012)
#6      _FutureListener.handleValue (dart:async/future_impl.dart:129)
#7      _Future._propagateToListeners.handleValueCallback (dart:async/future_impl.dart:636)
#8      _Future._propagateToListeners (dart:async/future_impl.dart:665)
#9      _Future._completeWithValue (dart:async/future_impl.dart:478)
#10     Future.wait.<anonymous closure> (dart:async/future.dart:392)
#11     _rootRunUnary (dart:async/zone.dart:1128)
#12     _CustomZone.runUnary (dart:async/zone.dart:1012)
#13     _FutureListener.handleValue (dart:async/future_impl.dart:129)
#14     _Future._propagateToListeners.handleValueCallback (dart:async/future_impl.dart:636)
#15     _Future._propagateToListeners (dart:async/future_impl.dart:665)
#16     _Future._complete (dart:async/future_impl.dart:468)
#17     _SyncCompleter.complete (dart:async/future_impl.dart:51)
#18     _completeOnAsyncReturn (dart:async-patch/async_patch.dart:246)
#19     Isolate.reloadSources (package:flutter_tools/src/vmservice.dart:1047)
#20     _asyncThenWrapperHelper.<anonymous closure> (dart:async-patch/async_patch.dart:30)
#21     _rootRunUnary (dart:async/zone.dart:1128)
#22     _CustomZone.runUnary (dart:async/zone.dart:1012)
#23     _FutureListener.handleValue (dart:async/future_impl.dart:129)
#24     _Future._propagateToListeners.handleValueCallback (dart:async/future_impl.dart:636)
#25     _Future._propagateToListeners (dart:async/future_impl.dart:665)
#26     _Future._complete (dart:async/future_impl.dart:468)
#27     _SyncCompleter.complete (dart:async/future_impl.dart:51)
#28     _completeOnAsyncReturn (dart:async-patch/async_patch.dart:246)

It would be good if we could somehow allow these to be collapsed so the user can focus on their own code, and only expand these if required.

Maybe it could even just be another presentationHint type for "unimportant" or "external" code, and VS Code can collapse consecutive frames with that hint, allowing them to be expanded by clicking?

@weinand weinand added debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality labels May 23, 2018
@weinand weinand self-assigned this May 23, 2018
@microsoft microsoft deleted a comment from vscodebot bot Sep 11, 2018
@weinand
Copy link
Contributor

weinand commented Sep 11, 2018

@isidorn we could collapse subsequent "uninteresting" stackframes into one expandable, right?

@weinand weinand assigned isidorn and unassigned weinand Sep 11, 2018
@weinand weinand added this to the On Deck milestone Sep 11, 2018
@isidorn
Copy link
Contributor

isidorn commented Sep 18, 2018

@weinand yes, this could be easily done since we already use a tree for representing stack frames

@weinand
Copy link
Contributor

weinand commented Sep 18, 2018

Then we should plan to implement this.

@isidorn isidorn modified the milestones: On Deck, October 2018 Oct 3, 2018
@isidorn
Copy link
Contributor

isidorn commented Oct 3, 2018

Currently we have a presentationHint both on the Source and on the StackFrame.
This feature should be based on the presenatationHint on the StackFrame imo. We could use the word deemphasize since we already use that as the presentationHint value for the Source. To be consistent and hopefully not confusing. An alternative is to have the value of something which corresponds to the UI. Or simply presentationHint: collapsed

The questions is how to name the expando element which would expand all subsequent deemphasized sources?
Foreign Stack Frames
Other Stack Frames
Collapsed Stack Frames

Also note that these unimportant stack frames will be indented. Tree does not allow me to do variable indentation based on item type, only on item depth.

fyi @roblourens

@roblourens
Copy link
Member

Do we differentiate between different types of frames that might be collapsed separately? For example, if I have code that is "skipped by skipFiles" and code that is "skipped by smartStep", it might make sense for these to be marked differently so they would be collapsed as separate groups. I'm not sure what else debuggers are using presentationHint for.

I can imagine it making sense to hide "smartStep" skipped frames entirely, since I never care about them, while "skipFiles" frames I may want to inspect but don't want them to be in my face.

Maybe the DA should be able to label the collapsed group with a label that describes what's in it.

And think about the case where the collapsed frames are at the top of the stack. e.g. I break on an exception in a "skipped" file. Chrome devtools will collapse the blackboxed frames at the top of the stack and focus the first non-blackboxed frame, and show the error at that non-blackboxed frame. I'm not sure whether I like that or not, typically in that case I end up expanding the frames and looking at the real line of code that errored. But it's worth thinking about different scenarios.

@DanTup
Copy link
Contributor Author

DanTup commented Oct 8, 2018

I'm not sure what else debuggers are using presentationHint for.

In Dart/Flutter we use a few:

  1. label for async boundary markers
  2. subtle for frames we don't have source for (I'm not totally sure why we'd have this, but it's in the code!)
  3. deemphasize for non-debuggable code (eg. SDK or third party packages; users can toggle these) - this hint is on the source property

Probably we'd want to collapse them all together. Eg. imagine the stack looks like:

  • User code
  • Framework Code
  • Async boundary
  • Framework Code
  • Async boundary
  • Framework Code
  • Unknown Source
  • User Code

Having them collapse individually wouldn't make sense here (so doing it automatically by presentationHint would probably suck); though allowing the DA to assign a named group so they can control it might make sense.

e.g. I break on an exception in a "skipped" file. Chrome devtools will collapse the blackboxed frames at the top of the stack and focus the first non-blackboxed frame

I would love something like this. One of my gripes with the Dart debugger right now is that it breaks on exceptions inside code I've marked non-debuggable and I couldn't see an obvious way to jump back to the first debuggable source frame.

@isidorn
Copy link
Contributor

isidorn commented Oct 25, 2018

I have pushed an initial version of this. Please try it out in tomorrow's insiders

In short it will collapse all subsequent stack frames with source that has presentation hint demphasized. The parent element will have the title Show More Stack Frame + source.origin if they all share the origin.
In the future we can do a more sophisticated grouping based on origin.

Here's an example how it looks

showmore

@roblourens
Copy link
Member

Awesome. Would be great to show the number of hidden frames (Show N more frames)

@isidorn
Copy link
Contributor

isidorn commented Oct 25, 2018

Suggestion accepted. Great idea

@DanTup
Copy link
Contributor Author

DanTup commented Oct 31, 2018

@isidorn This currently shows undefined in the message for my existing code (before I've changed anything); I don't think that's expected:

screen shot 2018-10-31 at 1 40 18 pm

Edit: Looks like this comes from origin which I don't set because it's marked optional. I guess it just needs a check before displaying.

@DanTup
Copy link
Contributor Author

DanTup commented Oct 31, 2018

Also - it feels slightly strange to re-collapse as you're clicking on them. If you do want to navigate up the stack while debugging. I don't have a suggestion to fix that though; it'd need some other way to re-collapse 🤔

@DanTup
Copy link
Contributor Author

DanTup commented Oct 31, 2018

Also, is there a better description for what we can/should put in origin? The comment says:

/** The (optional) origin of this source: possible values 'internal module', 'inlined content from source map', etc. */

But the etc. isn't very helpful. The comment here suggests that it may have specific meanings (beyond just a string), though it seems like it might really just be a text label?

The values I'd like to put in it are for SDK sources and for third party packages.

@weinand
Copy link
Contributor

weinand commented Oct 31, 2018

origin is just a string shown in the UI but not interpreted by VS Code in any way.

@DanTup
Copy link
Contributor Author

DanTup commented Oct 31, 2018

@weinand Great, thanks - I'll add suitable labels then :-)

(though the undefined label should still be fixed, I guess a lot of code might not provide it).

@weinand
Copy link
Contributor

weinand commented Oct 31, 2018

@isidorn please fix the "undefined" label for October.

@DanTup
Copy link
Contributor Author

DanTup commented Oct 31, 2018

Also - it feels slightly strange to re-collapse as you're clicking on them

Seems like this behaviour is specific to when a frame has no source. If the frame has source, it navigates there and leaves the section expanded.

@isidorn
Copy link
Contributor

isidorn commented Oct 31, 2018

I have fixed the undefined label via 9f4969b

isidorn added a commit that referenced this issue Oct 31, 2018
@roblourens
Copy link
Member

Also - it feels slightly strange to re-collapse as you're clicking on them

Yeah I thought I couldn't recollapse them, is this a bug or how do I do that?

@isidorn
Copy link
Contributor

isidorn commented Oct 31, 2018

You can not recolapse. This is a design decision made by @weinand and me since they collapse again after you step.
Let's go with this design for now, and if you hate it after trying it out a bit more we can reconsider this.
Thanks

@DanTup
Copy link
Contributor Author

DanTup commented Oct 31, 2018

Yeah I thought I couldn't recollapse them, is this a bug or how do I do that?

Based on the above, it's not supposed to collapse. However it seems to be a bug that if you don't have source code for a frame (you return an error from sourceRequest) then it does collapse immediately when you click on that frame.

@isidorn isidorn added the release-notes Release notes issues label Nov 2, 2018
@vscodebot vscodebot bot locked and limited conversation to collaborators Dec 10, 2018
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-testplan release-notes Release notes issues
Projects
None yet
Development

No branches or pull requests

4 participants