Make StoppedEvent's 'reason' attribute robust against translations

[weinand]

Today StoppedEvent's reason attribute is the only reason attribute that is shown in the UI. As a consequence debug adapters are expected to provide different translations for its value (but I'm only aware of a single fully translated debug adapter: node-debug).

A translated reason value makes it difficult for a client to interpret the value. Previously VS Code did not interpret the value but with the advent of microsoft/vscode#16860 VS Code has to do this (and other clients might want to do this as well).

So there is a need to have the reasoninformation both in a translated and untranslated form.

To address this we propose the following:

• we change the semantics of the existing reason attribute from being used in the UI to become the untranslated form. This means we will remove the comment "This string is shown in the UI" from the description of the reason attribute. With this the semantics of the StoppedEvent's reason attribute starts to match the other reason attributes.
• we introduce a new attribute description for the translated valued. This description will be shown in the UI 'as is', and it is not embedded in some other string. If this attribute is missing, clients are expected to fall back to use the reason attribute like before.

For debug extension authors this means the following:

• if the debug adapter does not return translated reason values, then no code changes are necessary.
• if the debug adapter returns translated values, then the debug adapter should be modified to return the translated value as the new description attribute, and start returning the untranslated value as the reason attribute.
• if a debug extension author wants to have more control over what gets displayed in the UI, she can return the reason as before, and provide a more descriptive string as the description attribute.

Comments, opinions?
@delmyers @gregg-miskelly @DanTup @WebFreak001 @roblourens @andysterland @hbenl @ramya-rao-a @felixfbecker @daviwil @rkeithhill @DonJayamanne @rebornix @isidorn @dlebu @michelkaporin @richardstanton @andrewcrawley @DavidKarlas

[WebFreak001]

Didn't it always either say exception, unknown or paused on breakpoint? I couldn't get any other strings to show even if I wanted to when I implemented the stopped event.

[weinand]

@WebFreak001 I've just tried this by adding a 'XXX' to the reason strings returned from node-debug and here is the result:

This proves that the 'reason' text is shown in the UI as is.

But from an i18n perspective it is probably better to have the description return the full text shown in the UI (e.g. 'Paused on exception') and not just only the 'exception'. I will add this to the proposal from above.

[felixfbecker]

Yeah, isn't this just a string enum like 'step' | 'breakpoint' | 'exception' etc?

[weinand]

@felixfbecker it looks a bit like a string enum, but the TypeScript declarations shows that it is not:

   /** The reason for the event (such as: 'step', 'breakpoint', 'exception', 'pause').
       This string is shown in the UI. */
   reason: string;

But if you have assumed that it is a string enum, then my proposal will work for you because the php debugger most likely does not translate these values, correct?

[felixfbecker]

@weinand The typings were written before TypeScript had string literals iirc, so I assumed they were an enum. The PHP debugger only ever sends these values:
https://sourcegraph.com/github.com/felixfbecker/vscode-php-debug/-/blob/src/phpDebug.ts#L297:1-298:1

I would suggest you just turn this into a real string enum.

[weinand]

@felixfbecker the value set has some predefined values but is extensible ("open"). I'll have to figure out how to express this as a JSON schema...

[gregg-miskelly]

@weinand sounds like a good plan to me

[DanTup]

If this attribute is missing, clients are expected to fall back to use the reason attribute like before.

I'm slightly confused - isn't it Code that will fall back to reason, there's nothing us to change in our debug adapters? (as far as I can tell, we only provide this value to Code; we never read/use/display the reason attribute ourselves?)

[weinand]

@DanTup yes, if you are not returning a translated reason in your debug adapter, then you don't have to change anything.

But if you are translating the reason then you should return the translated value as the new description attribute, and start to return the untranslated value as the reason attribute.

And if you want to have more control over what gets displayed in the UI, you can return the reason as before, and provide a more descriptive string as the description attribute.

I've added these 3 items to the proposal from above.

[DanTup]

Thanks for clarifying! 👍

[andrewcrawley]

@weinand - What's the meaning of "has some predefined values but is extensible"? If the reason field is now intended to be interpreted by the host, what does it mean if I pass arbitrary text? I think an enum is appropriate here.

Going by other notes in the protocol, I think the following are necessary enum values: step | breakpoint | exception | pause | restart | goto. entry might also be a good one to have.

[weinand]

@andrewcrawley currently only a single predefined value ('exception') will be interpreted by the client. All other predefined values are not (yet) interpreted but they are displayed in fallback mode (if no 'description' attribute is given).
But if a debugger 'foo' needs to introduce a reason 'bar', this should be possible. So the enum must be open for additional values. I don't know how to express this in the JSON schema.

[andrewcrawley]

Why would a debugger "need" to introduce an additional reason if no host interprets that reason to mean anything? It sounds like that would be a case for someone to file an issue and request a new enum value.

Plus, VS does differentiate between reasons - stopping on a breakpoint is different from stopping on an exception, which is different from stopping on entry, etc.

[weinand]

@andrewcrawley For compatibility.
Today 'reason' is just a string that is rendered in the UI. We cannot constrain this by introducing a fixed set of enum values because this would break existing debug adapters.
But we want to specify the "well known" values that are interpreted by clients (e.g. 'breakpoint', 'exception', 'pause', 'restart', 'goto', 'entry').
If the 'foo' debugger adds a reason 'bar' no client will be able to interpret this, but VS Code will render the reason 'bar' in the UI without problems.