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

Migrate Terminal Panel to ViewPane #90252

Merged
merged 5 commits into from
Feb 10, 2020
Merged

Conversation

sbatten
Copy link
Member

@sbatten sbatten commented Feb 7, 2020

This PR refs #89729

@sbatten sbatten added terminal Integrated terminal issues workbench-views Workbench view issues labels Feb 7, 2020
@sbatten sbatten added this to the February 2020 milestone Feb 7, 2020
@sbatten sbatten self-assigned this Feb 7, 2020
@Tyriar
Copy link
Member

Tyriar commented Feb 8, 2020

Lowercase "hide" on right click? Isn't the terminal check item enough below also?

Screen Shot 2020-02-08 at 1 33 47 PM

@Tyriar
Copy link
Member

Tyriar commented Feb 8, 2020

I don't know how to move it to the viewlet, "workbench.view.experimental.allowMovingToNewContainer": true didn't do it.

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.

Looks great, thanks for this!

@@ -289,7 +289,7 @@ export class TerminalTaskSystem implements ITaskSystem {
return false;
}
const activeTerminalInstance = this.terminalService.getActiveInstance();
const isPanelShowingTerminal = this.panelService.getActivePanel()?.getId() === TERMINAL_PANEL_ID;
const isPanelShowingTerminal = this.panelService.getActivePanel()?.getId() === TERMINAL_VIEW_ID;
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to change to isViewShowingTerminal and views service instead?

@@ -964,7 +964,7 @@ export class TerminalInstance extends Disposable implements ITerminalInstance {

private _refreshSelectionContextKey() {
const activePanel = this._panelService.getActivePanel();
const isActive = !!activePanel && activePanel.getId() === TERMINAL_PANEL_ID;
const isActive = !!activePanel && activePanel.getId() === TERMINAL_VIEW_ID;
Copy link
Member

Choose a reason for hiding this comment

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

Again should this not use views service?

Registry.as<IViewsRegistry>(ViewContainerExtensions.ViewsRegistry).registerViews([{
id: TERMINAL_VIEW_ID,
name: nls.localize('terminal', "Terminal"),
canToggleVisibility: false,
Copy link
Member

Choose a reason for hiding this comment

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

No idea what this is meant to drive, I can still right click to toggle the visibility in the panel?

Copy link
Member

Choose a reason for hiding this comment

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

This shall be set to true unless we do not want terminal to hidden.

Comment on lines +667 to 672
const pane = this._viewsService.getActiveViewWithId(TERMINAL_VIEW_ID) as TerminalViewPane;
if (pane) {
pane.hideFindWidget();
this._findWidgetVisible.reset();
panel.focus();
pane.focus();
}
Copy link
Member

Choose a reason for hiding this comment

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

This and findNext/findPrevious no longer verify if the terminal is active or not, I guess that's fine though.

Copy link
Member Author

Choose a reason for hiding this comment

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

getActiveViewWithId only returns the requested view if it is active. so if (pane) is sufficient

Registry.as<IViewsRegistry>(ViewContainerExtensions.ViewsRegistry).registerViews([{
id: TERMINAL_VIEW_ID,
name: nls.localize('terminal', "Terminal"),
canToggleVisibility: false,
Copy link
Member

Choose a reason for hiding this comment

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

This shall be set to true unless we do not want terminal to hidden.

@sandy081
Copy link
Member

I don't know how to move it to the viewlet, "workbench.view.experimental.allowMovingToNewContainer": true didn't do it.

You shall enable canMove on the view while registering.

@sbatten sbatten merged commit 542de19 into microsoft:master Feb 10, 2020
@sbatten sbatten deleted the terminalView branch February 10, 2020 17:45
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
terminal Integrated terminal issues workbench-views Workbench view issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants