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

Checks integrated terminal output for more types of relative paths #22602

Merged
merged 3 commits into from
Mar 17, 2017
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,13 @@ const pathSeparatorClause = '\\/';
const excludedPathCharactersClause = '[^\\0\\s!$`&*()\\[\\]+\'":;]'; // '":; are allowed in paths but they are often separators so ignore them
const escapedExcludedPathCharactersClause = '(\\\\s|\\\\!|\\\\$|\\\\`|\\\\&|\\\\*|(|)|\\+)';
/** A regex that matches paths in the form /path, ~/path, ./path, ../path */
const UNIX_LIKE_LOCAL_LINK_REGEX = new RegExp('(' + pathPrefix + '?(' + pathSeparatorClause + '(' + excludedPathCharactersClause + '|' + escapedExcludedPathCharactersClause + ')+)+)');

const winPathPrefix = '([a-zA-Z]:|\\.\\.?|\\~)';
const UNIX_LIKE_LOCAL_LINK_REGEX = new RegExp('((' + pathPrefix + '|(' + excludedPathCharactersClause + '|' + escapedExcludedPathCharactersClause + ')+)?(' + pathSeparatorClause + '(' + excludedPathCharactersClause + '|' + escapedExcludedPathCharactersClause + ')+)+)');
const winDrivePrefix = '[a-zA-Z]:';
const winPathPrefix = '(' + winDrivePrefix + '|\\.\\.?|\\~)';
const winPathSeparatorClause = '(\\\\|\\/)';
const winExcludedPathCharactersClause = '[^\\0<>\\?\\|\\/\\s!$`&*()\\[\\]+\'":;]';
/** A regex that matches paths in the form c:\path, ~\path, .\path */
const WINDOWS_LOCAL_LINK_REGEX = new RegExp('(' + winPathPrefix + '?(' + winPathSeparatorClause + '(' + winExcludedPathCharactersClause + ')+)+)');

const WINDOWS_LOCAL_LINK_REGEX = new RegExp('((' + winPathPrefix + '|(' + winExcludedPathCharactersClause + ')+)?(' + winPathSeparatorClause + '(' + winExcludedPathCharactersClause + ')+)+)');
/** Higher than local link, lower than hypertext */
const CUSTOM_LINK_PRIORITY = -1;
/** Lowest */
Expand Down Expand Up @@ -129,33 +128,42 @@ export class TerminalLinkHandler {
});
}

