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

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

Merged
merged 3 commits into from Sep 13, 2017

Conversation

Projects
None yet
6 participants
@forivall
Contributor

forivall commented Sep 8, 2017

Fixes #34047

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

@roblourens

This comment has been minimized.

Show comment
Hide comment
@roblourens

roblourens Sep 8, 2017

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.

Member

roblourens commented Sep 8, 2017

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

This comment has been minimized.

Show comment
Hide comment
@roblourens

roblourens Sep 8, 2017

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.

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

This comment has been minimized.

Show comment
Hide comment
@forivall

forivall Sep 9, 2017

Contributor

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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@forivall

forivall Sep 9, 2017

Contributor

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

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@isidorn

isidorn Sep 12, 2017

Contributor

@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
Contributor

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

This comment has been minimized.

Show comment
Hide comment
@forivall

forivall Sep 12, 2017

Contributor

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

Contributor

forivall commented Sep 12, 2017

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

@Microsoft Microsoft deleted a comment from msftclas Sep 12, 2017

@isidorn

This comment has been minimized.

Show comment
Hide comment
@isidorn

isidorn Sep 13, 2017

Contributor

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

Contributor

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

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment