Link detection for the exception widget and debug console #24451

michelkaporin · 2017-04-10T16:32:31Z

Extracted link detection to a separate common class, shared and reused between REPL viewer and exception widget.
Updated link detection to handle multiple links in a provided string.
Added link validation to resolve only files which are part of file system.

Partially reused regex of the reverted PR by @felixfbecker, credits to you!

Fixes #15760, #17085 (and all its ancestors).
Partially implements #21349.

…scode into exception-link-detection

mention-bot · 2017-04-10T16:32:32Z

@michelkaporin, thanks for your PR! By analyzing the history of the files in this pull request, we identified @egamma and @bpasero to be potential reviewers.

felixfbecker · 2017-04-10T16:39:19Z

Awesome! Did you test this with stack traces printed by mocha (relative paths)?

michelkaporin · 2017-04-10T16:53:40Z

@felixfbecker Thank you :) I did not explicitly test mocha but those mocha stack traces described in #15760 should be picked up now (see the the sample image above which is a super set of what was outputted there).

The things that won't work currently:

If file is coming from e.g. <node internals> or does not exist on file system, it won't be anchored.
http:// links still not supported.
If the path to the file defined without the column, it won't be recognised.

I plan to tackle those things in a separate issues.

felixfbecker · 2017-04-10T16:57:14Z

I don't see this explicitely in the image above, will it work for relative paths that do not take up a whole line? Eg

  at myfunc (src/myfunc.ts:1:0)

right now that only highlights /myfunc.ts:1:0 like it was an absolute path

michelkaporin · 2017-04-11T07:11:15Z

@felixfbecker yes, it will work for relative paths that do not take up a whole line. src/myfunc.ts:1:0 would be highlighted in your example.

isidorn · 2017-04-13T08:17:11Z

@michelkaporin I appologize for the slow response was busy around the release.

Just checked the PR, looks great!
I really like that you are reusing the logic between the repl and the exceptionWidget.

After discussing with @bpasero we agreed that doing a FS stat to figure out if something is a relative link is too heavy for a simple operation like this.

How precise would the relative path mathching be if you can not verify it against the real file system? If not very precise then we should not support relative file matching.
But all in all we need to drop the dependency to 'fs' in the linkDetector

felixfbecker · 2017-04-13T08:22:52Z

I think it should be doable to detect relative links without a stat. If spaces in paths are not to be supported, it's trivial. Otherwise you would need to have to look at common seperators like parenthesis. But please please keep relative link detection, I need to click on these >100 times a day in stack traces.

michelkaporin · 2017-04-13T08:39:05Z

@isidorn Ok, I can drop link validation and try to match it with regex then.
However, current terminal implementation is also doing fs.stat to validate the links in the terminal. If we plan to reuse link detection code between terminal, debug console and exception widget, then we will have to drop fs.stat for terminal as well.

isidorn · 2017-04-13T08:45:17Z

Thanks for letting me know, we should also not use fs.stat for link detection in the terminal.
fyi @Tyriar

Tyriar · 2017-04-17T18:44:50Z

@michelkaporin @isidorn @bpasero I disagree that we shouldn't stat the files. Without fs.stat there are a heap of false positives. A lot of stuff in the terminal is formed like a path, especially if we want to linkify files in the output of ls. Note that the terminal does not linkify all output, it happens 200ms after the viewport has not changed, so it will stat as many links that are matched in the viewport (typically 10-20 lines) at most every 200ms.

"Link validation" introduced in 1.11 (ie. stat the paths) https://code.visualstudio.com/updates/v1_11#_link-improvements, it was pretty bad before this was added imo.
I brought up the concern when linkifying ls output and suggested we opt for readdir there instead. Linkify terminal paths without any path separators #22772 (comment)
xterm.js link impl https://github.com/sourcelair/xterm.js/blob/master/src/Linkifier.ts

bpasero · 2017-04-18T04:45:55Z

Stating things we "think" are links is crazy to put it in simple words (keep in mind that people work in environments with network drives where a stat might not be as fast as on your SSD disk). Since output is unbounded this could easily cause tons of disk access for no helpful reason. A link should be detected without going to disk and it is totally fine to have false positives (e.g. it is OK to not be 100% correct). Once the user clicks on a link, it is fine to show an error if the file does not exist.

Tyriar · 2017-04-18T18:35:13Z

@bpasero to be more specific the terminal stuff is using pfs so it's not a blocking call or anything. Also for the terminal it is bounded as only the current viewport is checked for links.

Is it so bad to check the existence of a handful of paths after a terminal command is run (or when the user scrolls the terminal)?

bpasero · 2017-04-19T05:34:55Z

I would not do it, the output panel has never done it. There are tons of regexes that try to detect links and render them optimistically. What is wrong with that approach?

