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

Various kinds of sorting in explorer #29509

Merged
merged 13 commits into from
Jul 6, 2017
57 changes: 56 additions & 1 deletion src/vs/base/common/comparers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ export function compareFileNames(one: string, other: string): number {
return noIntlCompareFileNames(one, other);
}

const FileNameMatch = /^([^.]*)(\.(.*))?$/;
const FileNameMatch = /^(.*?)(\.([^.]*))?$/;

export function noIntlCompareFileNames(one: string, other: string): number {
let oneMatch = FileNameMatch.exec(one.toLowerCase());
Expand All @@ -57,6 +57,61 @@ export function noIntlCompareFileNames(one: string, other: string): number {
return oneExtension < otherExtension ? -1 : 1;
}

export function compareFileExtensions(one: string, other: string): number {
if (intlFileNameCollator) {
const oneMatch = one ? FileNameMatch.exec(one) : [];
const otherMatch = other ? FileNameMatch.exec(other) : [];

const oneName = oneMatch[1] || '';
const oneExtension = oneMatch[3] || '';

const otherName = otherMatch[1] || '';
const otherExtension = otherMatch[3] || '';

let result = intlFileNameCollator.compare(oneExtension, otherExtension);

if (result === 0) {
// Using the numeric option in the collator will
// make compare(`foo1`, `foo01`) === 0. We must disambiguate.
if (intlFileNameCollatorIsNumeric && oneExtension !== otherExtension) {
return oneExtension < otherExtension ? -1 : 1;
}

// Extensions are equal, compare filenames
result = intlFileNameCollator.compare(oneName, otherName);

if (intlFileNameCollatorIsNumeric && result === 0 && oneName !== otherName) {
return oneName < otherName ? -1 : 1;
}
}

return result;
}

return noIntlCompareFileExtensions(one, other);
}

function noIntlCompareFileExtensions(one: string, other: string): number {
const oneMatch = one ? FileNameMatch.exec(one.toLowerCase()) : [];
const otherMatch = other ? FileNameMatch.exec(other.toLowerCase()) : [];

const oneName = oneMatch[1] || '';
const oneExtension = oneMatch[3] || '';

const otherName = otherMatch[1] || '';
const otherExtension = otherMatch[3] || '';

if (oneExtension !== otherExtension) {
return oneExtension < otherExtension ? -1 : 1;
}

if (oneName === otherName) {
return 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

return oneName < otherName ? -1 : 1;
}

export function comparePaths(one: string, other: string): number {
const oneParts = one.split(paths.nativeSep);
const otherParts = other.split(paths.nativeSep);
Expand Down
30 changes: 29 additions & 1 deletion src/vs/base/test/browser/comparers.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

'use strict';

import { compareFileNames, setFileNameComparer } from 'vs/base/common/comparers';
import { compareFileNames, compareFileExtensions, setFileNameComparer } from 'vs/base/common/comparers';
import * as assert from 'assert';

suite('Comparers', () => {
Expand All @@ -27,4 +27,32 @@ suite('Comparers', () => {
assert(compareFileNames('abc1.txt', 'abc2.txt') < 0, 'filenames with numbers should be in numerical order, not alphabetical order');
assert(compareFileNames('abc2.txt', 'abc10.txt') < 0, 'filenames with numbers should be in numerical order even when they are multiple digits long');
});

test('compareFileExtensions', () => {

// Setup Intl
setFileNameComparer(new Intl.Collator(undefined, { numeric: true, sensitivity: 'base' }));

assert(compareFileExtensions(null, null) === 0, 'null should be equal');
assert(compareFileExtensions(null, '.abc') < 0, 'null should come before real files');
assert(compareFileExtensions(null, 'abc') < 0, 'null should come before real files without extension');
assert(compareFileExtensions('', '') === 0, 'empty should be equal');
assert(compareFileExtensions('abc', 'abc') === 0, 'equal names should be equal');
assert(compareFileExtensions('.abc', '.abc') === 0, 'equal full names should be equal');
assert(compareFileExtensions('file.ext', 'file.ext') === 0, 'equal full names should be equal');
assert(compareFileExtensions('a.ext', 'b.ext') < 0, 'if equal extensions, filenames should be compared');
assert(compareFileExtensions('.ext', 'a.ext') < 0, 'if equal extensions, filenames should be compared, empty filename should come before others');
assert(compareFileExtensions('file.aaa', 'file.bbb') < 0, 'files should be compared by extensions');
assert(compareFileExtensions('bbb.aaa', 'aaa.bbb') < 0, 'files should be compared by extensions even if filenames compare differently');
assert(compareFileExtensions('1', '1') === 0, 'numerically equal full names should be equal');
assert(compareFileExtensions('abc1.txt', 'abc1.txt') === 0, 'equal filenames with numbers should be equal');
assert(compareFileExtensions('abc1.txt', 'abc2.txt') < 0, 'filenames with numbers should be in numerical order, not alphabetical order');
assert(compareFileExtensions('abc2.txt', 'abc10.txt') < 0, 'filenames with numbers should be in numerical order even when they are multiple digits long');
assert(compareFileExtensions('txt.abc1', 'txt.abc1') === 0, 'equal extensions with numbers should be equal');
assert(compareFileExtensions('txt.abc1', 'txt.abc2') < 0, 'extensions with numbers should be in numerical order, not alphabetical order');
assert(compareFileExtensions('txt.abc2', 'txt.abc10') < 0, 'extensions with numbers should be in numerical order even when they are multiple digits long');
assert(compareFileExtensions('a.ext1', 'b.ext1') < 0, 'if equal extensions with numbers, filenames should be compared');
assert(compareFileExtensions('file2.ext2', 'file1.ext10') < 0, 'extensions with numbers should be in numerical order, not alphabetical order');
assert(compareFileExtensions('file.ext01', 'file.ext1') < 0, 'extensions with equal numbers should be in alphabetical order');
});
});
16 changes: 15 additions & 1 deletion src/vs/workbench/parts/files/browser/files.contribution.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import { IWorkbenchContributionsRegistry, Extensions as WorkbenchExtensions } fr
import { IEditorRegistry, Extensions as EditorExtensions, IEditorInputFactory, EditorInput, IFileEditorInput } from 'vs/workbench/common/editor';
import { AutoSaveConfiguration, HotExitConfiguration, SUPPORTED_ENCODINGS } from 'vs/platform/files/common/files';
import { EditorDescriptor } from 'vs/workbench/browser/parts/editor/baseEditor';
import { FILE_EDITOR_INPUT_ID, VIEWLET_ID } from 'vs/workbench/parts/files/common/files';
import { FILE_EDITOR_INPUT_ID, VIEWLET_ID, SortOrderConfiguration } from 'vs/workbench/parts/files/common/files';
import { FileEditorTracker } from 'vs/workbench/parts/files/common/editors/fileEditorTracker';
import { SaveErrorHandler } from 'vs/workbench/parts/files/browser/saveErrorHandler';
import { FileEditorInput } from 'vs/workbench/parts/files/common/editors/fileEditorInput';
Expand All @@ -32,6 +32,7 @@ import * as platform from 'vs/base/common/platform';
import { IWorkspaceConfigurationService } from 'vs/workbench/services/configuration/common/configuration';
import { DirtyFilesTracker } from 'vs/workbench/parts/files/common/dirtyFilesTracker';


// Viewlet Action
export class OpenExplorerViewletAction extends ToggleViewletAction {
public static ID = VIEWLET_ID;
Expand Down Expand Up @@ -321,6 +322,19 @@ configurationRegistry.registerConfiguration({
'type': 'boolean',
'description': nls.localize('enableDragAndDrop', "Controls if the explorer should allow to move files and folders via drag and drop."),
'default': true
},
'explorer.sortOrder': {
'type': 'string',
'enum': [SortOrderConfiguration.DEFAULT, SortOrderConfiguration.MIXED, SortOrderConfiguration.FILES_FIRST, SortOrderConfiguration.TYPE, SortOrderConfiguration.MODIFIED],
'default': SortOrderConfiguration.DEFAULT,
'enumDescriptions': [
nls.localize('sortOrder.default', 'Files and directories are sorted by their names, in alphabetical order. Directories are displayed before files.'),
nls.localize('sortOrder.mixed', 'Files and directories are sorted by their names, in alphabetical order. Files are interwoven with directories.'),
nls.localize('sortOrder.filesFirst', 'Files and directories are sorted by their names, in alphabetical order. Files are displayed before directories.'),
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.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}
});
48 changes: 35 additions & 13 deletions src/vs/workbench/parts/files/browser/views/explorerView.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import { prepareActions } from 'vs/workbench/browser/actions';
import { memoize } from 'vs/base/common/decorators';
import { ITree } from 'vs/base/parts/tree/browser/tree';
import { Tree } from 'vs/base/parts/tree/browser/treeImpl';
import { IFilesConfiguration, ExplorerFolderContext, FilesExplorerFocussedContext, ExplorerFocussedContext } from 'vs/workbench/parts/files/common/files';
import { IFilesConfiguration, ExplorerFolderContext, FilesExplorerFocussedContext, ExplorerFocussedContext, SortOrderConfiguration, SortOrder } from 'vs/workbench/parts/files/common/files';
import { FileOperation, FileOperationEvent, IResolveFileOptions, FileChangeType, FileChangesEvent, IFileService } from 'vs/platform/files/common/files';
import { RefreshViewExplorerAction, NewFolderAction, NewFileAction } from 'vs/workbench/parts/files/browser/fileActions';
import { FileDragAndDrop, FileFilter, FileSorter, FileController, FileRenderer, FileDataSource, FileViewletState, FileAccessibilityProvider } from 'vs/workbench/parts/files/browser/views/explorerViewer';
Expand Down Expand Up @@ -82,6 +82,8 @@ export class ExplorerView extends CollapsibleView {

private autoReveal: boolean;

private sortOrder: SortOrder;

private settings: any;

constructor(
Expand Down Expand Up @@ -261,6 +263,12 @@ export class ExplorerView extends CollapsibleView {
needsRefresh = this.filter.updateConfiguration();
}

const configSortOrder = configuration && configuration.explorer && configuration.explorer.sortOrder || 'default';
if (this.sortOrder !== configSortOrder) {
this.sortOrder = configSortOrder;
needsRefresh = true;
}

// Refresh viewer as needed
if (refresh && needsRefresh) {
this.doRefresh().done(null, errors.onUnexpectedError);
Expand Down Expand Up @@ -375,7 +383,7 @@ export class ExplorerView extends CollapsibleView {
const dataSource = this.instantiationService.createInstance(FileDataSource);
const renderer = this.instantiationService.createInstance(FileRenderer, this.viewletState);
const controller = this.instantiationService.createInstance(FileController, this.viewletState);
const sorter = new FileSorter();
const sorter = this.instantiationService.createInstance(FileSorter);
this.filter = this.instantiationService.createInstance(FileFilter);
const dnd = this.instantiationService.createInstance(FileDragAndDrop);
const accessibilityProvider = this.instantiationService.createInstance(FileAccessibilityProvider);
Expand Down Expand Up @@ -580,14 +588,12 @@ export class ExplorerView extends CollapsibleView {
// Filter to the ones we care
e = this.filterFileEvents(e);

// We only ever refresh from files/folders that got added or deleted
if (e.gotAdded() || e.gotDeleted()) {
const added = e.getAdded();
const deleted = e.getDeleted();
if (!this.isCreated) {
return false;
}

if (!this.isCreated) {
return false;
}
if (e.gotAdded()) {
const added = e.getAdded();

// Check added: Refresh if added file/folder is not part of resolved root and parent is part of it
const ignoredPaths: { [fsPath: string]: boolean } = <{ [fsPath: string]: boolean }>{};
Expand Down Expand Up @@ -616,6 +622,10 @@ export class ExplorerView extends CollapsibleView {
ignoredPaths[parent] = true;
}
}
}

if (e.gotDeleted()) {
const deleted = e.getDeleted();

// Check deleted: Refresh if deleted file/folder part of resolved root
for (let j = 0; j < deleted.length; j++) {
Expand All @@ -630,15 +640,27 @@ export class ExplorerView extends CollapsibleView {
}
}

if (this.sortOrder === SortOrderConfiguration.MODIFIED && e.gotUpdated()) {
const updated = e.getUpdated();

// Check updated: Refresh if updated file/folder part of resolved root
for (let j = 0; j < updated.length; j++) {
const upd = updated[j];
if (!this.contextService.isInsideWorkspace(upd.resource)) {
continue; // out of workspace file
}

if (this.model.findClosest(upd.resource)) {
return true;
}
}
}

return false;
}

private filterFileEvents(e: FileChangesEvent): FileChangesEvent {
return new FileChangesEvent(e.changes.filter(change => {
if (change.type === FileChangeType.UPDATED) {
return false; // we only want added / removed
}

if (!this.contextService.isInsideWorkspace(change.resource)) {
return false; // exclude changes for resources outside of workspace
}
Expand Down
65 changes: 56 additions & 9 deletions src/vs/workbench/parts/files/browser/views/explorerViewer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import glob = require('vs/base/common/glob');
import { FileLabel, IFileLabelOptions } from 'vs/workbench/browser/labels';
import { IDisposable } from 'vs/base/common/lifecycle';
import { ContributableActionProvider } from 'vs/workbench/browser/actions';
import { IFilesConfiguration } from 'vs/workbench/parts/files/common/files';
import { IFilesConfiguration, SortOrder } from 'vs/workbench/parts/files/common/files';
import { ITextFileService } from 'vs/workbench/services/textfile/common/textfiles';
import { FileOperationError, FileOperationResult, IFileService } from 'vs/platform/files/common/files';
import { ResourceMap } from 'vs/base/common/map';
Expand Down Expand Up @@ -526,20 +526,53 @@ export class FileController extends DefaultController {

// Explorer Sorter
export class FileSorter implements ISorter {
private toDispose: IDisposable[];
private sortOrder: SortOrder;

constructor(
@IConfigurationService private configurationService: IConfigurationService
) {
this.toDispose = [];

this.onConfigurationUpdated(configurationService.getConfiguration<IFilesConfiguration>());

this.registerListeners();
}

private registerListeners(): void {
this.toDispose.push(this.configurationService.onDidUpdateConfiguration(e => this.onConfigurationUpdated(this.configurationService.getConfiguration<IFilesConfiguration>())));
}

private onConfigurationUpdated(configuration: IFilesConfiguration): void {
this.sortOrder = configuration && configuration.explorer && configuration.explorer.sortOrder || 'default';
}

public compare(tree: ITree, statA: FileStat, statB: FileStat): number {
if (statA.isDirectory && !statB.isDirectory) {
return -1;
}
switch (this.sortOrder) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

case 'default':
case 'type':
case 'modified':
if (statA.isDirectory && !statB.isDirectory) {
return -1;
}
if (statB.isDirectory && !statA.isDirectory) {
return 1;
}
break;

if (statB.isDirectory && !statA.isDirectory) {
return 1;
case 'filesFirst':
if (statA.isDirectory && !statB.isDirectory) {
return 1;
}
if (statB.isDirectory && !statA.isDirectory) {
return -1;
}
break;
}

if (statA instanceof NewStatPlaceholder) {
return -1;
}

if (statB instanceof NewStatPlaceholder) {
return 1;
}
Expand All @@ -548,12 +581,26 @@ export class FileSorter implements ISorter {
if (statA.isRoot) {
return -1;
}

if (statB.isRoot) {
return 1;
}

return comparers.compareFileNames(statA.name, statB.name);
switch (this.sortOrder) {
case 'default':
case 'mixed':
case 'filesFirst':
return comparers.compareFileNames(statA.name, statB.name);

case 'type':
return comparers.compareFileExtensions(statA.name, statB.name);

case 'modified':
if (statA.mtime !== statB.mtime) {
return statA.mtime < statB.mtime ? 1 : -1;
} else {
return comparers.compareFileNames(statA.name, statB.name);
}
}
}
}

Expand Down
13 changes: 12 additions & 1 deletion src/vs/workbench/parts/files/common/files.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ export interface IFilesConfiguration extends IFilesConfiguration, IWorkbenchEdit
};
autoReveal: boolean;
enableDragAndDrop: boolean;
sortOrder: SortOrder;
};
editor: IEditorOptions;
}
Expand Down Expand Up @@ -93,4 +94,14 @@ export function explorerItemToFileResource(obj: FileStat | OpenEditor): IFileRes
}

return null;
}
}

export const SortOrderConfiguration = {
DEFAULT: 'default',
MIXED: 'mixed',
FILES_FIRST: 'filesFirst',
TYPE: 'type',
MODIFIED: 'modified'
};

export type SortOrder = 'default' | 'mixed' | 'filesFirst' | 'type' | 'modified';