Extend RunInTerminalRequestArguments to allow clearing the previous terminal history automatically

[testforstephen]

The runInTerminal request will try to run the subsequent commands from the same terminal again, and the previous run history will remain on the current screen. This is not clean for some users who want to be able to automatically clear the old terminal output before running the new program.

One suggestion is to add a new property such as clearTerminal to the RunInTerminalRequestArguments so that the client can perform additional clearing according to the new property.

interface RunInTerminalRequestArguments {
  /**
   * What kind of terminal to launch.
   * Values: 'integrated', 'external', etc.
   */
  kind?: 'integrated' | 'external';

  /**
   * Optional title of the terminal.
   */
  title?: string;

  /**
   * Clear the current terminal before running a new terminal request.
   */
  clearTerminal?: boolean;

  /**
   * Working directory for the command. For non-empty, valid paths this
   * typically results in execution of a change directory command.
   */
  cwd: string;

  /**
   * List of arguments. The first argument is the command to run.
   */
  args: string[];

  /**
   * Environment key-value pairs that are added to or removed from the default
   * environment.
   */
  env?: { [key: string]: string | null; };
}

[puremourning]

The runInTerminal request will try to run the subsequent commands from the same terminal again

Where in the spec does it say that? My client doesn’t do this.

[testforstephen]

i didn't see there is a spec to declare this, but at least VS Code client will try to reuse the same terminal to handle the request.

[weinand]

Yes, the spec doesn't say anything about reusing terminals, but this does not prevent clients from implementing it like this.

I suggest:

• we extend the description of runInTerminal to include a statement about "reusing terminals"
• we support features like "clearTerminal" as "hints": clients are free to use them or ignore them. And adapters cannot rely on them.
• we might want to consider another hint to "disable" reuse of terminals.

[testforstephen]

looks good to me.

[puremourning]

But should this flag really be ‘newTerminal’ or something that says this command is not related to the previous one? Otherwise just include a ‘clear’ command in your requests?

clear screen is quite specific and seems like a workaround for the vscode terminal reuse behaviour.

In practice I’m not sure How would a client implement that generically? Client might not even know how to issue a clear command for the terminal. Is ctrl-l universal? Does the clear command exist on $OS? Would it be legit to just start a new terminal and discard the existing one to ‘clear’ the old one?

Either way I would advocate for not baking assumptions into the protocol based on what one particular client does today. If it’s required that runInTerminal starts a shell (it’s not afict) and that subsequent messages are commands in that shell (not currently, despite the recent clarification about cd commands, which I guess I now understand based on this context), then we should document that clearly before baking further potentially confusing knobs like this into the protocol.

Just my opinion as a client maintainer .

Vimspector starts the command in a terminal window. There is no ‘shell’ just the command running. The command is started with the WD as supplied and with the environment as supplied. So there is no 'cd’ (the other recent change). If the previous command has completed, then a new terminal buffer is started with the new command and the old one discarded. If the previous one is still running, then the terminal window is split to create 2.

If that’s wrong in some way then I need to change it, but I don’t want to change it unless the protocol says clients must behave in a specific way and I don’t want servers to assume behaviour based on one specific client implementation.

But if not, based on that, I would say this switch is not required. by adding it I have a total headache: rewrite my implementation to follow vscode odd behaviour, or fail to respect the switch, which could lead to problems down the line. Do you see my concern?

[puremourning]

After writing that essay I just noticed @weinand comment which as usual is right on point. 👍

I’m not a fan of these ‘hints’ but it certainly solves the problem to say that servers must not rely on them.

[weinand]

@puremourning your implementation in Vimspector is totally correct and we don't want you to change anything.
That was the purpose of my "hint" based proposal.

[puremourning]

Yep, thanks. I totally missed your comment, which I pretty much wholly agree with :)

[mfussenegger]

In which scenarios would a debug adapter implementation want to set these hints? Not sure I see the advantage of having this as part of the protocol and up to the debug adapter implementations to influence instead of letting users configure the behavior in their dap-client?

[weinand]

@mfussenegger you raised a good point:

As already noted by others, the DAP spec says nothing about "reusing terminals".
The fact that VS Code tries to reuse terminals is an implementation detail (and feature) of this particular client.
Consequently you could argue that it is on VS Code to offer any configuration options related to this and there is no need to "loop" these options through DAP.

Lets try to find the pros and cons of the two approaches by having a more detailed look at the initial "clear terminal history" feature:

• the "clear" feature depends on the client implementation of terminal reuse (and lives in the client only).
• when a "clear" hint is offered in DAP (= DAP approach), a DA can either hardcode a value for this hint or the DA can provide a configuration mechanism: e.g. "clear" can be specified in a launch config or as a debug-type specific configuration (user of workspace setting).
• when "clear" is just offered by the client and not known to DAP (= Client approach), it can only be a user or workspace setting. Having it as a generic option in the launch config is no longer possible because we've made the decision not to have a generic "console" property in the launch config either (so VS Code does not know whether a debug extension has a way to configure a terminal or console).

Here are some pros and cons for the two approaches:

• DAP approach:
  • ⨁ every debug extension has the full flexibility in surfacing the feature (if at all and how),
  • ⊖ every debug extension can surface the feature in different ways which results in inconsistencies,
  • ⊖ every debug extension needs to "opt into" the feature (more effort).
• Client approach:
  • ⨁ since "clear" feature is generic it automatically works for all debug extensions that use the "runInTerminal" request,
  • ⨁ DAP is not polluted with client specific "hints",
  • ⊖ debug extensions know nothing about the "clear" feature and cannot provide specific defaults or surface the feature in a specific way.

@testforstephen @puremourning @mfussenegger any additional pros and cons? what do you think?

[int19h]

I think there may be some desire from the users to control it on a per-workspace basis. If that requires flowing it through DAP for the sake of ease of implementation, it's not a hassle from the adapter perspective.

But if VSCode can somehow do this transparently, without involving the adapter at all, I think that's preferable. I just can't think of a scenario where the adapter would want to control this, other than flowing through the setting from launch.json.

[puremourning]

Client only sounds more appropriate to me. There’s nothing stopping the setting/option working on a per-project/per config basis in the client:

• In a vscode world where you have adapter-specific client code which intercepts launch config
• In other worlds where the config format is owned by the client (modulo the actual launch/attach block)
• Etc

[weinand]

I suggest that we close this DAP feature request in favour of a new VS Code request that asks for new settings to control how a terminal is reused when launching debuggees.

@testforstephen is this OK for you?

[testforstephen]

@weinand Sounds better to introduce a setting in client to control this. Thank you all for the great idea. I will close this request.