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

Try use shell integration in debug terminals #205034

Merged
merged 1 commit into from
Feb 23, 2024
Merged

Try use shell integration in debug terminals #205034

merged 1 commit into from
Feb 23, 2024

Conversation

Tyriar
Copy link
Member

@Tyriar Tyriar commented Feb 12, 2024

Fixes #204694

Shell integration when debugging xterm.js' unit tests with console: integratedTerminal set.

Screenshot 2024-02-12 at 11 48 46

@Tyriar Tyriar added this to the February 2024 milestone Feb 12, 2024
@Tyriar Tyriar self-assigned this Feb 12, 2024
Copy link
Contributor

@karrtikr karrtikr left a comment

Choose a reason for hiding this comment

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

Thanks. Waiting on debug team: #204694 (comment) before approving.

Copy link
Contributor

@karrtikr karrtikr left a comment

Choose a reason for hiding this comment

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

Based on #204694 (comment) it looks like this will also require changes to RunInterminalRequestArguments.

@Tyriar
Copy link
Member Author

Tyriar commented Feb 20, 2024

What was the issue with always enabling this?

@karrtikr
Copy link
Contributor

If we can always enable shell integration by default (thereby enabling it for debug terminals) that'd be great.

@Tyriar
Copy link
Member Author

Tyriar commented Feb 20, 2024

Let's merge this early next iteration and see if there's any negative feedback for insiders

@Tyriar Tyriar requested a review from karrtikr February 20, 2024 18:09
@Tyriar Tyriar modified the milestones: February 2024, March 2024 Feb 20, 2024
* Attempt to force shell integration to be enabled by bypassing the {@link isFeatureTerminal}
* equals false requirement.
*/
forceShellIntegration?: boolean;
Copy link
Contributor

@karrtikr karrtikr Feb 20, 2024

Choose a reason for hiding this comment

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

Looking at this PR, I'm not clear if it is always enabled for debug terminals by default. Can you clarify? Looks to me like we're not forcing shell integration as default.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not always, but it will attempt to just like any other terminal. Previously "feature terminals" would all be excluded.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, who is using forceShellIntegration option? Note Python extension cannot access this property at the moment when debugging.

Copy link
Member Author

Choose a reason for hiding this comment

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

@karrtikr it's internal only (see no vscode.d.ts change) and only used right now for debug terminals

Copy link
Contributor

Choose a reason for hiding this comment

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

I see ok

@Tyriar Tyriar merged commit 2c3c6f8 into main Feb 23, 2024
6 checks passed
@Tyriar Tyriar deleted the tyriar/204694 branch February 23, 2024 19:42
@microsoft microsoft locked and limited conversation to collaborators Jun 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Shell integration isn't enabled for Debug terminals
3 participants