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

Link detection for the exception widget and debug console #24451

Merged
merged 13 commits into from
May 8, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions src/vs/workbench/parts/debug/browser/exceptionWidget.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,11 @@ import { ICodeEditor } from 'vs/editor/browser/editorBrowser';
import { IContextViewService } from 'vs/platform/contextview/browser/contextView';
Copy link
Contributor

Choose a reason for hiding this comment

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

looks good

import { IDebugService, IExceptionInfo } from 'vs/workbench/parts/debug/common/debug';
import { RunOnceScheduler } from 'vs/base/common/async';
import { IThemeService, ITheme } from 'vs/platform/theme/common/themeService';
import { Color } from 'vs/base/common/color';
import { registerColor } from 'vs/platform/theme/common/colorRegistry';
import { IThemeService, ITheme } from "vs/platform/theme/common/themeService";
import { Color } from "vs/base/common/color";
import { registerColor } from "vs/platform/theme/common/colorRegistry";
import { IInstantiationService } from 'vs/platform/instantiation/common/instantiation';
import { LinkDetector } from 'vs/workbench/parts/debug/browser/linkDetector';
const $ = dom.$;

// theming
Expand All @@ -28,7 +30,8 @@ export class ExceptionWidget extends ZoneWidget {
constructor(editor: ICodeEditor, private exceptionInfo: IExceptionInfo, private lineNumber: number,
@IContextViewService private contextViewService: IContextViewService,
@IDebugService private debugService: IDebugService,
@IThemeService themeService: IThemeService
@IThemeService themeService: IThemeService,
@IInstantiationService private instantiationService: IInstantiationService
) {
super(editor, { showFrame: true, showArrow: true, frameWidth: 1 });

Expand Down Expand Up @@ -79,7 +82,9 @@ export class ExceptionWidget extends ZoneWidget {

if (this.exceptionInfo.details && this.exceptionInfo.details.stackTrace) {
let stackTrace = $('.stack-trace');
stackTrace.textContent = this.exceptionInfo.details.stackTrace;
const linkDetector = this.instantiationService.createInstance(LinkDetector);
const linkedStackTrace = linkDetector.handleLinks(this.exceptionInfo.details.stackTrace);
typeof linkedStackTrace === 'string' ? stackTrace.textContent = linkedStackTrace : stackTrace.appendChild(linkedStackTrace);
dom.append(container, stackTrace);
}
}
Expand Down
113 changes: 113 additions & 0 deletions src/vs/workbench/parts/debug/browser/linkDetector.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
/*---------------------------------------------------------------------------------------------
* Copyright (c) Microsoft Corporation. All rights reserved.
* Licensed under the MIT License. See License.txt in the project root for license information.
*--------------------------------------------------------------------------------------------*/

import strings = require('vs/base/common/strings');
import uri from 'vs/base/common/uri';
import { isMacintosh } from 'vs/base/common/platform';
import * as errors from 'vs/base/common/errors';
import { IMouseEvent, StandardMouseEvent } from 'vs/base/browser/mouseEvent';
import * as nls from 'vs/nls';
import { IWorkbenchEditorService } from 'vs/workbench/services/editor/common/editorService';
import { IWorkspaceContextService } from 'vs/platform/workspace/common/workspace';

export class LinkDetector {
private static FILE_LOCATION_PATTERNS: RegExp[] = [
// group 0: full path with line and column
// group 1: full path without line and column, matched by `*.*` in the end to work only on paths with extensions in the end (s.t. node:10352 would not match)
// group 2: drive letter on windows with trailing backslash or leading slash on mac/linux
// group 3: line number, matched by (:(\d+))
// group 4: column number, matched by ((?::(\d+))?)
// eg: at Context.<anonymous> (c:\Users\someone\Desktop\mocha-runner\test\test.js:26:11)
/(?![\(])(?:file:\/\/)?((?:([a-zA-Z]+:)|[^\(\)<>\'\"\[\]:\s]+)(?:[\\/][^\(\)<>\'\"\[\]:]*)?\.[a-zA-Z]+[0-9]*):(\d+)(?::(\d+))?/g
];

constructor(
@IWorkbenchEditorService private editorService: IWorkbenchEditorService,
@IWorkspaceContextService private contextService: IWorkspaceContextService
) {
// noop
}

Copy link
Contributor

@isidorn isidorn May 5, 2017

Choose a reason for hiding this comment

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

Since this handleLinks is now a public method we could try to make the API a bit nicer, a personally find it ugly that it returns an HTMLElement or a string. It is hard to figure out what to expect as a return result for someobdy who just wants to use the link detector.
At least provide some comments to clarify each case, or try to always return an object with the corresponding fields.

For ideas you can also check what does the terminal / editor link detector do and how does their API look like

Copy link
Contributor Author

@michelkaporin michelkaporin May 5, 2017

Choose a reason for hiding this comment

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

@isidorn I've added the comment to it. I hope this explains it well.

If you have any suggestions on what should be a return result instead, I am happy to hear. Currently it perfectly suits exception widget and debug console needs.

/**
* Matches and handles relative and absolute file links in the string provided.
* Returns <span/> element that wraps the processed string, where matched links are replaced by <a/> and unmatched parts are surrounded by <span/> elements.
* 'onclick' event is attached to all anchored links that opens them in the editor.
* If no links were detected, returns the original string.
*/
public handleLinks(text: string): HTMLElement | string {
let linkContainer: HTMLElement;

for (let pattern of LinkDetector.FILE_LOCATION_PATTERNS) {
pattern.lastIndex = 0; // the holy grail of software development
let lastMatchIndex = 0;

let match = pattern.exec(text);
while (match !== null) {
let resource: uri = null;
try {
resource = (match && !strings.startsWith(match[0], 'http')) && (match[2] ? uri.file(match[1]) : this.contextService.toResource(match[1]));
} catch (e) { }

if (!resource) {
match = pattern.exec(text);
continue;
}
if (!linkContainer) {
linkContainer = document.createElement('span');
}

let textBeforeLink = text.substring(lastMatchIndex, match.index);
if (textBeforeLink) {
let span = document.createElement('span');
span.textContent = textBeforeLink;
linkContainer.appendChild(span);
}

const link = document.createElement('a');
link.textContent = text.substr(match.index, match[0].length);
link.title = isMacintosh ? nls.localize('fileLinkMac', "Click to follow (Cmd + click opens to the side)") : nls.localize('fileLink', "Click to follow (Ctrl + click opens to the side)");
linkContainer.appendChild(link);
const line = Number(match[3]);
const column = match[4] ? Number(match[4]) : undefined;
link.onclick = (e) => this.onLinkClick(new StandardMouseEvent(e), resource, line, column);

lastMatchIndex = pattern.lastIndex;
const currentMatch = match;
match = pattern.exec(text);

// Append last string part if no more link matches
if (!match) {
let textAfterLink = text.substr(currentMatch.index + currentMatch[0].length);
if (textAfterLink) {
let span = document.createElement('span');
span.textContent = textAfterLink;
linkContainer.appendChild(span);
}
}
}
}

return linkContainer || text;
}

private onLinkClick(event: IMouseEvent, resource: uri, line: number, column: number = 0): void {
const selection = window.getSelection();
if (selection.type === 'Range') {
return; // do not navigate when user is selecting
}

event.preventDefault();

this.editorService.openEditor({
resource,
options: {
selection: {
startLineNumber: line,
startColumn: column
}
}
}, event.ctrlKey || event.metaKey).done(null, errors.onUnexpectedError);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,11 @@
margin-top: 0.5em;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense

.monaco-editor .zone-widget .zone-widget-container.exception-widget a {
text-decoration: underline;
cursor: pointer;
}

/* High Contrast Theming */

.monaco-workbench.mac .zone-widget .zone-widget-container.exception-widget {
Expand Down
88 changes: 9 additions & 79 deletions src/vs/workbench/parts/debug/electron-browser/replViewer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,10 @@ import * as nls from 'vs/nls';
import { TPromise } from 'vs/base/common/winjs.base';
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good overall and I like we removed code from this class.

I only have the issue that you are creating a linkDetector at every iteration in a loop. Would it not make sense just to create one global one for the replViewer and always to reuse it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@isidorn You're right, did not see that for-loop when refactored. I've moved LinkDetector instantiation to the constructor.

import { IAction } from 'vs/base/common/actions';
import { isFullWidthCharacter, removeAnsiEscapeCodes, endsWith } from 'vs/base/common/strings';
import uri from 'vs/base/common/uri';
import { isMacintosh } from 'vs/base/common/platform';
import { IActionItem } from 'vs/base/browser/ui/actionbar/actionbar';
import * as dom from 'vs/base/browser/dom';
import * as errors from 'vs/base/common/errors';
import severity from 'vs/base/common/severity';
import { IMouseEvent, StandardMouseEvent } from 'vs/base/browser/mouseEvent';
import { IMouseEvent } from 'vs/base/browser/mouseEvent';
import { ITree, IAccessibilityProvider, IDataSource, IRenderer, IActionProvider } from 'vs/base/parts/tree/browser/tree';
import { ICancelableEvent } from 'vs/base/parts/tree/browser/treeDefaults';
import { IExpressionContainer, IExpression } from 'vs/workbench/parts/debug/common/debug';
Expand All @@ -23,6 +20,7 @@ import { ClearReplAction } from 'vs/workbench/parts/debug/browser/debugActions';
import { CopyAction } from 'vs/workbench/parts/debug/electron-browser/electronDebugActions';
import { IInstantiationService } from 'vs/platform/instantiation/common/instantiation';
import { IWorkbenchEditorService } from 'vs/workbench/services/editor/common/editorService';
import { LinkDetector } from 'vs/workbench/parts/debug/browser/linkDetector';

const $ = dom.$;

Expand Down Expand Up @@ -83,25 +81,18 @@ export class ReplExpressionsRenderer implements IRenderer {
private static VALUE_OUTPUT_TEMPLATE_ID = 'outputValue';
private static NAME_VALUE_OUTPUT_TEMPLATE_ID = 'outputNameValue';

private static FILE_LOCATION_PATTERNS: RegExp[] = [
// group 0: the full thing :)
// group 1: absolute path
// group 2: drive letter on windows with trailing backslash or leading slash on mac/linux
// group 3: line number
// group 4: column number
// eg: at Context.<anonymous> (c:\Users\someone\Desktop\mocha-runner\test\test.js:26:11)
/((\/|[a-zA-Z]:\\)[^\(\)<>\'\"\[\]]+):(\d+):(\d+)/
];

private static LINE_HEIGHT_PX = 18;

private width: number;
private characterWidth: number;

private linkDetector: LinkDetector;

constructor(
@IWorkbenchEditorService private editorService: IWorkbenchEditorService
@IWorkbenchEditorService private editorService: IWorkbenchEditorService,
@IInstantiationService private instantiationService: IInstantiationService
) {
// noop
this.linkDetector = this.instantiationService.createInstance(LinkDetector);
}

public getHeight(tree: ITree, element: any): number {
Expand Down Expand Up @@ -330,7 +321,7 @@ export class ReplExpressionsRenderer implements IRenderer {

// flush text buffer if we have any
if (buffer) {
this.insert(this.handleLinks(buffer), currentToken || tokensContainer);
this.insert(this.linkDetector.handleLinks(buffer), currentToken || tokensContainer);
buffer = '';
}

Expand All @@ -350,7 +341,7 @@ export class ReplExpressionsRenderer implements IRenderer {

// flush remaining text buffer if we have any
if (buffer) {
let res = this.handleLinks(buffer);
let res = this.linkDetector.handleLinks(buffer);
if (typeof res !== 'string' || currentToken) {
if (!tokensContainer) {
tokensContainer = document.createElement('span');
Expand All @@ -371,67 +362,6 @@ export class ReplExpressionsRenderer implements IRenderer {
}
}

private handleLinks(text: string): HTMLElement | string {
let linkContainer: HTMLElement;

for (let pattern of ReplExpressionsRenderer.FILE_LOCATION_PATTERNS) {
pattern.lastIndex = 0; // the holy grail of software development

const match = pattern.exec(text);
let resource: uri = null;
try {
resource = match && uri.file(match[1]);
} catch (e) { }

if (resource) {
linkContainer = document.createElement('span');

let textBeforeLink = text.substr(0, match.index);
if (textBeforeLink) {
let span = document.createElement('span');
span.textContent = textBeforeLink;
linkContainer.appendChild(span);
}

const link = document.createElement('a');
link.textContent = text.substr(match.index, match[0].length);
link.title = isMacintosh ? nls.localize('fileLinkMac', "Click to follow (Cmd + click opens to the side)") : nls.localize('fileLink', "Click to follow (Ctrl + click opens to the side)");
linkContainer.appendChild(link);
link.onclick = (e) => this.onLinkClick(new StandardMouseEvent(e), resource, Number(match[3]), Number(match[4]));

let textAfterLink = text.substr(match.index + match[0].length);
if (textAfterLink) {
let span = document.createElement('span');
span.textContent = textAfterLink;
linkContainer.appendChild(span);
}

break; // support one link per line for now
}
}

return linkContainer || text;
}

private onLinkClick(event: IMouseEvent, resource: uri, line: number, column: number): void {
const selection = window.getSelection();
if (selection.type === 'Range') {
return; // do not navigate when user is selecting
}

event.preventDefault();

this.editorService.openEditor({
resource,
options: {
selection: {
startLineNumber: line,
startColumn: column
}
}
}, event.ctrlKey || event.metaKey).done(null, errors.onUnexpectedError);
}

public disposeTemplate(tree: ITree, templateId: string, templateData: any): void {
// noop
}
Expand Down