Skip to content

Conversation

@forivall
Copy link
Contributor

@forivall forivall commented Sep 8, 2017

Fixes #34047

view the original poc fix at master...forivall:fix-multi-root-order-pocfix

@roblourens
Copy link
Member

I'm not sure the generic Tree control is the right place for this change. It should probably be controlled at the the File Explorer level. I thought we maybe already did that. But I yield to @bpasero on #34047.

@roblourens
Copy link
Member

roblourens commented Sep 8, 2017

I see we do have https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/parts/files/browser/views/explorerViewer.ts#L571 which marks roots as equal, but like you said it's related to V8 having an unstable sort, so I guess that alone won't work.

@forivall forivall force-pushed the fix-multi-root-order branch from 0d51af4 to 8396f97 Compare September 9, 2017 00:21
@forivall
Copy link
Contributor Author

forivall commented Sep 9, 2017

Yup! the original patch was mostly a proof of concept fix; it was my first time diving into the VSCode codebase, and this was easier than making the more correct fix, as you said, at the fileExplorer level.

I've updated it to add a rootIndex property to FileStat and sorting on that if the fileStat is a root.

This makes the FileStat constructor a little ugly though, i'm open to improvements.

@forivall
Copy link
Contributor Author

forivall commented Sep 9, 2017

Alternatively, a class RootStat extends FileStat could be added, like NewStatPlaceholder, and rootIndex could only be there, instead of added to FileStat

@bpasero bpasero modified the milestone: September 2017 Sep 9, 2017
@bpasero bpasero assigned isidorn and unassigned bpasero Sep 11, 2017
@isidorn
Copy link
Collaborator

isidorn commented Sep 12, 2017

@forivall thanks a lot for this PR.
Instead of changing the model. I would prefer if FileSorterwould depend on WorkspaceContextService and then if both stats are roots you would go to the Workspace Context Service and check the index of each root.

You could inject the Workspace Context Service to the Sorter using

@IWorkspaceContextService private contextService: IWorkspaceContextService

@forivall
Copy link
Contributor Author

forivall commented Sep 12, 2017

@isidorn cool! thanks! good ol' DI. (patch updated)

@microsoft microsoft deleted a comment from msftclas Sep 12, 2017
@isidorn
Copy link
Collaborator

isidorn commented Sep 13, 2017

@forivall looks good, thanks again! Merging in, will be available in tomorrow's insider build

@isidorn isidorn merged commit 297c44e into microsoft:master Sep 13, 2017
@isidorn isidorn added the file-explorer Explorer widget issues label Sep 13, 2017
@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

file-explorer Explorer widget issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Order of root folders is inconsistent when there are more than 10 root folders in a multi root workspace

6 participants