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

Add an API for specifying terminal location #45407

Closed
Tyriar opened this issue Mar 9, 2018 · 31 comments · Fixed by #131028 or #138530
Closed

Add an API for specifying terminal location #45407

Tyriar opened this issue Mar 9, 2018 · 31 comments · Fixed by #131028 or #138530
Assignees
Labels
api api-finalization feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders on-release-notes Issue/pull request mentioned in release notes terminal General terminal issues that don't fall under another label verification-needed Verification of issue is requested verified Verification succeeded
Milestone

Comments

@Tyriar
Copy link
Member

Tyriar commented Mar 9, 2018

Here are two options:

export interface TerminalOptions {
  splitFrom?: Terminal
}

or

export interface Terminal {
  split(options: TerminalOptions): Terminal;
}

/cc @fabiospampinato

@Tyriar Tyriar added feature-request Request for new features or functionality api terminal General terminal issues that don't fall under another label labels Mar 9, 2018
@Tyriar Tyriar added this to the On Deck milestone Mar 9, 2018
@Tyriar Tyriar self-assigned this Mar 9, 2018
@fabiospampinato
Copy link
Contributor

They both look pretty good to me.

Maybe I'm leaning a bit toward the latter, especially if split direction and shell/args/env inheriting were to be implemented in the future, I think this:

export interface TerminalSplitOptions {
  direction: string,
  inherit: boolean
};

export interface Terminal {
  split(options: TerminalOptions, splitOptions?: TerminalSplitOptions): Terminal;
}

Looks a little nicer than this:

export interface TerminalOptions {
  splitFrom?: Terminal
  splitDirection?: string,
  splitInherit?: boolean
}

@williamluke4
Copy link

Has there been any movement on this?

@ScriptPup
Copy link

Bump, looks like there hasn't been anything for quite a while. Is this still being worked? Thanks!

@99lives
Copy link

99lives commented Sep 12, 2019

Bump again

@Tyriar
Copy link
Member Author

Tyriar commented Oct 8, 2019

Note to self: we should probably align to how editor splitting works in the API, also if terminal tabs could be pulled into the editor area how does that affect things?

@Tyriar
Copy link
Member Author

Tyriar commented Oct 8, 2019

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

@Tyriar
Copy link
Member Author

Tyriar commented Oct 14, 2019

Discussed this in the API meeting today.

Creation vs placement

