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

Allow to DND multiple root-folders #44554

Closed
bpasero opened this issue Feb 27, 2018 · 6 comments · Fixed by #45428
Closed

Allow to DND multiple root-folders #44554

bpasero opened this issue Feb 27, 2018 · 6 comments · Fixed by #45428
Assignees
Labels
debt Code quality issues file-explorer Explorer widget issues release-notes Release notes issues verification-needed Verification of issue is requested verified Verification succeeded
Milestone

Comments

@bpasero
Copy link
Member

bpasero commented Feb 27, 2018

Testing #29715

I see no reason why we would not support to DND multiple root folders. The API is easy enough to use, just splice the folders by removing all and then adding all in the right order. We do the diff and will only send the correct change event, you don't have to compute it.

@isidorn isidorn added debt Code quality issues file-explorer Explorer widget issues labels Feb 27, 2018
@isidorn isidorn added this to the March 2018 milestone Feb 27, 2018
@isidorn isidorn modified the milestones: March 2018, Backlog Mar 9, 2018
@isidorn
Copy link
Contributor

isidorn commented Mar 9, 2018

I justr tried writing this and it is not so trivial to put them in the right order -> will overcomplicate the code -> backlog.
And doing two of them seperatly produces an error -> seems like the workspace editing service can not handle two operations in parallel of changing roots.

Code pointer https://github.com/Microsoft/vscode/blob/b40e6492ab415347a86c3841c0437dba996db6ce/src/vs/workbench/parts/files/electron-browser/views/explorerViewer.ts#L961

@bpasero
Copy link
Member Author

bpasero commented Mar 9, 2018

@isidorn yeah I would not expect that 2 calls to the workspace editing service is clever, but if you call it once with the right order after the drop operation it should work, no? Can you not keep an array of workspace folders when the drag operation starts and when you drop you simply call the splice method on that array to reflect the change and then just give that array to the workspace editing service to replace them all?

@isidorn
Copy link
Contributor

isidorn commented Mar 9, 2018

@bpasero in theory yes, in practice it is very tedious code. "Simply call the splice method" is not so trivial, I tried.

@bpasero
Copy link
Member Author

bpasero commented Mar 9, 2018

@isidorn ok lets look into it next week when we are both in the office

@isidorn
Copy link
Contributor

isidorn commented Mar 9, 2018

@bpasero I see you are very passionate about this. I can do a PR and then you will see how the code is clunky. And potentially improve it

@bpasero
Copy link
Member Author

bpasero commented Mar 9, 2018

@isidorn yeah, I am a little passionate about this feature, but more passionate about why the code is complex :). I am happy to check out a PR (even if prototypal) so understand the complexity.

@isidorn isidorn modified the milestones: Backlog, March 2018 Mar 13, 2018
@isidorn isidorn added the verification-needed Verification of issue is requested label Mar 13, 2018
@isidorn isidorn added the release-notes Release notes issues label Mar 26, 2018
@bpasero bpasero added the verified Verification succeeded label Mar 27, 2018
@vscodebot vscodebot bot locked and limited conversation to collaborators Apr 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debt Code quality issues file-explorer Explorer widget issues release-notes Release notes issues verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants