Skip to content

Actually don't sort folders when they are all root folders#34052

Merged
isidorn merged 3 commits into
microsoft:masterfrom
forivall:fix-multi-root-order
Sep 13, 2017
Merged

Actually don't sort folders when they are all root folders#34052
isidorn merged 3 commits into
microsoft:masterfrom
forivall:fix-multi-root-order

Conversation

@forivall

@forivall forivall commented Sep 8, 2017

Copy link
Copy Markdown
Contributor

Fixes #34047

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

@roblourens

Copy link
Copy Markdown
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

roblourens commented Sep 8, 2017

Copy link
Copy Markdown
Member

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

forivall commented Sep 9, 2017

Copy link
Copy Markdown
Contributor Author

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

forivall commented Sep 9, 2017

Copy link
Copy Markdown
Contributor Author

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

isidorn commented Sep 12, 2017

Copy link
Copy Markdown
Collaborator

@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

forivall commented Sep 12, 2017

Copy link
Copy Markdown
Contributor Author

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

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

isidorn commented Sep 13, 2017

Copy link
Copy Markdown
Collaborator

@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