private _resolvePath(link: string): TPromise<string> {
protected _preprocessPath(link: string): string {
if (this._platform === platform.Platform.Windows) {
// Resolve ~ -> %HOMEDRIVE%\%HOMEPATH%
if (link.charAt(0) === '~') {
if (!process.env.HOMEDRIVE || !process.env.HOMEPATH) {
return TPromise.as(void 0);
return null;
}
link = `${process.env.HOMEDRIVE}\\${process.env.HOMEPATH + link.substring(1)}`;
}
} else {
// Resolve workspace path . / .. -> <path>/. / <path/..
if (link.charAt(0) === '.') {
if (!this._contextService.hasWorkspace) {

//resolve relative paths
if (!link.match('^' + winDrivePrefix)) {
if (!this._contextService.hasWorkspace()) {
// Abort if no workspace is open
Copy link
Member

Choose a reason for hiding this comment

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

Good catch 👍

return TPromise.as(void 0);
return null;
}
link = path.join(this._contextService.getWorkspace().resource.fsPath, link);
}
}
// Resolve workspace path . / .. -> <path>/. / <path/..
if (link.charAt(0) === '.') {
if (!this._contextService.hasWorkspace) {
// Resolve workspace path . | .. | <relative_path> -> <path>/. | <path>/.. | <path>/<relative_path>
else if (link.charAt(0) !== '/' && link.charAt(0) !== '~') {
if (!this._contextService.hasWorkspace()) {
// Abort if no workspace is open
return TPromise.as(void 0);
return null;
}
link = path.join(this._contextService.getWorkspace().resource.fsPath, link);
}
return link;
}

private _resolvePath(link: string): TPromise<string> {
link = this._preprocessPath(link);
Copy link
Member

Choose a reason for hiding this comment

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

Was this factoring out purely for code cleanliness?

Copy link
Author

Choose a reason for hiding this comment

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

Mostly to to test link preprocessing (_preprocessPath() method) in isolation, and not have to worry about it returning a promise in the tests.


if (!link) {
return TPromise.as(void 0);
}

// Open an editor if the path exists
return pfs.fileExists(link).then(isFile => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,42 @@
import * as assert from 'assert';
import { Platform } from 'vs/base/common/platform';
import { TerminalLinkHandler } from 'vs/workbench/parts/terminal/electron-browser/terminalLinkHandler';
import { IWorkspace, WorkspaceContextService } from 'vs/platform/workspace/common/workspace';
import URI from 'vs/base/common/uri';
import * as path from 'path';
import * as sinon from 'sinon';

class TestTerminalLinkHandler extends TerminalLinkHandler {
public get localLinkRegex(): RegExp {
return this._localLinkRegex;
}
public preprocessPath(link: string): string {
return this._preprocessPath(link);
}
}

class TestXterm {
public setHypertextLinkHandler() { }
public setHypertextValidationCallback() { }
}

class TestURI extends URI {
constructor(private _fakePath: string) {
super();
};

get fsPath(): string {
return this._fakePath;
}
}

class TestWorkspace implements IWorkspace {
resource: URI;
constructor(basePath: string) {
this.resource = new TestURI(basePath);
}
}

suite('Workbench - TerminalLinkHandler', () => {
suite('localLinkRegex', () => {
test('Windows', () => {
Expand All @@ -41,6 +65,7 @@ suite('Workbench - TerminalLinkHandler', () => {
testLink('c:/a/long/path');
testLink('c:\\a\\long\\path');
testLink('c:\\mixed/slash\\path');
testLink('a/relative/path');
Copy link
Member

Choose a reason for hiding this comment

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

Will this also linkify just plain words for example "foo"? That way after validation the output of ls should all be linked. If so a test for that would be great.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it should. I'll add a test.

Copy link
Member

Choose a reason for hiding this comment

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

Actually now that I think about it that's probably not a good idea until we know more about the current working directory within the terminal. It could get confusing otherwise. Right now wealways open files relative to VS Code's folder, not the terminal's.

});

test('Linux', () => {
Expand All @@ -56,6 +81,45 @@ suite('Workbench - TerminalLinkHandler', () => {
testLink('./foo');
testLink('../foo');
testLink('/a/long/path');
testLink('a/relative/path');
});
});

suite('preprocessPath', () => {
test('Windows', () => {
const linkHandler = new TestTerminalLinkHandler(null, new TestXterm(), Platform.Windows, null,
new WorkspaceContextService(new TestWorkspace('C:\\base')));

let stub = sinon.stub(path, 'join', function (arg1, arg2) {
return arg1 + '\\' + arg2;
});
assert.equal(linkHandler.preprocessPath('./src/file1'), 'C:\\base\\./src/file1');
assert.equal(linkHandler.preprocessPath('src\\file2'), 'C:\\base\\src\\file2');
assert.equal(linkHandler.preprocessPath('C:\\absolute\\path\\file3'), 'C:\\absolute\\path\\file3');

stub.restore();
});

test('Linux', () => {
const linkHandler = new TestTerminalLinkHandler(null, new TestXterm(), Platform.Linux, null,
new WorkspaceContextService(new TestWorkspace('/base')));

let stub = sinon.stub(path, 'join', function (arg1, arg2) {
return arg1 + '/' + arg2;
});

assert.equal(linkHandler.preprocessPath('./src/file1'), '/base/./src/file1');
assert.equal(linkHandler.preprocessPath('src/file2'), '/base/src/file2');
assert.equal(linkHandler.preprocessPath('/absolute/path/file3'), '/absolute/path/file3');
stub.restore();
});

test('No Workspace', () => {
const linkHandler = new TestTerminalLinkHandler(null, new TestXterm(), Platform.Linux, null, new WorkspaceContextService(null));

assert.equal(linkHandler.preprocessPath('./src/file1'), null);
assert.equal(linkHandler.preprocessPath('src/file2'), null);
assert.equal(linkHandler.preprocessPath('/absolute/path/file3'), '/absolute/path/file3');
});
});
});