Honor new 'important' category of OutputEvents

[polinasok]

As a follow up to microsoft/debug-adapter-protocol#218, please highlight output messages tagged as 'important'. Perhaps log this in the console and trigger a showUser-like pop-up that disappears after a couple of moments?

[weinand]

@roblourens I've improved the output event support in Mock Debug. Now you can send output events for all output categories, e.g. prio(message) for "important" messages.

See https://github.com/microsoft/vscode-mock-debug/tree/main/sampleWorkspace#output-events

(since I haven't published a new release of Mock Debug to the market place, you will have to run out of source)

[roblourens]

I wonder whether these messages should also be marked somehow in the debug console?

[polinasok]

I wonder whether these messages should also be marked somehow in the debug console?

I think it's a great idea to have them stick out using something visual (bold font, different color or some other flag).

[weinand]

Yes, showing an important message in bold with a strong color (not red) makes sense.

@roblourens I've published a pre-release version (1.49.1) of Mock Debug to the Marketplace. It contains the "important category" support.

[roblourens]

@isidorn when I log an object, we ignore the category, why is that? I figure we should still show the "important" notification when logging an object

[weinand]

@roblourens I agree, we should not drop the category on logged objects.
Most likely, ignoring the category was an oversight.

[roblourens]

I guess we style the object differently and just don't have a style for it.

@misolori I wonder whether we should show an error icon next to logged errors in the debug console. Currently the only difference is the text color.

Then we could just show the same icon next to a logged object

[roblourens]

Implemented this. @polinasok out of curiosity can you give an example of when you will use this?

I noticed that since 'important' is a category, we can't mark a message as "important error" vs "important info". Is that your intention with the API change or would you want to be able to differentiate these things?

[weinand]

@roblourens from a DAP perspective there is no concept of "info" vs. "error".
DAP's "stderr" and "stdout" categories express the source/origin of a message and not the semantic (which is unknown to the DA anyway).
The "important" category can be considered as a variant of the "console" category: in both cases is the DA the source of the message and therefore the semantics and "importance" is known.

[weinand]

@roblourens cool!

[miguelsolorio]

If we ever do need an icon to represent this, we did start using this in the terminal for decorators

[roblourens]

Also if you expand that notification, the debug session name is set as the "source" so you can tell why it is showing up.

I will open another issue for setting an icon on debug console stderr messages. I think that could be an accessibility issue right @isidorn?

[polinasok]

We might want to use this for errors or for very important info messages (as opposed to just general logging that users might or might not need/want to inspect).

Here are some of the examples:

1. Build Errors
   We currently use OutputEvent(stderr). The message is better formatted in the Debug Console than in the launch error pop-up. And users find the pop-up annoying because they have to close it every time they iterate on their build errors.
2. Failure to set breakpoints
   When some breakpoints fail to set, we return a success response with a list of requested breakpoints and their status. The UI highlights the failed ones in a different way, and there is a hover with the error. But if none of the breakpoints are hit (e.g. we have a path mismatch in a remote debugging set-up), then the program might quickly run to completion and those markers would disappear as soon as the session is over, leaving the users with no record of what went wrong. A message in the Debug Console would remain after the session's end.
   See also discussion in Different breakpoint marker for unverified breakpoint before debug session start #135105.
3. Breakpoint on one thread interrupts a step operation on another
   This is not an error, and can look very unexpected to the users as the view jumps somewhere else. Also, we have the option of cancelling the step operation or pausing it and allowing it to be resumed with "Continue". Either way we need to let the user know what's going on and how to proceed.
4. When we fail to retrieve goroutines
   Go users generally care about goroutines (lightweight Go threads), but not so much about the underlying runtime threads, so that's we use the ThreadsRequest to communicate the info about the goroutines. But when the program is stopped on entry or in some other corner cases, we might not be able to retrieve any gorotuines. To play nice with DAP we are forced to send back a successful Dummy thread, which at the same time yields no usable Call Stack, so additional info in the debug console can be useful.

[isidorn]

@roblourens we should not ignore the category. I forgot the original intention when I coded that up.
Icon sounds like a good first step. For accessibility - we could just append the category at the end of the aria label of the REPL element.

[roblourens]

Verification steps

• Install the mock debug extension
• Clone this repo and debug this workspace using the mock debug extension: https://github.com/microsoft/vscode-mock-debug/tree/main/sampleWorkspace
• When stepping over the prio() line, you should get a notification in VS Code
• The "source" field on the notification is the name of the debug session

[polinasok]

The mock above shows a pop-up. Will it disappear on its own after a while or will the user always have to close it manually?

[roblourens]

Yeah, they timeout after 30s or so, and after that you can click the bell indicator to see the list of notifications.

[connor4312]

Seems to work. I wonder if the notifications should automatically be dismissed if they're still open when the session ends. It could be annoying if there was an error in a session that generated a lot of messages the user would need to manually close..

[weinand]

If the debug sessions end quickly because of the "important" issues shown as notifications, then dismissing them immediately is not that helpful. But dismissing them when starting a new session might be a better approach.

[connor4312]

That makes sense. They should only dismiss when a debug configuration of the same name as created them is started.

[roblourens]

If it's annoying, you can just hit esc, otherwise they will automatically hide relatively quickly. Is it that bad?

[roblourens]

@polinasok I was wondering whether you ever adopted this? I don't think I see any usages of the "important" category in the vscode-go or delve repos. I'm asking because I was rethinking the decision to make this a notification, in microsoft/debugpy#1019

[roblourens]

I guess I'm not sure whether @polinasok is still working on that, if not it looks like @hyangah has been active on it

[polinasok]

Delve-Dap has not adopted this yet. But it should. Pop-up is nice - similar to ErrorResponse with showUser. What's aa notification?