microsoft / debug-adapter-protocol

Defines a common protocol for debug adapters.
https://microsoft.github.io/debug-adapter-protocol/
Other
1.44k stars 131 forks source link

Add deemphasize as presentationHint of a stack frame #464

Closed adpi2 closed 8 months ago

adpi2 commented 9 months ago

The goal of this PR is to make it possible to deemphasize a single frame in the call stack, instead of the entire source of that frame. This is particularly useful when the smart step skips a single frame in a source file and not the subsequent steps in the same file.

Note that this is already supported by VSCode here and advertised here by @weinand . Also @isidorn suggested here to make it part of the protocol.

gregg-miskelly commented 9 months ago

What is the difference between subtle and deemphasize? If we are going to support both, I think we need a clearer explanation of how they differ.

mfussenegger commented 9 months ago

I think there are also already a few debug adapters using subtle for frames that are getting skipped. So not sure if there's much value in retroactively adding a new hint with that definition.

adpi2 commented 9 months ago

Thanks for your quick comments.

What is the difference between subtle and deemphasize? If we are going to support both, I think we need a clearer explanation of how they differ.

In terms of rendering in VSCode subtle and deemphasize are really different: subtle frames are printed in italics, while deemphasize frames are collapsed.

image

When expanded the deemphasize frames appear grayed out.

image

I personally don't use subtle in the scala-debug-adapter because its semantic meaning is not clear. It appears slightly different but what does that mean? In contrast, collapsing frames clearly shows that some frames are not debuggable and automatically skipped.

If we should keep only one I would suggest to keep deemphasize, but since subtle is already in the spec and used I think we should keep them both.

I think there are also already a few debug adapters using subtle for frames that are getting skipped. So not sure if there's much value in retroactively adding a new hint with that definition.

vscode-js-debug already uses deemphasize on stack frames. It is also already supported by vscode and azure-data-studio.

mfussenegger commented 9 months ago

If we should keep only one I would suggest to keep deemphasize, but since subtle is already in the spec and used I think we should keep them both.

I think there are also already a few debug adapters using subtle for frames that are getting skipped. So not sure if there's much value in retroactively adding a new hint with that definition.

vscode-js-debug already uses deemphasize on stack frames. It is also already supported by vscode and azure-data-studio.

If Microsoft chooses to do off-spec additions that shouldn't count as "keeping". From my perspective as client author what matters is what's in the specification. deemphasize as of yet doesn't exist. The questions here should be:

Based on what's here so far, I'd probably just display it the same way as subtle in the client I maintain.

adpi2 commented 9 months ago

Based on what's here so far, I'd probably just display it the same way as subtle in the client I maintain.

Do you display subtle the same way as deemphasize sources?

It seems that VSCode and other clients do not agree on the meaning of subtle. When it was introduced by @isidorn and @weinand, their initial intention was to display those frames in italics. Later some adapters used subtle to outline skipped frames, for lack of a better alternative, and some clients displayed it the same way as deemphasized sources.

I see 2 options forward:

  1. subtle and deemphasized are the same thing and VSCode should display them collapsed and/or grayed out.
  2. subtle and deemphasized are not the same thing. subte should be in italics, deemphasized should be collapsed and/or grayed out.
mfussenegger commented 9 months ago

Do you display subtle the same way as deemphasized sources?

For nvim-dap:

nvim-dap-ui afaik allows to toggle/expand/collapse frames with the subtle presentation hint.

I think defining things like italic, grayed out or whatever is way too much UI detail for the specification. It should focus on the semantics.

puremourning commented 9 months ago

Vimspector behaviour is essentially same as nvim-dap

adpi2 commented 9 months ago

deemphasize isn't in the specification, so they are displayed like a regular frame.

deemphasize is in the specification of Source. Don't you render them?

gregg-miskelly commented 9 months ago

My two cents:

  1. If you are interested in using deemphasize because italics doesn't convey "less important" to you, I would agree with you and I wish VS Code had choose to grey them out instead. But the spec doesn't mandate italics. If you open a VS Code issue, I would +1 it.
  2. If we want to have a new "even more subtle" presentation hint, I think that is reasonable, but I don't think it should be called deemphasize as I couldn't tell you which one is more subtle between deemphasize and subtle. If the goal of this presentation hint is specifically to support collapsing for clients that support such a thing, perhaps, collapse would be a good name.
adpi2 commented 9 months ago

If you are interested in using deemphasize because italics doesn't convey "less important" to you, I would agree with you and I wish VS Code had choose to grey them out instead. But the spec doesn't mandate italics. If you open a VS Code issue, I would +1 it.

I opened the issue in https://github.com/microsoft/vscode/issues/206801

connor4312 commented 8 months ago

I agree, I believe a change in VS Code is the right thing to do here.

@adpi2 unless you have further thoughts we can close this PR