Linkify variable values in repl; #79198

[dgozman]

Every place which uses renderExpression or renderVariable provides a LinkDetector
to turn matching text into links.

Here is a screenshot with a lot of links:

@isidorn Could you please take a look?

[isidorn]

@dgozman thanks for this PR. The code changes look great, and I like this UI change overall. I have left some minor comments inline. Also I have the following concerns / questitions:

• LinkDetector.handleLinks might not be the best code since it came through numerous PRs, and now we are relying on it more
• Did you also render links as clicable everywhere in Chrome Dev Tools?

fyi @weinand for potential feedback on rendering links everywhere. Though this feels like a good move to me if our link handling proves to be good enough (where I have some doubts)

[dgozman]

Thank you for taking a look, @isidorn!

LinkDetector.handleLinks might not be the best code since it came through numerous PRs, and now we are relying on it more

I am open to any suggestions 😄 I see that TerminalLinkHandler has a different way of handling links.

Did you also render links as clicable everywhere in Chrome Dev Tools?

No, we did not. I think that repl should linkify everything, debug hover should not, and variables/watches could do it. I did it everywhere in this PR to facilitate UX discussion. I am not sure whether there is a process to experiment with such a change, or gather user feedback, or something else. For now, we can land links in repl, because that should be safe. What do you think?

[isidorn]

I forgot why are we not using the same LinkHandler everywhere, might you remember @Tyriar
Can we somehow extract parts of the TerminalLinkHandler such that it does not require a Terminal and that it can be reused in the Debug Console.

I also see we have a LinkComputer in the editor area. However that probably can not be used wihtout an editor (and we only have a tree in the debug console). @rebornix

So ideally we could get rid of the debug link detector. Though I doubt that will be possible. But I think we can optimistically expose the link handler in more places as this PR suggests and then fix the Link Detector later if we hit issues.

As for where to show links:

• Agree that we should not linkify in Debug Hover
• Agree we should linkify everywhere in Debug Console
• Watch Expressions and Variables should have the same experience. I can be convinced to go either way

We usually get feedback by pushing to Insiders. So this is a good time to push this change since we will have 3 weeks before we release a new Stable build.

Apart from that we never really gave Links a lot of love in Debug. I think we can improve that now, and for that we should just follow what Editor is doing. You can type "http://www.microsoft.com" in the editor and checkout the experience:

• It is underlined - same as you did which is great
• On hover we show the message "Cmd + click to follow link" probably "Ctrl + click" on windows
• We only follow the link on cmd / ctrl + click
• Mouse is a pointer only when cmd is pressed. Should not be too tricky to add a class if the cmd is pressed or not

I believe that we currently open link on click which is too aggressive imho, especially in the Watch and Variables view.

fyi @octref since he worked on links in general last milestone

[Tyriar]

I forgot why are we not using the same LinkHandler everywhere, might you remember @Tyriar
Can we somehow extract parts of the TerminalLinkHandler such that it does not require a Terminal and that it can be reused in the Debug Console.

The reason was because we never invested effort trying to make the method for link computing compatible across all the components, the terminal in particular is tougher because it's out of the microsoft/vscode repo.

I'm also not actually a fan of how the terminal does links right now and want to change the system eventually. Here's the xterm.js API right now, it's still marked as experimental because I don't like it:

  /**
   * An object containing options for a link matcher.
   */
  export interface ILinkMatcherOptions {
    /**
     * The index of the link from the regex.match(text) call. This defaults to 0
     * (for regular expressions without capture groups).
     */
    matchIndex?: number;

    /**
     * A callback that validates whether to create an individual link, pass
     * whether the link is valid to the callback.
     */
    validationCallback?: (uri: string, callback: (isValid: boolean) => void) => void;

    /**
     * A callback that fires when the mouse hovers over a link for a moment.
     */
    tooltipCallback?: (event: MouseEvent, uri: string) => boolean | void;

    /**
     * A callback that fires when the mouse leaves a link. Note that this can
     * happen even when tooltipCallback hasn't fired for the link yet.
     */
    leaveCallback?: () => void;

    /**
     * The priority of the link matcher, this defines the order in which the
     * link matcher is evaluated relative to others, from highest to lowest. The
     * default value is 0.
     */
    priority?: number;

    /**
     * A callback that fires when the mousedown and click events occur that
     * determines whether a link will be activated upon click. This enables
     * only activating a link when a certain modifier is held down, if not the
     * mouse event will continue propagation (eg. double click to select word).
     */
    willLinkActivate?: (event: MouseEvent, uri: string) => boolean;
  }

  export class Terminal implements IDisposable {
    /**
     * (EXPERIMENTAL) Registers a link matcher, allowing custom link patterns to
     * be matched and handled.
     * @param regex The regular expression to search for, specifically this
     * searches the textContent of the rows. You will want to use \s to match a
     * space ' ' character for example.
     * @param handler The callback when the link is called.
     * @param options Options for the link matcher.
     * @return The ID of the new matcher, this can be used to deregister.
     */
    registerLinkMatcher(regex: RegExp, handler: (event: MouseEvent, uri: string) => void, options?: ILinkMatcherOptions): number;

    /**
     * (EXPERIMENTAL) Deregisters a link matcher if it has been registered.
     * @param matcherId The link matcher's ID (returned after register)
     */
    deregisterLinkMatcher(matcherId: number): void;
  }

In xtermjs/xterm.js#583 I hope to move to a parser model so things like trailing ),]: etc. chars can be caught without crazy complex regexes. This would likely involve xterm.js embedders to plug in their own parsers since fs links are definitely not desired in a standard web terminal.

Another interesting thing about links in the terminal is that fs links are currently getting verified which causes issues in remote file systems due to the latency of these requests, the links parsing is also done very sparingly and restricted to the viewport only (not sure if the editor does this).

[dgozman]

@isidorn I've addressed the feedback and constrained links only to repl for now. I can follow up with a separate PR for variables/watches. I will also look into improving LinkDetector and/or reusing some functionality from TerminalLinkHandler, in a separate refactoring. Please take another look.

[isidorn]

I think changes in this file are no longer necessery. Especially for the debug hover.

[dgozman]

The .expression .value a is still needed for everything rendered by renderExpressionValue. Removed the debug hover part. Actually, these generic ones also work for repl, so I removed extra css from repl.css.

[isidorn]

@dgozman thanks. Starting with just the repl makes sense. I am very happy for the possible improvements to the LinkDetector. Once we have that if we are happy with how it behaves we can also start linkifying in the Variables and Watch.
I have done an additional review and left comments inline. Once you address them we can merge this in.

The biggest issue for me is that it seems that the link does not get detected if it does not have a colon line number at the end. Though this is probably an issue with the LinkDetector which we can tackle in a seperate PR. Example:
/Users/isidor/development/w/test/hello.js:4is detected
/Users/isidor/development/w/test/hello.js is not detected

[dgozman]

@isidorn Please take another look.

I agree that LinkDetector has some strange logic which should be fixed in a separate PR.

[dgozman]

The .expression .value a is still needed for everything rendered by renderExpressionValue. Removed the debug hover part. Actually, these generic ones also work for repl, so I removed extra css from repl.css.

[isidorn]

Looks great - merging in 🍻
Thanks a lot! In case you want to change the LinkDetector feel free to go crazy in that file, I would love to get rid of as much code as possible from there.