Tyriar · 2017-04-19T16:45:23Z

Tracking stat in terminal links in #25024

…ith resource schemes.

michelkaporin · 2017-05-03T15:54:58Z

@isidorn I have updated regex to be more precise without fs.stat validation. We get higher false positive rate now without it, although it is more efficient.

Exception widget is an example of high false positive rate with file names coming from debug adapter, that are detected by regex as workspace' relative links, whereas they are not, in reality. On the good side the user will get an 'Unable to open' message, when clicking on such links.

isidorn · 2017-05-05T09:38:51Z

src/vs/workbench/parts/debug/browser/linkDetector.ts

+		// group 3: line number
+		// group 4: column number
+		// 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


I notice that the comments are the same but the regex is now more complicated.
Could you please add some comments in code to clarify this regex beast

@isidorn I've added more comments, hope it makes things more clear. It is quite big just because it tries not to match lots of special characters. In the end we still plan to move away from it to the common regex between terminal, debug console and exception widget.

isidorn · 2017-05-05T09:40:14Z

src/vs/workbench/parts/debug/browser/linkDetector.ts

+		// noop
+	}
+
+	public handleLinks(text: string): HTMLElement | string {


As far as I understand there have been no changes till the end of this file, it has just been moved over.
If that is the case it looks good to me

@isidorn It was changed to handle multiple links in the provided string. Previously it used to detect only first link match and then stopped.

isidorn · 2017-05-05T09:41:48Z

src/vs/workbench/parts/debug/browser/linkDetector.ts

+	) {
+		// noop
+	}
+


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

@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.

isidorn · 2017-05-05T09:44:13Z

src/vs/workbench/parts/debug/browser/media/exceptionWidget.css

@@ -27,6 +27,11 @@
 	margin-top: 0.5em;
 }



Makes sense

isidorn · 2017-05-05T09:46:06Z

src/vs/workbench/parts/debug/electron-browser/replViewer.ts

@@ -7,13 +7,10 @@ import * as nls from 'vs/nls';
 import { TPromise } from 'vs/base/common/winjs.base';


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?

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

isidorn · 2017-05-05T09:46:55Z

src/vs/workbench/parts/debug/browser/exceptionWidget.ts

@@ -11,9 +11,11 @@ import { ICodeEditor } from 'vs/editor/browser/editorBrowser';
 import { IContextViewService } from 'vs/platform/contextview/browser/contextView';


isidorn · 2017-05-05T09:47:53Z

@michelkaporin great work. I have added some comments directly in the code. Once you adrress them we can merge it in.
I see that the builds are failing, is everything ok when you build it localy?

…ut. Corrected regex description.

…scode into exception-link-detection

michelkaporin · 2017-05-05T14:54:54Z

@isidorn thank you! I've addressed all your comments, please see my replies. The build is passing locally, AppVeyour succeeds also on the latest codebase. Cannot find any build issues in the Travis log that are to do with my changes.

isidorn · 2017-05-08T09:07:32Z

@michelkaporin looks good, let's merge it in to see how it behaves :)
@michelkaporin make sure all the issues that are fixed by this are closed and have a bug label - so we verify them in the endgame

michelkaporin added 8 commits April 3, 2017 15:53

Added multiple link detection for debug console and exception widget.

b32837c

Updated regex to handle relative paths and paths without column number.

353eec9

Added file link validation.

dd96b88

Added multiple link detection for debug console and exception widget.

c09c79f

Updated regex to handle relative paths and paths without column number.

0337cfd

Added file link validation.

e793d3e

Removed old regexes.

583fe76

Merge branch 'exception-link-detection' of github.com:michelkaporin/v…

37241e6

…scode into exception-link-detection

michelkaporin added the debug-console label Apr 10, 2017

michelkaporin added this to the April 2017 milestone Apr 10, 2017

michelkaporin requested a review from isidorn April 10, 2017 16:32

msftclas added the cla-already-signed label Apr 10, 2017

michelkaporin mentioned this pull request Apr 11, 2017

Merge link detection code between terminal, debug console and exception widget #24489

Closed

jrieken assigned isidorn Apr 11, 2017

Tyriar mentioned this pull request Apr 17, 2017

Adding line and column support for terminal #24832

Merged

Tyriar mentioned this pull request Apr 19, 2017

Look into removing file stat checks in terminal link detection #25024

Closed

michelkaporin modified the milestones: May 2017, April 2017 Apr 25, 2017

michelkaporin and others added 2 commits May 3, 2017 17:45

Got rid of fs.stat for validation. Amended regex to be more precise w…

5f87c7b

…ith resource schemes.

Merge branch 'master' into exception-link-detection

a6d3ae2

isidorn reviewed May 5, 2017

View reviewed changes

src/vs/workbench/parts/debug/browser/media/exceptionWidget.css

