KEMBAR78
Port view container logic from activitybarpart to panelpart by sbatten · Pull Request #87444 · microsoft/vscode · GitHub
Skip to content

Conversation

@sbatten
Copy link
Member

@sbatten sbatten commented Dec 19, 2019

Refs #85164

I can drop the outline contrib changes, they just make it easy to test.

@sbatten sbatten requested a review from sandy081 December 19, 2019 22:34
@sbatten sbatten self-assigned this Dec 19, 2019
@sbatten sbatten added this to the December/January 2020 milestone Dec 19, 2019
Copy link
Member

@sandy081 sandy081 left a comment

Choose a reason for hiding this comment

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

Approved given the provided comments are taken care of


private getViewContainer(panelId: string): ViewContainer | undefined {
const viewContainerRegistry = Registry.as<IViewContainersRegistry>(ViewContainerExtensions.ViewContainersRegistry);
return viewContainerRegistry.get(panelId);
Copy link
Member

Choose a reason for hiding this comment

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

Shall we have to look for the view container only in the Panel location? (even though ids are unique).

this.activePanelContextKey.set(panel.getId());

// This panel supports view containers
if (panel instanceof PaneCompositePanel) {
Copy link
Member

Choose a reason for hiding this comment

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

I would check if view container exists/registered instead.

Copy link
Member

Choose a reason for hiding this comment

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

I just checked activityBarPart and I think we do not need to have this check and we can do it for all panels.

this.compositeBar.activateComposite(panel.getId());

// This panel supports view containers
if (panel instanceof PaneCompositePanel) {
Copy link
Member

Choose a reason for hiding this comment

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

Same as above. You can remove this check because you check for view container after


return cachedPanel?.views && cachedPanel.views.length
? cachedPanel.views.every(({ when }) => !!when && !this.contextKeyService.contextMatchesRules(ContextKeyExpr.deserialize(when)))
: panelId === TEST_VIEW_CONTAINER_ID /* Hide Test Panel for the first time or it had no views registered before */;
Copy link
Member

Choose a reason for hiding this comment

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

Checking with TEST_VIEW_CONTAINER_ID is specific to Sidebar, so not sure if you need this here

Copy link
Member Author

Choose a reason for hiding this comment

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

originally left this in if we didn't enforce unique ids across location and we chose to re-use the test container, removing.

@sbatten sbatten merged commit 57c30dd into microsoft:master Dec 20, 2019
@sbatten sbatten deleted the step2 branch December 20, 2019 16:22
@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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants