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

dap: add 'clipboard' support, and truncate a long value #2513

Merged
merged 8 commits into from
Jun 4, 2021

Conversation

hyangah
Copy link
Contributor

@hyangah hyangah commented May 26, 2021

  • add 'clipboard' capability
  • apply a larger string limit for 'hover' and 'clipboard' context
  • truncate the string representation of a compound type (or pointer of a compound type) variable if the length of the value is larger than 256. But when a variable is returned as a result of Evaluate Request for hover or copy value feature (clipboard or variables context), do not truncate the value.

Updates golang/vscode-go#1318
Fixes golang/vscode-go#1435
Updates golang/vscode-go/issues/126

- add 'clipboard' capability
- apply a larger string limit for 'hover' and 'clipboard' context
- truncate the string representation of compound (or pointer of compound)
type variable
@hyangah
Copy link
Contributor Author

hyangah commented May 26, 2021

Arbitrarily chosen 1K, Still 1K is too large for my taste.

Before:
Screen Shot 2021-05-26 at 2 58 52 PM

After:
Screen Shot 2021-05-26 at 2 59 42 PM

Still it's possible that we'd end up sending a large JSON message for variables response if the requested variable/scope contains many long variables. This PR doesn't mean to prevent that.

service/dap/server.go Outdated Show resolved Hide resolved
@derekparker
Copy link
Member

Arbitrarily chosen 1K, Still 1K is too large for my taste.

Would you like time to live with it and possibly change this before it gets submitted?

@suzmue
Copy link
Contributor

suzmue commented May 28, 2021

Arbitrarily chosen 1K, Still 1K is too large for my taste.

I agree I think that these strings become unnecessarily long.

When I was testing paging, this was the cause of some things being so slow that it was unusable for investigating the values.

I would say drop it to ~100 (or even less) especially if it has children to expand to get more information.

- use flag bits instead two bools for convertVariableWithOpts options.
- lower the defaultMaxValueLen from 1k to 256.
@hyangah
Copy link
Contributor Author

hyangah commented May 28, 2021

Lowered the max limit to 256. Tried 128 and it broke some tests and some didn't look nice with 128 (e.g. slice of pointers to a compound type, ...) due to all the type info and len/cap info before the actual data. :-(
I think a better solution is to truncate more intelligently, but I don't want to go too deep into it now.

@hyangah
Copy link
Contributor Author

hyangah commented May 29, 2021

@polinasok @suzmue, is this 256 limit ok? Or should we look into further to do better than SinglelineString and avoid truncating informational messages (e.g. failed load, or partial load...)?

I am not sure what you mean by "truncating information messages".

I do agree that 1K is too long and something shorter is sufficient as long as it can be expanded further. 256 seems ok.

I took a quick look at Python. They truncated my map at about 300+ characters, but it appeared to be based on the number of elements, not characters, and the output was {a, b, c, ...}, where entries were removed intelligently. But in any case, our number is in the similar ballpark.

Copy link
Member

@aarzilli aarzilli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, needs decision over the length constant.

@suzmue
Copy link
Contributor

suzmue commented Jun 2, 2021

LGTM

Copy link
Member

@derekparker derekparker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I'll wait to merge until the length is sorted out and we get an ack from @polinasok.

@derekparker derekparker added this to the v1.6.2 milestone Jun 2, 2021
Copy link
Collaborator

@polinasok polinasok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you remind me please what actions in the vscode UI generate evaluate requests with "hover" and "clipboard" context. I know that right click + "Copy to Value" is "variabels" and using Debug Console is "repl".

service/dap/server.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@polinasok polinasok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR description needs updating.

service/dap/server.go Show resolved Hide resolved
@suzmue
Copy link
Contributor

suzmue commented Jun 3, 2021

When the server declares the capability supportsClipboardContext, Copy Value from both the variable pane and WATCH use the clipboard context.

@hyangah
Copy link
Contributor Author

hyangah commented Jun 4, 2021

@polinasok It looks like you accidentally edited my comment instead of responding to it.

I am not sure what you mean by "truncating information messages".

  • information other than the variable's value such as len/cap of slice/array, or type information or address expression
  • information that indicates the loading states, such as " ... 2 more", "- FAILED TO LOAD: some error", ...

Could you remind me please what actions in the vscode UI generate evaluate requests with "hover" and "clipboard" context. I know that right click + "Copy to Value" is "variabels" and using Debug Console is "repl".

"hover" -> when you hover over the variable from the editor.
"clipboard" -> "Copy Value" context menu from VARIABLES or WATCH view

@hyangah
Copy link
Contributor Author

hyangah commented Jun 4, 2021

PR description is updated. Seems like we all agreed on 256 for now.
@suzmue has a prototype of removing type info and slice/array's len/cap info from the value presentation and making them available in a different way (type info is already supplied as the separate type attribute)

@aarzilli aarzilli merged commit 152c74e into go-delve:master Jun 4, 2021
@polinasok
Copy link
Collaborator

"hover" -> when you hover over the variable from the editor.

I believe this is only true when you hover over the code, not in the Variables section.

@suzmue has a prototype of removing type info and slice/array's len/cap info from the value presentation
Should we consider smaller limit once the type info is dropped from the inlined value?

@polinasok
Copy link
Collaborator

@hyangah Oh and sorry for editing the comment instead of replying.

@hyangah
Copy link
Contributor Author

hyangah commented Jun 4, 2021

"hover" -> when you hover over the variable from the editor.

I believe this is only true when you hover over the code, not in the Variables section.

fyi @polinasok 'editor' refers to the main area where code/document is presented in vscode. 'views' or 'viewlets' are the sections presented in the side bar area.

suzmue pushed a commit to suzmue/delve that referenced this pull request Jun 4, 2021
- add 'clipboard' capability
- apply a larger string limit for 'hover' and 'clipboard' context
- truncate the string representation of compound (or pointer of compound)
type variable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

debug: 'value' of Variables response for a large struct is incomprehensible
5 participants