-
Notifications
You must be signed in to change notification settings - Fork 27.9k
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 */ | ||
|
@@ -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) === '.') { | ||
|
||
//resolve relative paths | ||
if (!link.match('^' + winDrivePrefix)) { | ||
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); | ||
} | ||
} | ||
// Resolve workspace path . / .. -> <path>/. / <path/.. | ||
if (link.charAt(0) === '.') { | ||
// 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was this factoring out purely for code cleanliness? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 => { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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', () => { | ||
|
@@ -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'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will this also linkify just plain words for example There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it should. I'll add a test. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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', () => { | ||
|
@@ -56,6 +81,37 @@ 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(); | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch 👍