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

Fix: Wait for clear command to execute before reusing terminal #240970

Merged
merged 7 commits into from
Feb 19, 2025

Conversation

tcvdh
Copy link
Contributor

@tcvdh tcvdh commented Feb 17, 2025

clear command was in the middle of executing while the main (debugger) command started executing. Adding a small timeout to make sure the clear command executes before allowing the main command to be sent to the terminal.

this would happen when the settings.json has these settings:

{
  "debug.terminal.clearBeforeReusing": true,
  "terminal.integrated.focusAfterRun": "terminal",
}

Fixes #240953

@VACLAVHAVLIS
Copy link

V pohodě, jsem moc spokojen

@connor4312
Copy link
Member

We should not just blindly add a delay. If shell integration is available (the common case) we should use TerminalShellIntegration.executeCommand and avoid having to delay.

@tcvdh
Copy link
Contributor Author

tcvdh commented Feb 18, 2025

@connor4312 if you check the lines above the pr code:

if (terminal.state.isInteractedWith && !terminal.shellIntegration) {
terminal.sendText('\u0003'); // Ctrl+C for #106743. Not part of the same command for #107969
await timeout(200); // mirroring https://github.com/microsoft/vscode/blob/c67ccc70ece5f472ec25464d3eeb874cfccee9f1/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts#L852-L857
}

you can see after the ^C there is also a 200ms delay (which was related to an issue exactly the same as this, except about ^C being in the terminal command instead of clear). so i used the same fix/copied that for that on this issue.

for the clearBeforeReusing code, there isn't an if statement making sure that there is a terminal.shellIntegration (so we can't use it yet). so if you suggest using the shellIntegration for this instead of a delay, then where should there be an added if for the terminal.shellIntegration? and what if terminal.shellIntegration isn't true? should we then fall back to the delay? To me it makes more sense to copy the delay from the few lines above as it works always and doesn't depend on if terminal.shellIntegration is there or not.

if (terminal.shellIntegration) {
    terminal.shellIntegration.executeCommand('clear');
}

So what do you suppose is the best way to handle this? add an if statement like above to check if it is available? and if it isn't available fall back to the old solution with a delay?

(or add the if statement to the main if clearBeforeReusing statement, but this would only allow the setting clearBeforeReusing if the shellIntegration is also available, which isn't always the case)

@tcvdh
Copy link
Contributor Author

tcvdh commented Feb 18, 2025

@microsoft-github-policy-service agree

@tcvdh
Copy link
Contributor Author

tcvdh commented Feb 18, 2025

@connor4312 i have updated the PR code, i added the shellIntegration and kept the delay only for when the shellIntegration isn't available. Can you review my PR again to see if this is what you mean and are satisfied with this implementation?

connor4312
connor4312 previously approved these changes Feb 18, 2025
@connor4312 connor4312 enabled auto-merge (squash) February 18, 2025 18:19
@vs-code-engineering vs-code-engineering bot added this to the February 2025 milestone Feb 18, 2025
DonJayamanne
DonJayamanne previously approved these changes Feb 19, 2025
@Tyriar Tyriar self-assigned this Feb 19, 2025
auto-merge was automatically disabled February 19, 2025 12:07

Head branch was pushed to by a user without write access

@tcvdh tcvdh dismissed stale reviews from DonJayamanne and connor4312 via 37ac155 February 19, 2025 12:07
Tyriar
Tyriar previously approved these changes Feb 19, 2025
Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@connor4312 connor4312 enabled auto-merge (squash) February 19, 2025 16:17
@connor4312 connor4312 merged commit 384ae8e into microsoft:main Feb 19, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants