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

Explorer multiple selection #41461

Merged
merged 16 commits into from
Jan 11, 2018
Merged

Explorer multiple selection #41461

merged 16 commits into from
Jan 11, 2018

Conversation

isidorn
Copy link
Contributor

@isidorn isidorn commented Jan 11, 2018

fixes #1023

After discussing with @bpasero we came to the following conclusions regarding extension commands in the explorer:

Pass the selected resources as an additional array

We would continue to pass the focused resource as the first argument, however if there is a multi selection we would pass those resources as an additional argument. We would only pass them if the focused element is part of the selection since that means the user triggered the action on the selection.
resource, [selected_resources]
This also means that the first argument is containted in the array,

Option 1: show all extension commands for multiple selection

Commands from extensions would always still show up even if multi select is on. And we would still continue passing the same argument so nothing would break. Only the users might find surprising that extension commands only work on the actual focused element, not on all. But extension can adopt to the new arguments over time.

Option 2: no extension command shows up for multi-select unless they opt-in

Basically for each extension command we actually add to the when condition: when-single-selection and we provide a mechanism for commands to opt into this.
If we decide to go down this path do we still show those command but disabled or not show them at all? For me it makes most sense to show them disabled.

@jrieken looking forward to your feedback

Edit: After discussion in the standup we have decided to go with Option 1.

@isidorn isidorn added this to the January 2018 milestone Jan 11, 2018
@isidorn isidorn mentioned this pull request Jan 11, 2018
originalEvent.dataTransfer.setData(DataTransfers.DOWNLOAD_URL, [MIME_BINARY, source.name, source.resource.toString()].join(':'));
originalEvent.dataTransfer.setData(DataTransfers.TEXT, getPathLabel(sources[0].resource));
} else {
originalEvent.dataTransfer.setData(DataTransfers.URLS, JSON.stringify(sources.map(s => s.resource.toString())));
Copy link
Member

Choose a reason for hiding this comment

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

@isidorn make sure to only include stats that are files, not folders

@isidorn isidorn merged commit 88f6de2 into master Jan 11, 2018
@isidorn isidorn deleted the isidorn/explorerSelection branch January 11, 2018 15:42
@isidorn isidorn mentioned this pull request Jan 11, 2018
3 tasks
@jrieken
Copy link
Member

jrieken commented Jan 11, 2018

Yeah, I think option 1 is the winner here. I like.

@eamodio
Copy link
Contributor

eamodio commented Jan 11, 2018

Is this only for the file explorer? Or also for custom views?

I'm happy that at least the extra selection is in an additional array, but why not always pass the array, so that extensions could just switch over to using the array directly, without having to dance with a variable number of arguments. This also doesn't match the structure of the SCM resource multi-select events (which on one hand is a good thing because it is a known set of variable args, where SCM is an unknown set). More and more, I would love a more cohesive callback structure to commands -- they are getting harder and harder to deal with. See #25716

Also as more command will be added to more contexts, I REALLY want to see something like: #34048

I don't know about other extensions, but I will certainly need to make GitLens changes to deal with this.

@bpasero
Copy link
Member

bpasero commented Jan 12, 2018

@eamodio I agree that we should have a consistent arguments list for commands executed on a selection in lists and trees and I think the structure that we have now makes sense:

  • the first argument is always the focused item resource (there can always be just 1 focused)
  • the second argument is an array of selected item resources (there can be 1-N)

I think it is important to know which of the selected items is the focused one so that actions can still work that only support a single resource.

What we could think about is even sending around an array of resources if a single item is selected to be consistent.

@eamodio
Copy link
Contributor

eamodio commented Jan 12, 2018

Thinking about it more, it is probably best to leave it as you have it outlined -- focused uri, and optional array of uris if multi-select. That would have the least backward compat ripple -- since there could only be an issue during a multi-select. Thanks for the consideration.

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.

Implement multi-select in left-panel file tree
4 participants