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

debug: show output test when present with variablesReference #172880

Merged
merged 1 commit into from
Feb 1, 2023

Conversation

connor4312
Copy link
Member

Previously, if a DAP output event contained a variablesReference, we
would entirely ignore the output text in favor of shows its variables.
However, output can be richer than variable data: namely, it can show
ANSI sequences which would be inappropriate to format in
a variable value.

In this PR, if an output event has a single variable in its
variablesReference and output text, we show the output text instead
of the variable value. (Maybe we should also log output as plain text,
though this could be a confusing experience. I don't know of any DA's
that actually emit >1 variable in their output.)

For #172868
For #171732

(will need adoption in js-debug to actually fix those)

Previously, if a DAP output event contained a variablesReference, we
would entirely ignore the `output` text in favor of shows its variables.
However, output can be richer than variable data: namely, it can show
ANSI sequences which would be inappropriate to format in
a variable `value`.

In this PR, if an output event has a single variable in its
`variablesReference` and output text, we show the output text instead
of the variable value. (Maybe we should also log output as plain text,
though this could be a confusing experience. I don't know of any DA's
that actually emit >1 variable in their output.)

For #172868
For #171732

(will need adoption in js-debug to actually fix those)
@connor4312 connor4312 self-assigned this Jan 31, 2023
@connor4312 connor4312 enabled auto-merge (squash) January 31, 2023 01:00
@vscodenpa vscodenpa added this to the February 2023 milestone Jan 31, 2023
@roblourens
Copy link
Member

One thing I noticed (which isn't new) is that the Process exited message shows up before other output, should I open an issue for that?

image

@roblourens
Copy link
Member

Is this supposed to be colored now?
image

@connor4312
Copy link
Member Author

Is this supposed to be colored now?

No, it need adoption in js-debug first. js-debug always sends an empty output string for variables, so you always see the old behavior.

One thing I noticed (which isn't new) is that the Process exited message shows up before other output, should I open an issue for that?

Yep, looks like an ordering bug in core. I'll fix that since I know where to look.

@connor4312
Copy link
Member Author

Oh, nevermind. It is an ordering problem but I'm not sure it's core's thing to fix. The "process exited" is sent from the 'parent 'virtual' debug session, where the console logs are sent from the child session, so they are enqueued separately in core. Will have to think of the right way to deal with that, but that would also be fixed with microsoft/debug-adapter-protocol#85

@connor4312 connor4312 merged commit edde095 into main Feb 1, 2023
@connor4312 connor4312 deleted the connor4312/debug-repl-show-output-text branch February 1, 2023 17:57
connor4312 added a commit to microsoft/vscode-js-debug that referenced this pull request Feb 3, 2023
connor4312 added a commit to microsoft/vscode-js-debug that referenced this pull request Feb 3, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Mar 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants