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

#42557 fix - make open in command multiselect aware #43552

Merged
merged 4 commits into from
Feb 16, 2018

Conversation

sriram-dev
Copy link
Contributor

No description provided.

@msftclas
Copy link

msftclas commented Feb 13, 2018

CLA assistant check
All CLA requirements met.

Copy link

@nzec nzec left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@isidorn
Copy link
Contributor

isidorn commented Feb 13, 2018

@sriram-dev thanks for your PR. This looks good for me.
I would only argue that you should use a Set and not a Map of boolean, since you are using your Map as a Set.
Did you test this out?

@Tyriar do you wish to review as well or you leave this up to me?

@isidorn isidorn added this to the February 2018 milestone Feb 13, 2018
directoryMap.set(directoryToOpen, true);
if (configurationService.getValue<ITerminalConfiguration>().terminal.explorerKind === 'integrated') {
const instance = integratedTerminalService.createInstance({ cwd: directoryToOpen }, true);
if (instance) {
Copy link
Member

Choose a reason for hiding this comment

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

Instead of setting active and showing the panel for every instance, how about we set and show only once for the "primary" selection (I think that's a thing @isidorn?).

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure will check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a primary selection? How do we identify it if so. Currently its being set for every instance on the order in which instances are selected. So we could choose to set active only for the last instance or first instance. Confirm the approach.

Copy link
Contributor

Choose a reason for hiding this comment

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

@sriram-dev primary selection is the resource passed in the command. The getResourcesForCommand gets all the other selected elements (including the primary)

@Tyriar Tyriar self-assigned this Feb 13, 2018
@sriram-dev
Copy link
Contributor Author

@isidorn Yes set makes sense. Will change it.

@sriram-dev
Copy link
Contributor Author

@isidorn Please review and confirm if the changes are good

@isidorn isidorn merged commit aa2ef66 into microsoft:master Feb 16, 2018
@isidorn
Copy link
Contributor

isidorn commented Feb 16, 2018

@sriram-dev thanks for your PR I have merged it in 🍻
I have pushed minor polish on top of it 119d495
@Tyriar feel free to edit it as you wish

@sriram-dev
Copy link
Contributor Author

Yay! Thanks!

@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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants