fix #6767: don't reuse integrated terminal that has child processes

[a1994846931931]

Signed-off-by: Cai Xuye [email protected]

What it does

Fix #6767.

How to test

1. Check whether the reuse of the integrated terminal is still working.
2. Check if an integrated terminal is in use, it won't be reused.
   Demo gif for the 1st commit (propose 1 in Integrated terminal should not be resused if a process is running in it #6767):
   
3. Check if the title of the integrated terminal keeps its original value
   Update gif for the 2nd commit (propose 2 in Integrated terminal should not be resused if a process is running in it #6767):
   

I've tested in only on Linux. Could anyone test it on windows as well? Thanks!

Review checklist

• as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• as a reviewer, I agree to behave in accordance with the review guidelines
• CQ approval needed before merge https://dev.eclipse.org/ipzilla/show_bug.cgi?id=21340

[paul-marechal]

Thanks for the patch @a1994846931931, please note that I am working on the shell argument escaping for the runInTerminal request, I don't know if you had issues with this too.

[a1994846931931]

Thanks for the patch @a1994846931931, please note that I am working on the shell argument escaping for the runInTerminal request, I don't know if you had issues with this too.

@marechal-p
Hi, I do notice that we simply join the argument before sending it as the command to terminal:

theia/packages/debug/src/browser/debug-session.tsx

Line 372 in f54e970

terminal.sendText(args.join(' ') + '\n');

which may be problematic, while VSCode's implementation is far more complicated:
https://github.com/microsoft/vscode/blob/4636be2b71c87bfb0bfe3c94278b447a5efcc1f1/src/vs/workbench/contrib/debug/node/terminals.ts#L79-L210

Although I haven't encountered any broken issues so far, it reminds me of a line in cdt-gdb-adapter:
https://github.com/eclipse-cdt/cdt-gdb-adapter/blob/7b04c93f7e3a0646f6be5b6eac45c5f071e04829/src/GDBBackend.ts#L67

        let args = [gdb, '-ex', `new-ui mi2 ${pty.name}`];

where the argument new-ui mi2 ${pty.name} should be quoted before being send as a text command.
So I tested it on theia, and it does have a problem:

I hope the information helpful.

[paul-marechal]

Yes, thank you for the infos. So again, just so that we do not duplicate work: I already have a commit for the command escaping, I will open and attend a PR with it first week of January as I will be taking vacations. With your patch + the commit I made, we should be pretty close to what VS Code does :)

[a1994846931931]

Yes, thank you for the infos. So again, just so that we do not duplicate work: I already have a commit for the command escaping, I will open and attend a PR with it first week of January as I will be taking vacations. With your patch + the commit I made, we should be pretty close to what VS Code does :)

@marechal-p
Ah, I see! Thanks for reminding me. And luckily, this PR seems will not conflict with yours~

[akosyakov]

@elaihau @vince-fugnitto @marechal-p please finish the review

[vince-fugnitto]

I created a CQ for the following copied code:
https://dev.eclipse.org/ipzilla/show_bug.cgi?id=21340

[paul-marechal]

Changes LGTM, shells are also not reused if a process is already running. Should be good to merge once the CQs are resolved.

[elaihau]

I played with the change and found the following behavior - should this scenario be covered by this change? Maybe it is unrelated?

[a1994846931931]

I played with the change and found the following behavior - should this scenario be covered by this change? Maybe it is unrelated?

@elaihau
No, it's not covered in this PR. But similar behavior could be realized, by changing:

theia/packages/task/src/browser/task-service.ts

Line 924 in 44efdc4

if (!terminal) {

to

if (!terminal || (await terminal.hasChildProcesses())) {

after this commit.

Nevertheless, however, I'm not sure whether the Run selected text command should open a new terminal when the current one is running a program. I just tested it on VSCode, and it actually sends the text directly to the current one, even if it's running a program.

[vince-fugnitto]

The CQ has finally been approved! 🍾

[akosyakov]

@a1994846931931 please merge!