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.

Sign up for GitHub

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

Jump to bottom

Support terminal links with end row/col #185888

Merged
merged 4 commits into from
Jun 23, 2023
Merged

Support terminal links with end row/col #185888

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
158ae0a
Support parsing link end row and col
Tyriar Jun 22, 2023
9d817eb
Add test for new lines+chars format
Tyriar Jun 22, 2023
b44aede
Support links with end rows and/or cols
Tyriar Jun 22, 2023
f0e8f86
Include end col/line in link openers tests
Tyriar Jun 23, 2023
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
6 changes: 4 additions & 2 deletions src/vs/workbench/contrib/terminalContrib/links/browser/terminalLinkOpeners.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,10 @@ export class TerminalLocalFileLinkOpener implements ITerminalLinkOpener {
}
const linkSuffix = link.parsedLink ? link.parsedLink.suffix : getLinkSuffix(link.text);
const selection: ITextEditorSelection | undefined = linkSuffix?.row === undefined ? undefined : {
startLineNumber: linkSuffix?.row ?? 1,
startColumn: linkSuffix?.col ?? 1
startLineNumber: linkSuffix.row ?? 1,
startColumn: linkSuffix.col ?? 1,
endLineNumber: linkSuffix.rowEnd,
endColumn: linkSuffix.colEnd
};
await this._editorService.openEditor({
resource: link.uri,
Expand Down
73 changes: 40 additions & 33 deletions src/vs/workbench/contrib/terminalContrib/links/browser/terminalLinkParsing.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ export interface IParsedLink {
export interface ILinkSuffix {
row: number | undefined;
col: number | undefined;
rowEnd: number | undefined;
colEnd: number | undefined;
suffix: ILinkPartialRange;
}

Expand All @@ -42,12 +44,20 @@ const linkSuffixRegex = new Lazy<RegExp>(() => generateLinkSuffixRegex(false));
function generateLinkSuffixRegex(eolOnly: boolean) {
let ri = 0;
let ci = 0;
function l(): string {
let rei = 0;
let cei = 0;
function r(): string {
return `(?<row${ri++}>\\d+)`;
}
function c(): string {
return `(?<col${ci++}>\\d+)`;
}
function re(): string {
return `(?<rowEnd${rei++}>\\d+)`;
}
function ce(): string {
return `(?<colEnd${cei++}>\\d+)`;
}

const eolSuffix = eolOnly ? '$' : '';

Expand All @@ -62,12 +72,12 @@ function generateLinkSuffixRegex(eolOnly: boolean) {
// foo:339
// foo:339:12
// foo 339
// foo 339:12 [#140780]
// foo 339:12 [#140780]
// "foo",339
// "foo",339:12
`(?::| |['"],)${l()}(:${c()})?` + eolSuffix,
// The quotes below are optional [#171652]
// "foo", line 339 [#40468]
`(?::| |['"],)${r()}(:${c()})?` + eolSuffix,
// The quotes below are optional [#171652]
// "foo", line 339 [#40468]
// "foo", line 339, col 12
// "foo", line 339, column 12
// "foo":line 339
Expand All @@ -80,18 +90,19 @@ function generateLinkSuffixRegex(eolOnly: boolean) {
// "foo" on line 339, col 12
// "foo" on line 339, column 12
// "foo" line 339 column 12
// "foo", line 339, character 12 [#171880]
// "foo", line 339, characters 12-13 [#171880]
// "foo", lines 339-340 [#171880]
`['"]?(?:,? |: ?| on )lines? ${l()}(?:-\\d+)?(?:,? (?:col(?:umn)?|characters?) ${c()}(?:-\\d+)?)?` + eolSuffix,
// "foo", line 339, character 12 [#171880]
// "foo", line 339, characters 12-14 [#171880]
// "foo", lines 339-341 [#171880]
// "foo", lines 339-341, characters 12-14 [#178287]
`['"]?(?:,? |: ?| on )lines? ${r()}(?:-${re()})?(?:,? (?:col(?:umn)?|characters?) ${c()}(?:-${ce()})?)?` + eolSuffix,
// foo(339)
// foo(339,12)
// foo(339, 12)
// foo (339)
// ...
// foo: (339)
// ...
`:? ?[\\[\\(]${l()}(?:, ?${c()})?[\\]\\)]` + eolSuffix,
`:? ?[\\[\\(]${r()}(?:, ?${c()})?[\\]\\)]` + eolSuffix,
];

const suffixClause = lineAndColumnRegexClauses
Expand Down Expand Up @@ -129,25 +140,6 @@ export function removeLinkQueryString(link: string): string {
return link.substring(0, index);
}

/**
* Returns the optional link suffix which contains line and column information.
* @param link The link to parse.
*/
export function getLinkSuffix(link: string): ILinkSuffix | null {
const matches = linkSuffixRegexEol.value.exec(link);
const groups = matches?.groups;
if (!groups || matches.length < 1) {
return null;
}
const rowString = groups.row0 || groups.row1 || groups.row2;
const colString = groups.col0 || groups.col1 || groups.col2;
return {
row: rowString !== undefined ? parseInt(rowString) : undefined,
col: colString !== undefined ? parseInt(colString) : undefined,
suffix: { index: matches.index, text: matches[0] }
};
}

export function detectLinkSuffixes(line: string): ILinkSuffix[] {
// Find all suffixes on the line. Since the regex global flag is used, lastIndex will be updated
// in place such that there are no overlapping matches.
Expand All @@ -164,20 +156,35 @@ export function detectLinkSuffixes(line: string): ILinkSuffix[] {
return results;
}

/**
* Returns the optional link suffix which contains line and column information.
* @param link The link to parse.
*/
export function getLinkSuffix(link: string): ILinkSuffix | null {
return toLinkSuffix(linkSuffixRegexEol.value.exec(link));
}

export function toLinkSuffix(match: RegExpExecArray | null): ILinkSuffix | null {
const groups = match?.groups;
if (!groups || match.length < 1) {
return null;
}
const rowString = groups.row0 || groups.row1 || groups.row2;
const colString = groups.col0 || groups.col1 || groups.col2;
return {
row: rowString !== undefined ? parseInt(rowString) : undefined,
col: colString !== undefined ? parseInt(colString) : undefined,
row: parseIntOptional(groups.row0 || groups.row1 || groups.row2),
col: parseIntOptional(groups.col0 || groups.col1 || groups.col2),
rowEnd: parseIntOptional(groups.rowEnd0 || groups.rowEnd1 || groups.rowEnd2),
colEnd: parseIntOptional(groups.colEnd0 || groups.colEnd1 || groups.colEnd2),
suffix: { index: match.index, text: match[0] }
};
}

function parseIntOptional(value: string | undefined): number | undefined {
if (value === undefined) {
return value;
}
return parseInt(value);
}

// This defines valid path characters for a link with a suffix, the first `[]` of the regex includes
// characters the path is not allowed to _start_ with, the second `[]` includes characters not
// allowed at all in the path. If the characters show up in both regexes the link will stop at that
Expand Down
12 changes: 9 additions & 3 deletions src/vs/workbench/contrib/terminalContrib/links/test/browser/terminalLinkOpeners.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,9 @@ suite('Workbench - TerminalLinkOpeners', () => {
source: 'editor',
selection: {
startColumn: 5,
startLineNumber: 10
startLineNumber: 10,
endColumn: undefined,
endLineNumber: undefined
},
});
});
Expand Down Expand Up @@ -421,7 +423,9 @@ suite('Workbench - TerminalLinkOpeners', () => {
source: 'editor',
selection: {
startColumn: 5,
startLineNumber: 10
startLineNumber: 10,
endColumn: undefined,
endLineNumber: undefined
},
});
await opener.open({
Expand All @@ -434,7 +438,9 @@ suite('Workbench - TerminalLinkOpeners', () => {
source: 'editor',
selection: {
startColumn: 5,
startLineNumber: 10
startLineNumber: 10,
endColumn: undefined,
endLineNumber: undefined
},
});
});
Expand Down
46 changes: 43 additions & 3 deletions src/vs/workbench/contrib/terminalContrib/links/test/browser/terminalLinkParsing.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,11 @@ interface ITestLink {
link: string;
prefix: string | undefined;
suffix: string | undefined;
// TODO: These has vars would be nicer as a flags enum
hasRow: boolean;
hasCol: boolean;
hasRowEnd?: boolean;
hasColEnd?: boolean;
}

const operatingSystems: ReadonlyArray<OperatingSystem> = [
Expand All @@ -33,6 +36,8 @@ const osLabel: { [key: number | OperatingSystem]: string } = {

const testRow = 339;
const testCol = 12;
const testRowEnd = 341;
const testColEnd = 14;
const testLinks: ITestLink[] = [
// Simple
{ link: 'foo', prefix: undefined, suffix: undefined, hasRow: false, hasCol: false },
Expand Down Expand Up @@ -117,8 +122,9 @@ const testLinks: ITestLink[] = [

// OCaml-style
{ link: '"foo", line 339, character 12', prefix: '"', suffix: '", line 339, character 12', hasRow: true, hasCol: true },
{ link: '"foo", line 339, characters 12-13', prefix: '"', suffix: '", line 339, characters 12-13', hasRow: true, hasCol: true },
{ link: '"foo", lines 339-340', prefix: '"', suffix: '", lines 339-340', hasRow: true, hasCol: false },
{ link: '"foo", line 339, characters 12-14', prefix: '"', suffix: '", line 339, characters 12-14', hasRow: true, hasCol: true, hasColEnd: true },
{ link: '"foo", lines 339-341', prefix: '"', suffix: '", lines 339-341', hasRow: true, hasCol: false, hasRowEnd: true },
{ link: '"foo", lines 339-341, characters 12-14', prefix: '"', suffix: '", lines 339-341, characters 12-14', hasRow: true, hasCol: true, hasRowEnd: true, hasColEnd: true },

// Non-breaking space
{ link: 'foo\u00A0339:12', prefix: undefined, suffix: '\u00A0339:12', hasRow: true, hasCol: true },
Expand Down Expand Up @@ -148,6 +154,8 @@ suite('TerminalLinkParsing', () => {
testLink.suffix === undefined ? null : {
row: testLink.hasRow ? testRow : undefined,
col: testLink.hasCol ? testCol : undefined,
rowEnd: testLink.hasRowEnd ? testRowEnd : undefined,
colEnd: testLink.hasColEnd ? testColEnd : undefined,
suffix: {
index: testLink.link.length - testLink.suffix.length,
text: testLink.suffix
Expand All @@ -165,6 +173,8 @@ suite('TerminalLinkParsing', () => {
testLink.suffix === undefined ? [] : [{
row: testLink.hasRow ? testRow : undefined,
col: testLink.hasCol ? testCol : undefined,
rowEnd: testLink.hasRowEnd ? testRowEnd : undefined,
colEnd: testLink.hasColEnd ? testColEnd : undefined,
suffix: {
index: testLink.link.length - testLink.suffix.length,
text: testLink.suffix
Expand All @@ -181,6 +191,8 @@ suite('TerminalLinkParsing', () => {
{
col: 2,
row: 1,
rowEnd: undefined,
colEnd: undefined,
suffix: {
index: 3,
text: '(1, 2)'
Expand All @@ -189,6 +201,8 @@ suite('TerminalLinkParsing', () => {
{
col: 4,
row: 3,
rowEnd: undefined,
colEnd: undefined,
suffix: {
index: 13,
text: '[3, 4]'
Expand All @@ -197,6 +211,8 @@ suite('TerminalLinkParsing', () => {
{
col: undefined,
row: 5,
rowEnd: undefined,
colEnd: undefined,
suffix: {
index: 23,
text: ' on line 5'
Expand Down Expand Up @@ -233,6 +249,8 @@ suite('TerminalLinkParsing', () => {
suffix: {
col: 2,
row: 1,
rowEnd: undefined,
colEnd: undefined,
suffix: {
index: 3,
text: '(1, 2)'
Expand All @@ -248,6 +266,8 @@ suite('TerminalLinkParsing', () => {
suffix: {
col: 4,
row: 3,
rowEnd: undefined,
colEnd: undefined,
suffix: {
index: 13,
text: '[3, 4]'
Expand All @@ -266,6 +286,8 @@ suite('TerminalLinkParsing', () => {
suffix: {
col: undefined,
row: 5,
rowEnd: undefined,
colEnd: undefined,
suffix: {
index: 24,
text: '" on line 5'
Expand All @@ -292,6 +314,8 @@ suite('TerminalLinkParsing', () => {
suffix: {
row: 5,
col: 6,
rowEnd: undefined,
colEnd: undefined,
suffix: {
index: 4,
text: '", line 5, col 6'
Expand All @@ -318,6 +342,8 @@ suite('TerminalLinkParsing', () => {
suffix: {
row: 5,
col: 6,
rowEnd: undefined,
colEnd: undefined,
suffix: {
index: 10,
text: '", line 5, col 6'
Expand Down Expand Up @@ -353,6 +379,8 @@ suite('TerminalLinkParsing', () => {
suffix: {
row: 5,
col: 6,
rowEnd: undefined,
colEnd: undefined,
suffix: {
index: 41,
text: '", line 5, col 6'
Expand Down Expand Up @@ -392,6 +420,8 @@ suite('TerminalLinkParsing', () => {
suffix: {
col: undefined,
row: 400,
rowEnd: undefined,
colEnd: undefined,
suffix: {
index: 27,
text: ':400'
Expand Down Expand Up @@ -446,6 +476,8 @@ suite('TerminalLinkParsing', () => {
suffix: {
col: undefined,
row: 400,
rowEnd: undefined,
colEnd: undefined,
suffix: {
index: 1 + osTestPath[os].length,
text: ':400'
Expand All @@ -466,6 +498,8 @@ suite('TerminalLinkParsing', () => {
suffix: {
col: undefined,
row: 400,
rowEnd: undefined,
colEnd: undefined,
suffix: {
index: 1 + osTestPath[os].length,
text: ':400'
Expand Down Expand Up @@ -573,7 +607,7 @@ suite('TerminalLinkParsing', () => {
const link2 = testLinksWithSuffix[i + 1];
const link3 = testLinksWithSuffix[i + 2];
const line = ` ${link1.link} ${link2.link} ${link3.link} `;
test('`' + line + '`', () => {
test('`' + line.replaceAll('\u00A0', '<nbsp>') + '`', () => {
strictEqual(detectLinks(line, OperatingSystem.Linux).length, 3);
ok(link1.suffix);
ok(link2.suffix);
Expand All @@ -590,6 +624,8 @@ suite('TerminalLinkParsing', () => {
suffix: {
row: link1.hasRow ? testRow : undefined,
col: link1.hasCol ? testCol : undefined,
rowEnd: link1.hasRowEnd ? testRowEnd : undefined,
colEnd: link1.hasColEnd ? testColEnd : undefined,
suffix: {
index: 1 + (link1.link.length - link1.suffix.length),
text: link1.suffix
Expand All @@ -608,6 +644,8 @@ suite('TerminalLinkParsing', () => {
suffix: {
row: link2.hasRow ? testRow : undefined,
col: link2.hasCol ? testCol : undefined,
rowEnd: link2.hasRowEnd ? testRowEnd : undefined,
colEnd: link2.hasColEnd ? testColEnd : undefined,
suffix: {
index: (detectedLink1.prefix?.index ?? detectedLink1.path.index) + link1.link.length + 1 + (link2.link.length - link2.suffix.length),
text: link2.suffix
Expand All @@ -626,6 +664,8 @@ suite('TerminalLinkParsing', () => {
suffix: {
row: link3.hasRow ? testRow : undefined,
col: link3.hasCol ? testCol : undefined,
rowEnd: link3.hasRowEnd ? testRowEnd : undefined,
colEnd: link3.hasColEnd ? testColEnd : undefined,
suffix: {
index: (detectedLink2.prefix?.index ?? detectedLink2.path.index) + link2.link.length + 1 + (link3.link.length - link3.suffix.length),
text: link3.suffix
Expand Down