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

Linkify variable values in repl; #79198 #80502

Merged
merged 1 commit into from Sep 13, 2019

Conversation

@dgozman
Copy link
Contributor

commented Sep 6, 2019

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:
Screen Shot 2019-09-06 at 2 20 37 PM

@isidorn Could you please take a look?

@isidorn isidorn self-assigned this Sep 9, 2019
@isidorn isidorn added this to the September 2019 milestone Sep 9, 2019
@isidorn

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2019

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

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

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 dgozman force-pushed the dgozman:fix-79198 branch from 785add9 to 993a2e5 Sep 12, 2019
@dgozman dgozman changed the title Linkify values in debug hover, repl, watches and variables; fixes #79198 Linkify variable values in repl; fixes #79198 Sep 12, 2019
Copy link
Contributor Author

left a comment

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

@@ -147,6 +147,18 @@
margin-left: 6px;
}

/* Links */

This comment has been minimized.

Copy link
@isidorn

isidorn Sep 12, 2019

Contributor

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

This comment has been minimized.

Copy link
@dgozman

dgozman Sep 12, 2019

Author Contributor

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.

src/vs/workbench/contrib/debug/browser/repl.ts Outdated Show resolved Hide resolved
@isidorn

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2019

@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

Every place which uses renderExpression or renderVariable can provide a LinkDetector
to turn matching text into links. We use this on repl renderers.
@dgozman dgozman force-pushed the dgozman:fix-79198 branch from 993a2e5 to c526947 Sep 12, 2019
@dgozman dgozman changed the title Linkify variable values in repl; fixes #79198 Linkify variable values in repl; #79198 Sep 12, 2019
Copy link
Contributor Author

left a comment

@isidorn Please take another look.

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

src/vs/workbench/contrib/debug/browser/repl.ts Outdated Show resolved Hide resolved
@@ -147,6 +147,18 @@
margin-left: 6px;
}

/* Links */

This comment has been minimized.

Copy link
@dgozman

dgozman Sep 12, 2019

Author Contributor

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

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2019

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.

@isidorn isidorn merged commit 9b0b596 into microsoft:master Sep 13, 2019
2 checks passed
2 checks passed
VS Code #20190912.88 succeeded
Details
license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.