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

Improve discoverability of 'Restart Frame' #84045

Closed
weinand opened this issue Nov 6, 2019 · 14 comments
Closed

Improve discoverability of 'Restart Frame' #84045

weinand opened this issue Nov 6, 2019 · 14 comments
Assignees
Labels
debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality release-notes Release notes issues verification-needed Verification of issue is requested verified Verification succeeded
Milestone

Comments

@weinand
Copy link
Contributor

weinand commented Nov 6, 2019

No description provided.

@weinand weinand added debug Debug viewlet, configurations, breakpoints, adapter issues debt Code quality issues labels Nov 6, 2019
@weinand weinand added this to the November 2019 milestone Nov 6, 2019
@isidorn isidorn added feature-request Request for new features or functionality and removed debt Code quality issues labels Nov 20, 2019
@isidorn
Copy link
Contributor

isidorn commented Nov 20, 2019

Currently we show the restart frame in the context menu when the debug adapter supports the restart frame request.
However we show this on every stack frame in the call stack. For node I get an error whenever I try to execute this command on any stack frame that is not top level.

  1. Should we show this action only on the top stack frame? Since it does not make a lot of sense to me that a user can restart any other stack frame than the top

How to improve its discoverability? My idea is the following:

  • Similar to the inline thread and session actions in the call stack view, we would on hover and focus render a restart icon. It would be rendered instead of line and column infomation. In case we go down this approach we would could use our old restart icon, or a new one to distuingish that this does not actually restart the whole program. So the icon should have a meaning of "mini restart" @misolori

Let me know what you think @weinand @roblourens @connor4312

Pic for reference of inline session actions. However for stack frame we would just have one for now
Screenshot 2019-11-20 at 16 52 00

@connor4312
Copy link
Member

connor4312 commented Nov 20, 2019

Showing it on hover makes sense to me. My once concern would be accidentally clicking it since it's a new clickable area that shows up only on hover. Maybe having the icon to the left of the location would be less likely to have misclicks (i.e. left of "_onPaused")
image

Being in or beside a stack frame the standard restart icon conveys the message to me. Or alternately something like this would also work:

image

@isidorn
Copy link
Contributor

isidorn commented Nov 20, 2019

Fancy design skill and a good idea.
Let's see what others have to say. New icon, or old icon + hover text explanation

Having the action left of the tree item is not really aligned with what we do in other parts of our UX (open editors being the only exception). And I am not worried about people accidently clicking on it, since that area is not really clickable so far (why would a user click there)

@miguelsolorio
Copy link
Contributor

This is merely for illustration purposes, but something like this?

image

@isidorn
Copy link
Contributor

isidorn commented Nov 21, 2019

@misolori that is pretty awesome

isidorn added a commit that referenced this issue Nov 21, 2019
@isidorn
Copy link
Contributor

isidorn commented Nov 21, 2019

I have pushed a change to show the restart frame icon on hover and on row focus. When the adapter does not support restartFrame action everything behaves as before. So on hover no change

Some follow up work:

  1. @misolori once we have a new icon can you just update the codicon used here
  2. @weinand @roblourens @connor4312 can we decide if we need this action for not top stack frames. In theory this makes sense, however I am not sure if debuggers can support this in practice. Currently node2 throws an error when trigged by the user. It could instead restart the top frame instead or we show the action as disabled (would need to be contributed by the adapter)
  3. @misolori once I focus on the action, it jumps up a bit. And stays vertically not centered. I believe this is nothing special for this icon, but a general codicon action bar issue. Can you look into this ( I can also create a new issue)

Try it out and let me know how you feel about it. Thanks!
I had an alternative solution where I would just hide the line numbers (and still show the filename) for the action but after discussion with @weinand we decided on the curren solution

restartFrame

@connor4312
Copy link
Member

connor4312 commented Nov 21, 2019

It looks like the chrome devtools can, so in js-debug/PWA we should be able to restart frames anywhere in the current stack. But with async stack traces we won't be able to restart frames that happened on a different stack that the current one (even though it's displayed as the 'same' call stack in the UI).

I think it would make sense for a 'restartable' flag to to be annotated on stack frames sent back down the DAP. And other languages might only be able to restart the top stack frame / only be able to restart under certain conditions.

@miguelsolorio
Copy link
Contributor

Icon is now in via b24775a:

image

@isidorn
Copy link
Contributor

isidorn commented Nov 22, 2019

Thanks @misolori I also checked and the icon looks nice when selected and it is white.
Let's close this one for now. And we can create follow up items.
@weinand would you like me to create a new issue for the "restartable" discussion?

@isidorn isidorn closed this as completed Nov 22, 2019
@isidorn isidorn added the verification-needed Verification of issue is requested label Dec 2, 2019
@isidorn
Copy link
Contributor

isidorn commented Dec 2, 2019

Verifier: start debugging and verify that when you hover over a call stack item you see the restart frame icon. Clicking on it restarts the frame (note that Node only supports restartgin the top frame currently)

@jrieken jrieken added the verified Verification succeeded label Dec 3, 2019
@jrieken
Copy link
Member

jrieken commented Dec 3, 2019

Verified tho I have noticed that this makes the call stack view quite nervous - I believe that's because the hide the filename and line/column information on hover

@isidorn
Copy link
Contributor

isidorn commented Dec 3, 2019

Yes, the view is now more nervous. I could show this with a timeout. However we do not do this in other views. Open to suggestions.

@jrieken
Copy link
Member

jrieken commented Dec 4, 2019

Would it be possible to not have either or but to have the restart button push the details to the left?

@isidorn
Copy link
Contributor

isidorn commented Dec 4, 2019

Makes sense. Created a follow up item for this #86248

@isidorn isidorn added the release-notes Release notes issues label Dec 6, 2019
@vscodebot vscodebot bot locked and limited conversation to collaborators Jan 6, 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 release-notes Release notes issues verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

5 participants