#82352 Implement sorting for search results

[jzyrobert]

This PR fixes #82352

Added a setting to allow sorting of search results.
Demo:

The following are the possible options:

• default: As before, sorts by folder path first and then by file name. Uses compareFileNames to correct the ordering of numeral files (e.g. file1 is now in front of file10)
• filesOnly: Ignore the path name and sort only by file name
• type: Sort by extension name (Taken from Explorer sort)
• modified: Sort by last modified date (Taken from Explorer sort)
• countDescending: Sort by most results per file
• countAscending: Sort by least results per file (Should these fallback to name if results match?)

The modified sorting is implemented as follows:

• Whenever refreshTree is called, we ensure that if the sortOrder === modified all fileMatch items have a existing IFileStatWithMetaData, or it will retrieve it.
• Whenever onFilesChanged is called, modified files are retrieved and have their fileStat updated.
• Whenever sortOrder is set away from modified, remove fileStat information so it is re-calculated when it is set to modified again.

I think this is fair on performance, always tracking all meta-data would be performance intensive.
I'm not sure how I would add tests for the modified/count settings as you would have to set several properties.

[jzyrobert]

Things to look at:

• Did the path comparison break on other OS?
• Add a way to re-sort the tree elements without having to recreate them?

[jzyrobert]

So it turns out that:

• The searchViewlet.test.ts have been wrong all the time, C:\ was being added to each path even though the hard-coded paths already had C:
  
• The tests use hard-coded windows paths which leads to incorrect behaviour on posix
• FileMatch.name() calls getBaseLabel which calls basename from resources.ts. @aeschli committed the fact that only non-file URIs should call this function so I think something went wrong while creating the FileMatch class.
• @bpasero committed a fix for this here, however this was reverted right away.

I have:

• Corrected the tests to use windows/posix paths
• Added a new getFileBaseLabel and fileBaselabel function to bypass this and call paths.basename without forcing it to be posix.
• I assume that a function to get the filename from a file URI already exists somewhere, if someone could point me to it?

[jeanp413]

@jzyrobert The URI class converts \ characters to / for windows paths so getBaseLabel should be fine.

[aeschli]

Please keep using resource.getBaseName. It's designed to work for any schema, also for the file scheme.

[jzyrobert]

I see, thanks guys.
I will see if I can make some more improvements on this.

[roblourens]

This looks awesome, thanks for the PR! I am actually not clear what getBaseName refers to but it sounds like you are on top of it

[roblourens]

You don't need to explictly return a promise in an async function, this line can be removed.

[JacksonKearl]

Again here, you don't need to explicity return a promise in async functions.

[JacksonKearl]

Should this be ||? Otherwise const matches = this.viewModel.searchResult.matches(); below will throw when the search order is modified but the viewModel is undefined

[JacksonKearl]

This will check each file syncronously. You might consider using Promise.all to run these in parallel.

[jzyrobert]

Thanks, I was going to look into that

[JacksonKearl]

Is this used?

[JacksonKearl]

These two can be unified into a single const enum. Following that, the == 'modified'-style checks elsewhere should be changed to == SearchSortOrder.Modified. Also we aren't super consistent about CAPS_CASE vs. PascalCase for const enums in elsewhere in vscode, but I think here PascalCase feels better. Up to you.

[JacksonKearl]

Thanks for working on this! I have left some comments throughout.

[jzyrobert]

@roblourens @JacksonKearl I've made the requested changes.

I went with the object + type initially instead of the enum because the explorer sort uses that, should that be refactored into an enum too?

I also did not see an easy way to retrieve the elements out of the tree for re-ordering, is it an acceptable performance hit to resolve the search results every time when a file match count is updated?

[JacksonKearl]

@jzyrobert We probably don't want to touch Explorer files in this, as it would add another reviewer and complexity just for a minor refactoring.

is it an acceptable performance hit to resolve the search results every time when a file match count is updated?

What do you mean by "resolve"? The search isn't run again, correct? Are you referring to this

	// If updated counts affect our search order, re-sort the view.
			if (this.searchConfig.sortOrder === SearchSortOrder.CountAscending ||
				this.searchConfig.sortOrder === SearchSortOrder.CountDescending) {
				this.tree.setChildren(null, this.createResultIterator(collapseResults));
			}

?

That shouldn't be a particularly expensive operation. I played around with it and even with the max of 10000 results I didn't notice a problem. If it comes up as an issue we can look at it then.

I should have been more clear about the const enum thing, you need to additionally declare the values, or else they default to numbers:

export const enum SearchSortOrder {
	Default = 'default',
	FileNames = 'fileNames',
	Type = 'type',
	Modified = 'modified',
	CountDescending = 'countDescending',
	CountAscending = 'countAscending'
}

[JacksonKearl]

LGTM conditional on the

export const enum SearchSortOrder {
	Default = 'default',
	FileNames = 'fileNames',
	Type = 'type',
	Modified = 'modified',
	CountDescending = 'countDescending',
	CountAscending = 'countAscending'
}

[roblourens]

is it an acceptable performance hit to resolve the search results every time when a file match count is updated?

Or if you are referring to resolving file stats, then I think it should only be done once per search. If I am reading the code correctly, it looks like that is the case now right?

[roblourens]

Looks like it's updating all file stats, when just elements would be preferable

[jzyrobert]

@JacksonKearl

@jzyrobert We probably don't want to touch Explorer files in this, as it would add another reviewer and complexity just for a minor refactoring.

I could make it another PR if this is refactoring that should be done.

That shouldn't be a particularly expensive operation. I played around with it and even with the max of 10000 results I didn't notice a problem. If it comes up as an issue we can look at it then.

Sounds good. I tried it as well with the maximum results and it was very fast, but I have a (and you probably do) quite powerful machine, and I was thinking of users with older CPUs.

I should have been more clear about the const enum thing, you need to additionally declare the values, or else they default to numbers:

export const enum SearchSortOrder {
	Default = 'default',
	FileNames = 'fileNames',
	Type = 'type',
	Modified = 'modified',
	CountDescending = 'countDescending',
	CountAscending = 'countAscending'
}

I see the values are used for the actual option values, should have checked for that.

[roblourens]

I'm commenting on the one that does

		const files = this.searchResult.matches().map(f => f.resolveFileStat(this.fileService));

and the function is already taking elements which, I think, are the elements that need to be updated

[jzyrobert]

I'm commenting on the one that does

		const files = this.searchResult.matches().map(f => f.resolveFileStat(this.fileService));

and the function is already taking elements which, I think, are the elements that need to be updated

Oh I see, sorry about that. Yes you're right, I forgot to correct the mapping.