show symbolic links as decorations in explorer #43815

isidorn · 2018-02-16T10:15:48Z

I have adopted your idea in the fileService to first do an lstat and if only if it is a symbolic link to do an additional stat call.

As for visualisation to not do any icon crazyness I have decided to use decorations similar to what we do for errors and git decorations.
I went through all the ansi arrows and I personally like this one the most ⇲
Here's a list of unicode arrows https://unicode-table.com/en/sets/arrows-symbols/
After we get feedback from users we can decide to do some additional decorating like italic, but not for now imho and also the decoration api does not support italic

fyi @chryw
A nice icon overlay you provided looks great, we can look into adding that to the icon as step 2, after we merge this is and get some feedback.

bpasero

@isidorn some feedback provided

bpasero · 2018-02-19T09:16:01Z

src/vs/platform/files/common/files.ts

@@ -410,6 +410,11 @@ export interface IFileStat extends IBaseStat {
 	 */
 	isDirectory: boolean;

+	/**
+	 * The resource is a simbolic link.


simbolic => symbolic

bpasero · 2018-02-19T09:19:08Z

src/vs/workbench/services/files/test/node/resolver.test.ts

@@ -19,7 +19,7 @@ function create(relativePath: string): StatResolver {
 	let absolutePath = relativePath ? path.join(basePath, relativePath) : basePath;
 	let fsStat = fs.statSync(absolutePath);

-	return new StatResolver(uri.file(absolutePath), fsStat.isDirectory(), fsStat.mtime.getTime(), fsStat.size, void 0);
+	return new StatResolver(uri.file(absolutePath), fsStat.isSymbolicLink(), fsStat.isDirectory(), fsStat.mtime.getTime(), fsStat.size, void 0);


@isidorn can we have a test for this too? it looks like node.js allows to create symlinks (fs.symlink).

bpasero · 2018-02-19T09:20:08Z

src/vs/workbench/parts/files/electron-browser/views/explorerDecorationsProvider.ts

 		if (fileStat && fileStat.nonexistentRoot) {
 			return {
 				tooltip: localize('canNotResolve', "Can not resolve workspace folder"),
 				letter: '!',
 				color: listInvalidItemForeground,
 			};
 		}
+		if (fileStat && fileStat.isSymbolicLink) {
+			return {
+				tooltip: localize('symbolicLlink', "Symbolic link"),


Maybe better capital casing: Symbolic Link

bpasero · 2018-02-19T09:25:35Z

src/vs/workbench/parts/files/electron-browser/views/explorerDecorationsProvider.ts

+		if (fileStat && fileStat.isSymbolicLink) {
+			return {
+				tooltip: localize('symbolicLlink', "Symbolic link"),
+				letter: '\u21f2'


Maybe there is a better symbol? Something that looks more like this:

bpasero · 2018-02-19T09:26:42Z

src/vs/workbench/services/files/node/fileService.ts

@@ -1254,6 +1259,7 @@ export class StatResolver {
 						const childStat: IFileStat = {
 							resource: fileResource,
 							isDirectory: fileStat.isDirectory(),
+							isSymbolicLink: isSymbolicLink,


Can be simplified to: isSymbolicLink

bpasero · 2018-02-19T09:29:09Z

src/vs/workbench/services/files/node/fileService.ts

@@ -913,7 +913,7 @@ export class FileService implements IFileService {
 		const absolutePath = this.toAbsolutePath(resource);

 		return pfs.stat(absolutePath).then(stat => {
-			return new StatResolver(resource, stat.isDirectory(), stat.mtime.getTime(), stat.size, this.options.verboseLogging ? this.options.errorLogger : void 0);
+			return new StatResolver(resource, false, stat.isDirectory(), stat.mtime.getTime(), stat.size, this.options.verboseLogging ? this.options.errorLogger : void 0);


This is a bit weird, why not introduce a new method pfs.statLink that is implemented like this:

executes lstat

executes stat only if the stat is a symbolic link

returns something like { stat: stat, isSymbolicLink: boolean }

This method could then also be used from within the resolver

bpasero · 2018-02-19T09:30:08Z

src/vs/workbench/services/files/node/fileService.ts

@@ -1235,7 +1233,14 @@ export class StatResolver {
 					},

 					function stat(this: any): void {
-						fs.stat(fileResource.fsPath, this);
+						fs.lstat(fileResource.fsPath, (error, stat) => {


Use the new method from extfs/pfs for this.

isidorn · 2018-02-20T11:27:23Z

@bpasero I have addressed all your comments. Please review when you get a chance
I have also found a sexy unicode character
Have to check it on linux and win though.

bpasero

@isidorn feel free to commit after checking out my changes

bpasero · 2018-02-20T17:36:13Z

src/vs/workbench/services/files/node/fileService.ts

@@ -912,8 +912,8 @@ export class FileService implements IFileService {
 	private toStatResolver(resource: uri): TPromise<StatResolver> {
 		const absolutePath = this.toAbsolutePath(resource);

-		return pfs.stat(absolutePath).then(stat => {
-			return new StatResolver(resource, stat.isDirectory(), stat.mtime.getTime(), stat.size, this.options.verboseLogging ? this.options.errorLogger : void 0);
+		return pfs.statLink(absolutePath).then(statAndIsLink => {


Can be written nicer:

return pfs.statLink(absolutePath).then(({ isSymbolicLink, stat }) => { return new StatResolver(resource, isSymbolicLink, stat.isDirectory(), stat.mtime.getTime(), stat.size, this.options.verboseLogging ? this.options.errorLogger : void 0); });

bpasero · 2018-02-20T17:37:31Z

src/vs/workbench/services/files/node/fileService.ts

@@ -1235,7 +1233,10 @@ export class StatResolver {
 					},

 					function stat(this: any): void {
-						fs.stat(fileResource.fsPath, this);
+						extfs.statLink(fileResource.fsPath, (error: Error, statAndIsLink) => {


Can be nicer:

extfs.statLink(fileResource.fsPath, (error: Error, { stat, isSymbolicLink }) => { isSymbolicLink = isSymbolicLink; this(null, stat); });

Did not do this due to name clash of the isSymoblicLink variable a couple of lines up. Decided to leave like it is.

bpasero · 2018-02-20T17:38:51Z

src/vs/base/node/pfs.ts

@@ -54,6 +54,16 @@ export function stat(path: string): TPromise<fs.Stats> {
 	return nfcall(fs.stat, path);
 }

+export function statLink(path: string): TPromise<{ stat: fs.Stats, isSymbolicLink: boolean }> {


@isidorn why is this method not simply using extfs? E.g. it should be as simple as:

export function statLink(path: string): TPromise<string> { return nfcall(extfs.statLink, path); }

@bpasero you are correct. I was not aware I can depend on extfs in pfs, but now I see it is already done.
Fixed

show symbolic links as decorations in explorer

40b7a64

isidorn added this to the February 2018 milestone Feb 16, 2018

isidorn assigned bpasero Feb 16, 2018

bpasero self-requested a review February 19, 2018 09:15

bpasero assigned isidorn and unassigned bpasero Feb 19, 2018

bpasero requested changes Feb 19, 2018

View reviewed changes

isidorn added 4 commits February 20, 2018 10:51

Merge branch 'master' into isidorn/symbolicLinks

e3a88e9

pfs, extfs: add statLink

48daad5

stat link unit tests

92e7f72

use pfs.statLink

6ec78d6

bpasero requested changes Feb 20, 2018

View reviewed changes

polish

5e23401

isidorn merged commit 302fd56 into master Feb 21, 2018

isidorn deleted the isidorn/symbolicLinks branch February 21, 2018 09:15

PKief mentioned this pull request Feb 24, 2018

A way to identify symlinks (folders, files) PKief/vscode-material-icon-theme#30

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

show symbolic links as decorations in explorer #43815

show symbolic links as decorations in explorer #43815

isidorn commented Feb 16, 2018 •

edited

bpasero left a comment

bpasero Feb 19, 2018

bpasero Feb 19, 2018

bpasero Feb 19, 2018

bpasero Feb 19, 2018

bpasero Feb 19, 2018

bpasero Feb 19, 2018

bpasero Feb 19, 2018

isidorn commented Feb 20, 2018

bpasero left a comment

bpasero Feb 20, 2018

isidorn Feb 21, 2018

bpasero Feb 20, 2018

isidorn Feb 21, 2018

bpasero Feb 20, 2018 •

edited

isidorn Feb 21, 2018 •

edited

show symbolic links as decorations in explorer #43815

show symbolic links as decorations in explorer #43815

Conversation

isidorn commented Feb 16, 2018 • edited

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

Choose a reason for hiding this comment

isidorn commented Feb 20, 2018

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 Feb 20, 2018 • edited

Choose a reason for hiding this comment

isidorn Feb 21, 2018 • edited

Choose a reason for hiding this comment

isidorn commented Feb 16, 2018 •

edited

bpasero Feb 20, 2018 •

edited

isidorn Feb 21, 2018 •

edited