One big thing that has blocked this has been wanting to wait to see how the API ends up looking once positioning is more dynamic within the editor area and when terminal tabs (#10546) has arrived. I don't think we are actually blocked on this for the following reasons:

  • We should not try to combine creation and placement APIs. Take the editor for example, showTextDocument doesn't talk about creation it's primarily an API centered around placing a text document somewhere. I can imagine a showTerminal or moveTerminal API in the future which achieves this not just for moving terminal groups but also 'unsplitting' terminals, we can explore that more in the future though.
  • Splits are not tabs. When we add tabs, splits will remain within a single tab so they will be treated as a whole (ie. a 'terminal group', similar to editor groups).

Split direction

Terminals only split in a single direction, this was initially done for simplicity reasons and because the panel is typically quite narrow. We should probably stick to this philosophy when we allow terminals within tabs, for an elaborate layout it might be better to go with single terminal instances using the editor group's grid system.

Complexity around default split direction within the editor and the ability to toggle the split direction could be provided by commands exposed in the command palette.

Approach

We discussed the following two high-level approaches. Specifying how to split via options:

export interface TerminalOptions {
  splitFrom?: Terminal
}

Or via a method on Terminal:

export interface Terminal {
  split(options: TerminalOptions): Terminal;
}

We preferred the former, the wording needs some refinement but using "beside" to align with ViewColumn.Beside was suggested:

export interface TerminalOptions {
  splitBeside?: Terminal
}

This ties into a parallel discussion we're having in #63052 (comment) which would see this terminal exposed on Terminal.creationOptions. An interesting note here is that if the parent that a terminal was split from is disposed then this will keep hold of the disposed terminal reference (just as if you held onto Terminal in some other way). This is how editors work and shouldn't be a big deal.

Restoring terminal layout

If an extension were to restore terminal layout exactly as it was between reloads then it could iterate over window.terminals and identify the layout based on Terminal.splitBeside, unless a terminal is disposed or unless (when we support it) you can unsplit terminals. At that point we would need a dynamic property that specifies what terminal is next to the current terminal, for example:

export interface Terminal {
	// splitBeside as well?
	adjacentTerminal: Terminal | undefined;
}

This is because while TerminalOptions.splitBeside will likely be accessible in the future via Terminal.creationOptions, that one must be static.

@vallamost
Copy link

Can we add horizontal splitting support into this as well? It would be nice to split code and terminals panes horizontally..

@Tyriar
Copy link
Member Author

Tyriar commented Aug 10, 2021

Main feedback was remove TerminalLocation.Default as TerminalOptions.location is optional, that way it also fits in nicer if we end up including a Terminal.location.

  • Q: Do we need to specify a side?
    • Let's add this later if there's demand.
  • Q: Should we expose TerminalLocation on the Terminal interface?
    • We could add this layer, we'd want to do this if we allowed moving terminals to a different location via the API.

meganrogge added a commit that referenced this issue Aug 16, 2021
@Tyriar Tyriar reopened this Aug 20, 2021
@Tyriar
Copy link
Member Author

Tyriar commented Aug 20, 2021

Will need the test plan and then move this to Sept for finalization. We might want to wait a bit longer for this to make sure extensions pick it up but we can figure that out next month.

@Tyriar Tyriar modified the milestones: August 2021, September 2021 Aug 23, 2021
@Tyriar Tyriar changed the title Add an API for splitting a terminal Add an API for specifying terminal location Aug 27, 2021
@Tyriar
Copy link
Member Author

Tyriar commented Aug 27, 2021

Those interested in this we'd love to hear feedback on the proposal:

//#region Terminal location https://github.com/microsoft/vscode/issues/45407
export interface TerminalOptions {
location?: TerminalLocation | TerminalEditorLocationOptions | TerminalSplitLocationOptions;
}
export interface ExtensionTerminalOptions {
location?: TerminalLocation | TerminalEditorLocationOptions | TerminalSplitLocationOptions;
}
export enum TerminalLocation {
Panel = 1,
Editor = 2,
}
export interface TerminalEditorLocationOptions {
/**
* A view column in which the {@link Terminal terminal} should be shown in the editor area.
* Use {@link ViewColumn.Active active} to open in the active editor group, other values are
* adjusted to be `Min(column, columnCount + 1)`, the
* {@link ViewColumn.Active active}-column is not adjusted. Use
* {@linkcode ViewColumn.Beside} to open the editor to the side of the currently active one.
*/
viewColumn: ViewColumn;
/**
* An optional flag that when `true` will stop the {@link Terminal} from taking focus.
*/
preserveFocus?: boolean;
}
export interface TerminalSplitLocationOptions {
/**
* The parent terminal to split this terminal beside. This works whether the parent terminal
* is in the panel or the editor area.
*/
parentTerminal: Terminal;
}
//#endregion

It'll probably be best to wait for real feedback as this is a pretty impactful API we don't want to get wrong.

@Tyriar
Copy link
Member Author

Tyriar commented Oct 28, 2021

Still waiting for feedback 🙂 @fabiospampinato?

@Tyriar Tyriar modified the milestones: November 2021, December 2021 Oct 28, 2021
@fabiospampinato
Copy link
Contributor

@Tyriar Sorry I haven't thought about this in a while. The API proposal seems good to me, the only thing I sort of don't like is that the interface is a bit different depending on where the terminal instance is located, for example I can split a particular terminal if I'm in the panel, but it seems that I can only split the current terminal/editor if I'm in the editor area. Also it might be interesting to allow to set a split direction too, in order to be able to make complex 2d layouts, but it's unclear to me if there's an actual need for that, better not to overcomplicated the API for now until the situation is clearer IMO. Overall it seems good to me 👍

@github-actions github-actions bot locked and limited conversation to collaborators Jan 20, 2022
@hediet hediet added the verification-needed Verification of issue is requested label Jan 25, 2022
@jrieken jrieken added the verified Verification succeeded label Jan 26, 2022
@meganrogge meganrogge added the on-release-notes Issue/pull request mentioned in release notes label Jan 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api api-finalization feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders on-release-notes Issue/pull request mentioned in release notes terminal General terminal issues that don't fall under another label verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

Successfully merging a pull request may close this issue.

17 participants