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

Proposal: support autoExpand Scope field #116

Closed
isidorn opened this issue May 19, 2020 · 10 comments
Closed

Proposal: support autoExpand Scope field #116

isidorn opened this issue May 19, 2020 · 10 comments
Assignees
Labels
feature-request Request for new features or functionality

Comments

@isidorn
Copy link

isidorn commented May 19, 2020

Currently VS Code has a weird heuristic to auto expand the first scope if all the scopes are expanded. This does not work well in all cases as seen in this issue microsoft/vscode#93230

I propose that we introduce an autoExpand field in the Scope object such that the debug adapter can control which scopes should be autoExpanded by the UI and which not.

@puremourning
Copy link
Contributor

isn't that what expensive is for ?

@isidorn
Copy link
Author

isidorn commented May 19, 2020

Not really, but good point they are related.
expensive just means that the scope is large and that in no way should it be auto expanded.
However currently we do not have an opossite to say it has to be auto expanded.
Just auto expanding all non expensive scopes is not a good experience.

@puremourning
Copy link
Contributor

just auto expanding all non expensive scopes is not a good experience.

Well that's what vimspector does and it's a pretty good experience in my experience. But i don't disagree that having an explicit option is fine, it just means what's the point of expensive then ? What would a client actually do with expensive ?

@isidorn
Copy link
Author

isidorn commented May 19, 2020

@puremourning for example VS code uses the expensive flag when evalaution hover is not enabled to search for variables. Thus it does not search in expensive scopes.
Code pointer https://github.com/microsoft/vscode/blob/856dea7e718a54e6799c7cac96dbd80acd3b5889/src/vs/workbench/contrib/debug/browser/debugHover.ts#L60

Also when searching for mostSpecificScopes for the inline value decorations
Code pointer https://github.com/microsoft/vscode/blob/856dea7e718a54e6799c7cac96dbd80acd3b5889/src/vs/workbench/contrib/debug/common/debugModel.ts#L328

Also when to autoExpand, but that we will change with the new flag.

@weinand weinand added the feature-request Request for new features or functionality label May 19, 2020
@puremourning
Copy link
Contributor

OK thanks.

what should be the behaviour of:

auto expand: true,
expensive: true

?

@isidorn
Copy link
Author

isidorn commented May 19, 2020

Up to the client to decide.
I personally would auto expand it in VS Code. But would not search in it for mostSpecificScopes and debugHover variables.

@puremourning
Copy link
Contributor

"up to the client to decide" is an invitation for server vendors to base their implementation on a specific client. this is a problem in general (LSP is a disaster ni this regard), so ideally we'd include guidance (at least) in the DAP.

But like i said, i think this is a reasonable change to the protocol with a little sentence to explain the difference between the 2.

@connor4312
Copy link
Member

To avoid the ambiguity we could deprecate expensive and instead have a hint like autoExpand: 'always' | 'never' | 'auto', the latter leaving it up to the client to decide as it does today.

@weinand
Copy link
Contributor

weinand commented Nov 16, 2022

@connor4312 I like your proposal of an autoExpand hint.
We can deprecate (but never remove) the expensive property. In its description we could say: "if autoExpand is used, clients should ignore the expensive property".

@weinand weinand assigned connor4312 and unassigned weinand Nov 16, 2022
@connor4312
Copy link
Member

connor4312 commented Nov 16, 2022

Two years later, I don't think I'm a big fan of this proposal any more. We fixed the initial issue in vscode: turned out to just be some buggy logic that didn't keep expansion states very well. I think the choice of whether to expand non-expensive fields is a UI decision best left to (possibly configurable logic in) clients.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request Request for new features or functionality
Projects
None yet
Development

No branches or pull requests

5 participants