@@ -27,6 +27,11 @@

margin-top: 0.5em;

}

Copy link

Contributor

isidorn May 5, 2017

Choose a reason for hiding this comment

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

Makes sense

isidorn reviewed May 5, 2017

View reviewed changes

michelkaporin added 3 commits May 5, 2017 16:01

Added clarification to the regex and public method.

4962cfe

Removed redundant instantiations of LinkDetetector for each ANSI outp…

9901d2b

…ut. Corrected regex description.

Merge branch 'exception-link-detection' of github.com:michelkaporin/v…

df6122e

…scode into exception-link-detection

isidorn merged commit 63e81b3 into microsoft:master May 8, 2017

michelkaporin mentioned this pull request May 31, 2017

Cannot click on nodejs built-in modules #27712

Closed

DanTup mentioned this pull request Nov 23, 2017

Make file references in Debug Output (from hot reload, flutter test, exceptions) clickable Dart-Code/Dart-Code#451

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

Link detection for the exception widget and debug console #24451

Link detection for the exception widget and debug console #24451

michelkaporin commented Apr 10, 2017

mention-bot commented Apr 10, 2017

felixfbecker commented Apr 10, 2017 •

edited

Loading

michelkaporin commented Apr 10, 2017 •

edited

Loading

felixfbecker commented Apr 10, 2017

michelkaporin commented Apr 11, 2017

isidorn commented Apr 13, 2017

felixfbecker commented Apr 13, 2017

michelkaporin commented Apr 13, 2017

isidorn commented Apr 13, 2017

Tyriar commented Apr 17, 2017

bpasero commented Apr 18, 2017 •

edited

Loading

Tyriar commented Apr 18, 2017

bpasero commented Apr 19, 2017

Tyriar commented Apr 19, 2017

michelkaporin commented May 3, 2017

isidorn May 5, 2017

michelkaporin May 5, 2017

isidorn May 5, 2017

michelkaporin May 5, 2017 •

edited

Loading

isidorn May 5, 2017 •

edited

Loading

michelkaporin May 5, 2017 •

edited

Loading

isidorn May 5, 2017

isidorn May 5, 2017

michelkaporin May 5, 2017

isidorn May 5, 2017

isidorn commented May 5, 2017

michelkaporin commented May 5, 2017

isidorn commented May 8, 2017 •

edited

Loading

		@@ -7,13 +7,10 @@ import * as nls from 'vs/nls';
		import { TPromise } from 'vs/base/common/winjs.base';

		@@ -11,9 +11,11 @@ import { ICodeEditor } from 'vs/editor/browser/editorBrowser';
		import { IContextViewService } from 'vs/platform/contextview/browser/contextView';

Link detection for the exception widget and debug console #24451

Link detection for the exception widget and debug console #24451

Conversation

michelkaporin commented Apr 10, 2017

mention-bot commented Apr 10, 2017

felixfbecker commented Apr 10, 2017 • edited Loading

michelkaporin commented Apr 10, 2017 • edited Loading

felixfbecker commented Apr 10, 2017

michelkaporin commented Apr 11, 2017

isidorn commented Apr 13, 2017

felixfbecker commented Apr 13, 2017

michelkaporin commented Apr 13, 2017

isidorn commented Apr 13, 2017

Tyriar commented Apr 17, 2017

bpasero commented Apr 18, 2017 • edited Loading

Tyriar commented Apr 18, 2017

bpasero commented Apr 19, 2017

Tyriar commented Apr 19, 2017

michelkaporin commented May 3, 2017

isidorn May 5, 2017

Choose a reason for hiding this comment

michelkaporin May 5, 2017

Choose a reason for hiding this comment

isidorn May 5, 2017

Choose a reason for hiding this comment

michelkaporin May 5, 2017 • edited Loading

Choose a reason for hiding this comment

isidorn May 5, 2017 • edited Loading

Choose a reason for hiding this comment

michelkaporin May 5, 2017 • edited Loading

Choose a reason for hiding this comment

isidorn May 5, 2017

Choose a reason for hiding this comment

isidorn May 5, 2017

Choose a reason for hiding this comment

michelkaporin May 5, 2017

Choose a reason for hiding this comment

isidorn May 5, 2017

Choose a reason for hiding this comment

isidorn commented May 5, 2017

michelkaporin commented May 5, 2017

isidorn commented May 8, 2017 • edited Loading

felixfbecker commented Apr 10, 2017 •

edited

Loading

michelkaporin commented Apr 10, 2017 •

edited

Loading

bpasero commented Apr 18, 2017 •

edited

Loading

michelkaporin May 5, 2017 •

edited

Loading

isidorn May 5, 2017 •

edited

Loading

michelkaporin May 5, 2017 •

edited

Loading

isidorn commented May 8, 2017 •

edited

Loading