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

Split terminal should respect terminal options of the parent terminal #45218

Closed
IlyaBiryukov opened this issue Mar 7, 2018 · 20 comments
Closed
Assignees
Labels
api api-proposal feature-request Request for new features or functionality *out-of-scope Posted issue is not in scope of VS Code terminal General terminal issues that don't fall under another label

Comments

@IlyaBiryukov
Copy link

Issue Type: Feature Request

When our extension creates a vscode.Terminal and then user splits it, the new split terminal should use the same terminal options as the original one. What I see is that it does not respect shellPath and shellArgs in vscode.TerminalOptions used to create the original terminal. It just spawns plain Powershell for me on Windows.

VS Code version: Code - Insiders 1.21.0-insider (9a04587, 2018-03-01T08:09:20.917Z)
OS version: Windows_NT x64 10.0.16299

@vscodebot vscodebot bot added insiders and removed insiders labels Mar 7, 2018
@kieferrm kieferrm added the terminal General terminal issues that don't fall under another label label Mar 7, 2018
@kieferrm kieferrm added the feature-request Request for new features or functionality label Mar 7, 2018
@fabiospampinato
Copy link
Contributor

Maybe another command could be added for doing this. I don't think the second terminal should always inherit the configuration of the first, if you need that you can just set those settings in your global settings.json file.

@IlyaBiryukov
Copy link
Author

It will be confusing for users if split terminal is different from what the user split. We have an extension that starts a custom terminal with custom shellPath and shellArgs. If user splits it and gets a default terminal, it will definitely be inconsistent and confusing.
I'm fine with making the proposed behavior not default if it is customizable in TerminalOptions. E.g. have splitTerminalInheritsParentTerminal: boolean field on TerminalOptions.

@Tyriar
Copy link
Member

Tyriar commented Mar 8, 2018

@IlyaBiryukov this is as designed but I didn't think about the extension case much. For terminals launched via tasks and debugging for example the current behavior is exactly what you want; you "split" the terminal to get your standard go to terminal, rather than trying to spawn the same task or debug session again in a new terminal.

I'm fine with making the proposed behavior not default if it is customizable in TerminalOptions. E.g. have splitTerminalInheritsParentTerminal: boolean field on TerminalOptions.

This is a pretty good idea, I'm a little worried about an extension dictating how I want to use the terminal though. What does your extension do exactly?

@Tyriar Tyriar added the info-needed Issue requires more information from poster label Mar 8, 2018
@IlyaBiryukov
Copy link
Author

Our extension kicks off a terminal that starts our process that redirects stdio to a remote endpoint. So, the terminal is "virtual" and in fact doesn't even run on the user machine.

Another option for split action extensibility is to be able to provide an event handler in vscode.TerminalOptions that would be called when users click on the split button. That handler may then decide how to spawn the split terminal. I fact, I like this option even more than a boolean flag. It'll be more flexible and can start split terminal with more context.

@Tyriar
Copy link
Member

Tyriar commented Mar 9, 2018

Maybe something like this?

export interface TerminalOptions {
  getSplitTerminalOptions: () => TerminalOptions;
}

Related #45407, #13267

@Tyriar Tyriar added api and removed info-needed Issue requires more information from poster labels Mar 9, 2018
@IlyaBiryukov
Copy link
Author

Yup. Looks good.

@tomasaschan
Copy link

Not sure if this falls under what's already discussed here, but I'd like to add a use case that prompted me to search for solutions and find this issue:

I'm using the ShellLauncher extension to make it easy to launch PowerShell or WSL Bash terminals according to my needs. When I launch a WSL Bash terminal and split it, I expect to get another Bash, not a PowerShell.

@Tyriar
Copy link
Member

Tyriar commented Jun 22, 2018

@IlyaBiryukov you don't need this change with the new design in Live Share right?

@IlyaBiryukov
Copy link
Author

@lostintangent do we still need split terminal to be shared too in VSLS?

@Tyriar
Copy link
Member

Tyriar commented Jun 25, 2018

@IlyaBiryukov wouldn't #45407 be needed instead of this I mean once terminal renderers are adopted?

@IlyaBiryukov
Copy link
Author

Yep, #45407 should suffice in place of this one.

@Tyriar Tyriar added under-discussion Issue is under discussion for relevance, priority, approach and removed feature-request Request for new features or functionality labels Sep 12, 2018
@Tyriar Tyriar added api-proposal feature-request Request for new features or functionality and removed under-discussion Issue is under discussion for relevance, priority, approach labels Oct 8, 2019
@Tyriar
Copy link
Member

Tyriar commented Oct 8, 2019

Proposal:

export interface TerminalOptions {
  /**
   * Whether terminals created by splitting this terminal will inherit these options. This is
   * particularly useful for extensions that launch custom shells.
   */
  splitsInheritOptions?: boolean;
}

@IlyaBiryukov let me know if you really need the callback, a simple flag that passes everything on would be much simpler and lower the chance for accidental abuse.

FYI @TylerLeonhardt this might be useful for PS as well, let me know if you have feedback.

The only problem I see with the above is what @fabiospampinato called out in #45218 (comment), the user may not want the settings to be inherited for something like PowerShell (which is more of a preference like the splitCwd setting), for Live Share it would make much more sense to always work though to enable automatic sharing.

@Tyriar
Copy link
Member

Tyriar commented Oct 8, 2019

Adding to October to discuss, it won't happen in October though.

@Tyriar Tyriar added this to the October 2019 milestone Oct 8, 2019
@Tyriar
Copy link
Member

Tyriar commented Oct 9, 2019

Related to this discussion is how terminal.integrated.splitCwd works currently and how there is no way for users to opt in to splitting using the current shell #73194

@Tyriar
Copy link
Member

Tyriar commented Oct 9, 2019

This probably needs more discussion for the non-extension case before I bring this up in the API sync.

@Tyriar Tyriar removed this from the October 2019 milestone Oct 9, 2019
@IlyaBiryukov
Copy link
Author

@Tyriar we now use Pseudoterminal with vscode.window.createTerminal(options: ExtensionTerminalOptions).
Split terminal won't be able to just "inherit" the pty with a simple flag.
It will have to be a callback that would provide another ExtensionTerminalOptions or Pseudoterminal.

@Tyriar
Copy link
Member

Tyriar commented Oct 9, 2019

@IlyaBiryukov on the host side too?

@IlyaBiryukov
Copy link
Author

@Tyriar yes, on the host side too.

@Tyriar
Copy link
Member

Tyriar commented Oct 10, 2019

Closing this off then, no point discussing this if you wouldn't use it. This might be refined later on when the story for non-extension split behavior is expanded upon but for now I'm not keen on letting splits inherit Pseudoterminals somehow.

@Tyriar Tyriar added the *out-of-scope Posted issue is not in scope of VS Code label Oct 10, 2019
@vscodebot
Copy link

vscodebot bot commented Oct 10, 2019

This issue is being closed to keep the number of issues in our inbox on a manageable level, we are closing issues that are not going to be addressed in the foreseeable future: We look at the number of votes the issue has received and the number of duplicate issues filed. More details here. If you disagree and feel that this issue is crucial: We are happy to listen and to reconsider.

If you wonder what we are up to, please see our roadmap and issue reporting guidelines.

Thanks for your understanding and happy coding!

@vscodebot vscodebot bot closed this as completed Oct 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api api-proposal feature-request Request for new features or functionality *out-of-scope Posted issue is not in scope of VS Code terminal General terminal issues that don't fall under another label
Projects
None yet
Development

No branches or pull requests

5 participants