Various kinds of sorting in explorer #29509

foreverest · 2017-06-27T00:25:25Z

mention-bot · 2017-06-27T00:25:28Z

@foreverest, thanks for your PR! By analyzing the history of the files in this pull request, we identified @egamma and @bpasero to be potential reviewers.

msftclas · 2017-06-27T00:25:30Z

@foreverest,
Thanks for your contribution.
To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla.microsoft.com.

It will cover your contributions to all Microsoft-managed open source projects.
Thanks,
Microsoft Pull Request Bot

msftclas · 2017-06-27T00:33:43Z

@foreverest, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, Microsoft Pull Request Bot

bpasero · 2017-06-30T05:26:54Z

@foreverest good start but I think we can do more. There is a lot of people that have asked for sorting the explorer and I think instead of introducing a setting for each we should have one setting explorer.sortOrder with an enum of possible values:

modified (for Add option to sort files in Explorer side-bar by date modified #11823)
type (for Option for sorting files by type in Explorer #5222)
filesFirst (for Give an option to show files before directories in file explorer #22529)
mixed (for your Make listing directories first an option #29329)
default (this is the current sorting)

What do you think? They should all be straight forward, except for maybe the modified one that probably needs a file change listener to resort based on file access.

foreverest · 2017-06-30T06:53:52Z

@bpasero sounds great! Let me do this.

foreverest · 2017-07-01T13:27:50Z

@bpasero I believe we should enhance sorting config by expanding in three dimensions. To cover most needs we can have something like this:

sortOrder: asc, desc
sortBy: name, type, modified, (size?)
fileDirOrder: dirsFirst, filesFirst, mixed

What do you think?

bpasero · 2017-07-01T13:36:56Z

@foreverest what is the motivation for allowing to change sorting from ascending/descending? I have not heard a feature request for that so far. I think that rather belongs to a pure file explorer than an editor. Arguably, even sorting by type and modified date is already stretch for an editor.

foreverest · 2017-07-01T13:40:23Z

@bpasero as of now, the ascending order is used in all sortings. But for modified sorting it might be more useful to have descending option. So that the most recently changed files appeared at the top.

bpasero · 2017-07-01T14:29:38Z

@foreverest ah yeah, I was assuming that the order for "modified" actually is descending by default without an extra option to change that. overall I was hoping we could get this into a single option and not start to introduce many.

foreverest · 2017-07-01T21:14:31Z

@bpasero I'm done with this

bpasero

Very good start, some comments to make this a little bit better 👍

bpasero · 2017-07-02T05:55:00Z

src/vs/workbench/parts/files/common/files.ts

@@ -60,6 +60,7 @@ export interface IFilesConfiguration extends IFilesConfiguration, IWorkbenchEdit
 		};
 		autoReveal: boolean;
 		enableDragAndDrop: boolean;
+		sortOrder: string;


This should be sortOrder: 'default' | 'mixed' | 'filesFirst' | 'type' | 'modified'

This type will appear in three places:

IFilesConfiguration interface

ExplorerView class

The sorter class
Should I define a type alias like this type SortOrder = 'default' | 'mixed' | 'filesFirst' | 'type' | 'modified'; ?

Yes, that makes sense to me.

bpasero · 2017-07-02T05:56:12Z

src/vs/platform/explorer/common/explorer.ts

@@ -0,0 +1,13 @@
+/*---------------------------------------------------------------------------------------------


I suggest to move this out of this new file and into src/vs/workbench/parts/files/common/files.ts

bpasero · 2017-07-02T06:00:22Z

src/vs/workbench/parts/files/browser/views/explorerViewer.ts

-// Explorer Sorter
-export class FileSorter implements ISorter {
+// Default Sorter
+export class DefaultSorter implements ISorter {


There is lots of duplication across all sorters, specifically this code:

if (statA instanceof NewStatPlaceholder) { return -1; } if (statB instanceof NewStatPlaceholder) { return 1; } // Do not sort roots if (statA.isRoot) { return -1; } if (statB.isRoot) { return 1; }

I suggest to reduce the number of filters to just one which listens to configuration changes and updates itself based on the config. That avoids the extra call to setSorter() in the tree and makes the sorter self contained based on the configuration.

bpasero · 2017-07-02T06:01:33Z

src/vs/workbench/parts/files/browser/views/explorerView.ts

@@ -82,6 +83,8 @@ export class ExplorerView extends CollapsibleView {

 	private autoReveal: boolean;

+	private sortOrder: string;


Make this sortOrder: 'default' | 'mixed' | 'filesFirst' | 'type' | 'modified'

bpasero · 2017-07-02T06:03:01Z

src/vs/workbench/parts/files/browser/views/explorerView.ts

+		if (this.sortOrder !== configSortOrder) {
+			this.sortOrder = configSortOrder;
+			const sorter = this.getSorter(this.sortOrder);
+			(<Tree>this.explorerViewer).setSorter(sorter);


Instead of setting a different sorter after the configuration changed, I suggest to actually let the sorter implement the listening and update itself based on the config. You still need this code to decide to do the refresh, which is fine.

You still need this code to decide to do the refresh

Do you mean to leave this code here, or move it to the sorter class?

I mean, move it to the sorter class. But you still need that code to trigger a refresh of the explorer when the sorting config changed and imho it is fine that the explorerView triggers this and not the sorter.

bpasero · 2017-07-02T06:04:40Z

src/vs/base/parts/tree/browser/treeImpl.ts

@@ -364,6 +364,10 @@ export class Tree extends Events.EventEmitter implements _.ITree {
 		return this.model.hasTrait(trait, element);
 	}

+	public setSorter(sorter: _.ISorter): void {


This would not be needed once the sorter is configuring itself.

bpasero · 2017-07-02T06:05:09Z

src/vs/workbench/parts/files/browser/views/explorerView.ts

@@ -430,6 +443,21 @@ export class ExplorerView extends CollapsibleView {
 		return this.explorerViewer;
 	}

+	private getSorter(sortOrder: string): ISorter {


Not needed once the sorter configures itself based on the config.

bpasero · 2017-07-02T06:06:42Z

src/vs/workbench/parts/files/browser/views/explorerView.ts

+		if (e.gotUpdated()) {
+			// Check updated only if sortOrder === MODIFIED
+			const config = this.configurationService.getConfiguration<IFilesConfiguration>();
+			if (config && config.explorer && config.explorer.sortOrder === SortOrderConfiguration.MODIFIED) {


Why do you need to resolve the config here again? You should be able to just use this.sortOrder?

Yes, you're right! 👍

bpasero · 2017-07-02T06:09:52Z

src/vs/workbench/parts/files/browser/views/explorerViewer.ts

+		}
+
+		if (statA.mtime === statB.mtime) {
+			return 0;


Wouldn't it make sense here to fallback to the default sorting by file name in case the modified time is the same?

foreverest

@bpasero I've made the changes. Please review.

bpasero

Almost, good stuff 👍

bpasero · 2017-07-05T15:28:32Z

src/vs/base/common/comparers.ts

+	}
+
+	if (oneName === otherName) {
+		return 0;


Would it not make sense to use the filename compare fallback here as well?

Yes, and I do use it here. First, I compare extensions on line 104. And if they are equal, I compare filenames on lines 108-112.

bpasero · 2017-07-05T15:29:49Z

src/vs/workbench/parts/files/browser/files.contribution.ts

+				nls.localize('sortOrder.type', 'Files and directories are sorted by their extensions, in alphabetical order. Directories are displayed before files.'),
+				nls.localize('sortOrder.modified', 'Files and directories are sorted by last modified date, in descending order. Directories are displayed before files.')
+			],
+			'description': nls.localize('sortOrder', "Controls the way of sorting files and directories.")


Suggest: Controls the way of sorting files and directories in the explorer.

bpasero · 2017-07-05T15:32:05Z

src/vs/workbench/parts/files/browser/views/explorerViewer.ts

-		if (statA.isDirectory && !statB.isDirectory) {
-			return -1;
-		}
+		switch (this.sortOrder) {


I appreciate that we now can do this all from a single compare implementation, that is very cool. However, I am having some difficulties that we have the switch (this.sortOrder) twice in the method, makes it very hard to understand the flow imho. Can we:

move the things which are independent from the sort order up (NewStatPlaceholder and roots)

have a single switch statement and not two?

I know that having a single switch statement might mean some duplication of sorting logic, but I think it would still be easier to understand. We can still in that model extract logic into methods if we wanted to share.

But the order matters here. If I do this, it will affect the position of the new file/dir placeholder. It will appear at the very top of the current dir, not after all subdirs as currently.

To implement the approach you described and keep current behavior of placeholders, the code may look like this:

public compare(tree: ITree, statA: FileStat, statB: FileStat): number { // Do not sort roots if (statA.isRoot) { return -1; } if (statB.isRoot) { return 1; } switch (this.sortOrder) { case 'default': if (statA.isDirectory && !statB.isDirectory) { return -1; } if (statB.isDirectory && !statA.isDirectory) { return 1; } if (statA instanceof NewStatPlaceholder) { return -1; } if (statB instanceof NewStatPlaceholder) { return 1; } return comparers.compareFileNames(statA.name, statB.name); case 'type': if (statA.isDirectory && !statB.isDirectory) { return -1; } if (statB.isDirectory && !statA.isDirectory) { return 1; } if (statA instanceof NewStatPlaceholder) { return -1; } if (statB instanceof NewStatPlaceholder) { return 1; } return comparers.compareFileExtensions(statA.name, statB.name); case 'modified': if (statA.isDirectory && !statB.isDirectory) { return -1; } if (statB.isDirectory && !statA.isDirectory) { return 1; } if (statA instanceof NewStatPlaceholder) { return -1; } if (statB instanceof NewStatPlaceholder) { return 1; } if (statA.mtime !== statB.mtime) { return statA.mtime < statB.mtime ? 1 : -1; } else { return comparers.compareFileNames(statA.name, statB.name); } case 'filesFirst': if (statA.isDirectory && !statB.isDirectory) { return 1; } if (statB.isDirectory && !statA.isDirectory) { return -1; } if (statA instanceof NewStatPlaceholder) { return -1; } if (statB instanceof NewStatPlaceholder) { return 1; } return comparers.compareFileNames(statA.name, statB.name); case 'mixed': if (statA instanceof NewStatPlaceholder) { return -1; } if (statB instanceof NewStatPlaceholder) { return 1; } return comparers.compareFileNames(statA.name, statB.name); } }

Yeah, it looks more verbose but definitely less confusing. Should I commit this?

bpasero · 2017-07-06T05:35:42Z

@foreverest nah, it's fine, I will merge it, thanks.

wclr · 2017-07-12T17:56:46Z

@bpasero
if set sortOrder to type
dot folders come after ordinary folders:

Which is a bit unconventional. While dot files come first.

bpasero · 2017-07-12T18:31:13Z

@whitecolor good catch, can you file an issue? @foreverest do you want to take a look through a PR?

foreverest · 2017-07-12T19:28:06Z

@whitecolor That's because currently folders are handled in the same way as files. Namely, the algorithm first looks at extensions, then at names. In your case reader and shared folders have no extensions, while other ones have. So according to current algorithm that's correct behavior.
However, I think it makes sense not to apply the same rules to files and folders, since the term type is not fully applicable to folders.
@bpasero Should I change the algorithm so that it sorts folders in normal alphabetical order?

P.S.
@whitecolor

While dot files come first.

That's not correct.

gulshan · 2017-07-12T23:24:35Z

IMHO, extensions within folder name should not matter. They should be ordered by name alphabatecally.

bpasero · 2017-07-13T04:34:06Z

@foreverest yes I think folders with a leading "." should show before folders that do not have any extension. Thanks 👍

wclr · 2017-07-13T04:41:20Z

IMHO, extensions within folder name should not matter. They should be ordered by name alphabatecally.

I believe sometimes it may be useful to have folders sorted by extension too.
But there may be introduced an option for example folderSortOrder.

sylvain-pauly · 2017-07-13T07:18:27Z

Well, another sort type would be great : the default one, except it would be case sensitive. Every folders starting with a capital letter first. Github follow this rule if I'm not mistaken :) I would love to have this in VScode (which is awesome BTW).

Make listing directories first an option microsoft#29329

a3085ff

msftclas added the cla-required label Jun 27, 2017

msftclas added cla-signed and removed cla-required labels Jun 27, 2017

rebornix assigned bpasero and sandy081 Jun 30, 2017

bpasero added this to the On Deck milestone Jun 30, 2017

bpasero unassigned sandy081 Jun 30, 2017

Added refresh on dirsFirst config value changed

410f89d

foreverest force-pushed the dirsfirst branch from 774a0d6 to 410f89d Compare June 30, 2017 18:11

foreverest added 7 commits June 30, 2017 21:44

Switch sorting configuration to enum

40907c3

Added filesFirst sorting

4f68100

Added sorting by type

4835047

Got rid of the path dependency

b909230

Added sorting by last modified date

794fcb0

Merge branch 'master' into dirsfirst

968483b

Merge conflict fix

81c8bfa

foreverest added 2 commits July 1, 2017 19:08

modified sorting order changed to descending

efb17ac

New compareFileExtensions comparer added for TypeSorter

12ed0dd

bpasero requested changes Jul 2, 2017

View reviewed changes

bpasero modified the milestones: On Deck, July 2017 Jul 3, 2017

Switched to a single self-contained explorer sorter

66f978b

foreverest changed the title ~~Make listing directories first an option~~ Various kinds of sorting in explorer Jul 3, 2017

foreverest commented Jul 5, 2017

View reviewed changes

bpasero requested changes Jul 5, 2017

View reviewed changes

Improved description of 'explorer.sortOrder' config option

171ae31

bpasero merged commit 52004b4 into microsoft:master Jul 6, 2017

foreverest deleted the dirsfirst branch July 6, 2017 05:44

foreverest mentioned this pull request Jul 13, 2017

Sort by type doesn't affect folders #30572

Merged

github-actions bot locked and limited conversation to collaborators Mar 27, 2020

		@@ -0,0 +1,13 @@
		/*---------------------------------------------------------------------------------------------

		@@ -82,6 +83,8 @@ export class ExplorerView extends CollapsibleView {

		private autoReveal: boolean;

		private sortOrder: string;

Various kinds of sorting in explorer #29509

Various kinds of sorting in explorer #29509

Conversation

foreverest commented Jun 27, 2017 • edited Loading

mention-bot commented Jun 27, 2017

msftclas commented Jun 27, 2017

msftclas commented Jun 27, 2017

bpasero commented Jun 30, 2017 • edited Loading

foreverest commented Jun 30, 2017

foreverest commented Jul 1, 2017 • edited Loading

bpasero commented Jul 1, 2017 • edited Loading

foreverest commented Jul 1, 2017 • edited Loading

bpasero commented Jul 1, 2017

foreverest commented Jul 1, 2017

bpasero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpasero Jul 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

foreverest left a comment

Choose a reason for hiding this comment

bpasero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpasero commented Jul 6, 2017

wclr commented Jul 12, 2017

bpasero commented Jul 12, 2017

foreverest commented Jul 12, 2017 • edited Loading

gulshan commented Jul 12, 2017

bpasero commented Jul 13, 2017

wclr commented Jul 13, 2017 • edited Loading

sylvain-pauly commented Jul 13, 2017

foreverest commented Jun 27, 2017 •

edited

Loading

bpasero commented Jun 30, 2017 •

edited

Loading

foreverest commented Jul 1, 2017 •

edited

Loading

bpasero commented Jul 1, 2017 •

edited

Loading

foreverest commented Jul 1, 2017 •

edited

Loading

bpasero Jul 2, 2017 •

edited

Loading

foreverest commented Jul 12, 2017 •

edited

Loading

wclr commented Jul 13, 2017 •

edited

Loading