Re-enable link detection (fixes #34026)

[mymikemiller]

This change brings back a handy feature: clickable file links for stack traces in the Debug Console and Exception Widget.

[msftclas]

All CLA requirements met.

[isidorn]

Hi thanks for this PR. However I am not so happy how this is done.
In a perfect world the code should be shared across multiple locations: debug console, exception widget and so on. And here there does not seem to be any reuse and we are just creating html elements on the fly.

That feature is not so straight forward for Pull Requests that is why it does not have a label "Help Wanted".
A proper solution would requrire more refactoring and rewriting which I currently do not have time for.

Thanks again for the PR and next time before submitting a PR starting a discussion is usually best

[mymikemiller]

I understand, and thanks for taking a look at the code! This is my first attempt at contributing to an open source project; I guess I don't quite know the ropes yet.

From what I can see, the handleLinks function is only used in the two places you mentioned, and both of the callers have special logic depending on whether the returned value is a string (i.e. nothing was changed) or an HTMLElement (links were added). Is there a reason not to have handleLinks always return its result wrapped in a span element, even if no links were added? That way, the line-splitting logic I added to exceptionWidget could be put into handleLinks (where it probably belonged in the first place), both callers could be simplified and future callers wouldn't have to handle the results differently depending on the return type.

Would you mind if I made these changes and added them to the PR? I know it's not the proper solution, but I think it would make users happy until you have time for the refactor.

Let me know if you'd be willing to take a look at an updated PR, otherwise I'll wait patiently for the official fix from the pros.

[isidorn]

I will take a look at the updated PR but I do not guarantee we will merge it in.
Thanks

[mymikemiller]

I understand if you choose not to merge my changes. I mostly did it for my own benefit, as clickable links in stack traces make my debugging a lot easier.

My recent commit also fixes another issue I spotted. Previously (well, if execution could get past the "if (!resource)" check that currently always fails), if anyone ever added another regex to FILE_LOCATION_PATTERNS, and multiple patterns found matches, the text would be duplicated (once for each pattern that found a match) in the returned HTMLElement. My fix uses recursion to run handleLinks only on the parts of the text that didn't match the pattern.

[isidorn]

@mymikemiller nice job, this looks better now.
The only missing thing now is confidence for me to merge this in. Any my confidence would greatly be increased by unit tests. Is it possible that you write some good unit tests for this as it seems nicely testable imho.
If you have time it would be great if you create a src/vs/workbench/parts/debug/test/browser/linkDetector.test.ts with a lot of unit tests.

Did you test this both for the exception widget and the debug console?

[mymikemiller]

Thanks, @isidorn. I'd be happy to write some unit tests. Stay tuned.

And yes, it works for absolute paths in both the Exception Widget and Debug Console.

[mymikemiller]

@isidorn Tests are in. Sorry for the delay - I started a new job this week :)

The tests cover all paths in handleLinks, though they don't exhaustively test the regex. It currently only tests for a simple match of /foo/bar.js:12:34.

Also, I don't have a Windows machine, so I didn't try running the tests on Windows (or anything other than MacOS). I noticed that the output would be different when the tests are run on a Windows machine, so I coded for that, but didn't test if it works.

Let me know if there are any other cases I ought to test for. Thanks again for considering this PR!

[isidorn]

Amazing work, one of the better PRs I got in quite some time.
I tried it a bit and it looks good.
I will merge this in to master, so we start self hosting on this and we get user feedback. In case we hit some issues I will ping you on those issues.

Thanks again