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

views: support auxiliary bar in OpenViewContainerAction #170137

Merged
merged 1 commit into from Dec 29, 2022

Conversation

connor4312
Copy link
Member

Fixes #169907

@connor4312 connor4312 enabled auto-merge (squash) December 27, 2022 23:20
@connor4312 connor4312 self-assigned this Dec 27, 2022
@VSCodeTriageBot VSCodeTriageBot added this to the January 2023 milestone Dec 27, 2022
@@ -367,13 +367,16 @@ export class ViewsService extends Disposable implements IViewsService {
const viewsService = serviceAccessor.get(IViewsService);
const viewContainerLocation = viewDescriptorService.getViewContainerLocation(viewContainer);
switch (viewContainerLocation) {
case ViewContainerLocation.Sidebar:
if (!viewsService.isViewContainerVisible(viewContainer.id) || !layoutService.hasFocus(Parts.SIDEBAR_PART)) {
case ViewContainerLocation.AuxiliaryBar:
Copy link
Member

@sbatten sbatten Dec 28, 2022

Choose a reason for hiding this comment

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

Thanks for this change @connor4312 ... Looks like this command got forgotten in the changes with the views. Your fix actually highlights another problem with this command. The title of the command is Show X or Toggle X depending on where the view container lives at startup. The behavior however is based on the location at execution. I'm not really sure if there is a strong reason for having this differentiation so I suggest we stick to the same behavior for all locations: Show X and simplify here. @sandy081 what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

@sbatten This was brought multiple times and got dropped. See the discussion here #7540. I agree the change would be simple and consistent but not might be acceptable by all users who got used to it.

Copy link
Member

Choose a reason for hiding this comment

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

If we don't go for consistency, we need to fix the bug where the command name doesn't reflect the behavior. And we also need to make a decision on whether the Secondary Side Bar acts more like the panel or more like the Primary Side Bar. I would say Side Bar behavior, since that's what its called and that would be my vote for all view "commands".

@connor4312 connor4312 merged commit b0abaca into main Dec 29, 2022
@connor4312 connor4312 deleted the connor4312/issue169907 branch December 29, 2022 11:49
@sandy081
Copy link
Member

Oops, My approval auto merged it and I did not see the auto merge is enabled. Sorry about it.

@sbatten Feel free to suggest any additional comments if you have.

@github-actions github-actions bot locked and limited conversation to collaborators Feb 12, 2023
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.

Testing menu item broken, missing from activity bar
4 participants