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

terminal: Support VS Code Terminal Link Providers #11552

Merged

Conversation

planger
Copy link
Contributor

@planger planger commented Aug 10, 2022

What it does

  • Implements the previously mocked support for Terminal Link Providers ([vscode] Implement window.registerTerminalLinkProvider #11521)
  • Replaces interface TerminalLink with a class to be compatible with VS Code ([vscode] TerminalLink turned into a class with constructor #11507)
  • Introduces a dedicated Theia contribution point TerminalLinkProvider for registering link providers via dependency injection
  • Removes the previous link matcher API in favor of the new link provider contribution point API to avoid two different mechanisms for doing the same thing: adding links to the terminal; as well as to get rid of the usage of the deprecated xterm.registerLinkMatcher API (Use Xterm's registerLinkProvider instead of registerLinkMatcher #11491)
  • Migrate existing link matchers to the new link provider contribution point API (5 matchers)
  • Fix UI bug where the link hover doesn't show up sometimes
    This occurs when the mouse happened to be on the position where the hover is placed as it gets visible, which leads to a mouse event that makes xterm cancel the hover right away, because the mouse "seemingly" left the link. This happened quite easily when you enter the link from below and move the mouse up, passing 30 px up during the delay until the hover shows up.
    This has been fixed by preventing to hide the hover if the mouse left the link but is above the hover, as well as using the actual current mouse position to define the location of the hover, instead of the one that we had when scheduling the hover with delay.
  • Makes the hover delay configurable via the workspace hover preference.
  • Shows modifier info (e.g. Ctrl + Click) by default next to the hover text, which is needed because link providers can provider a custom message
  • Turn the hover text into a clickable link, similar to VS Code, which makes it very convenient to follow a link with just the mouse and without having to push Ctrl. This only got possible by avoiding to hide the hover if the mouse moves above the hover and thus leaves the link.

Contributed on behalf of STMicroelectronics.

Fixes #11521
Fixes #11507
Fixes #11491

How to test

Below is what I tested as well as a short GIF showing how it looks like:

terminal-links

General Theia link provider contribution
  • Test whether migrated link matchers still work
    • URL link matcher (e.g. https://theia-ide.org/docs)
    • Localhost link matcher (e.g. localhost:3000)
    • File matcher should show a link on existing relative (to cwd) and absolute files (e.g. ./test.html)
    • File matcher within a git diff (e.g. --- a/<git-relative-path>/file)
  • Test whether Ctrl/Cmd click on the link works as expected for all cases above
  • Test whether the hover behavior is nice when moving above links and hovers and exit them again
  • Test whether the link in the hover works by just clicking without holding Ctrl/Cmd
VS Code extensions' link providers
  • Create a VS Code extension with link provider: for testing I created and uploaded testterminallinkprovider-0.0.1.vsix to simplify testing
    • Activates only after running the task Enable test terminal link provider to test if dynamic registration works
    • It matches all occurrences of link: in the terminal and turns them into a link
    • Provides a custom tooltip
    • Shows a notification on click

Review checklist

Reminder for reviewers

@planger
Copy link
Contributor Author

planger commented Aug 10, 2022

Reminder to myself: Consider #11398 after that PR has been merged!

@JonasHelming JonasHelming added the vscode issues related to VSCode compatibility label Aug 11, 2022
@msujew
Copy link
Member

msujew commented Aug 11, 2022

@planger Great, I believe that also addresses #11491, right? Anyway, I'll look into the changes.

@planger
Copy link
Contributor Author

planger commented Aug 11, 2022

@msujew Yes, that's correct! Thanks for the info! I've updated the description.

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

I can already confirm that the functionality works as expected 👍

I have a small favor to ask for the provided test extension. Do you mind adding a command to dynamically unregister the link provider again? I would love to test that as well. The code for that looks reasonable, but I'd like to be better safe than sorry :D

packages/plugin-ext/src/plugin/terminal-ext.ts Outdated Show resolved Hide resolved
packages/plugin-ext/src/plugin/terminal-ext.ts Outdated Show resolved Hide resolved
packages/plugin-ext/src/plugin/types-impl.ts Outdated Show resolved Hide resolved
packages/plugin-ext/src/plugin/types-impl.ts Outdated Show resolved Hide resolved
packages/terminal/package.json Outdated Show resolved Hide resolved
packages/terminal/src/browser/terminal-link-provider.ts Outdated Show resolved Hide resolved
@planger
Copy link
Contributor Author

planger commented Aug 11, 2022

@msujew Thank you very much for your detailed review and your great feedback! 👍

Good idea to test the disposal of the terminal link provider registration!
I've enhanced the VS Code extension so that it also comes with a command to unregister the terminal link provider (ie disposing the Disposable that is returned from the registration. The command is called Disable test terminal link provider. Here is the enhanced version: testterminallinkprovider-0.0.2.zip
For me it works fine.

I'll address and/or comment on your other suggestions inline.

Thanks again!

@planger
Copy link
Contributor Author

planger commented Aug 11, 2022

I've kept the changes for your suggestion in a separate commit c99474d to make it easier to review incrementally. Once you approve, I'm happy to squash and rebase. I'll likely have to rebase and resolve conflicts on top of #11398 as soon as that PR is merged anyway.

@planger planger force-pushed the planger/issues/11521 branch from c99474d to 5f11e46 Compare August 12, 2022 14:05
@planger
Copy link
Contributor Author

planger commented Aug 12, 2022

5f11e46 rebases this change to include the changes of #11398 and also squashes all commits of this PR.

@msujew
Copy link
Member

msujew commented Aug 15, 2022

@planger Please ping me when the CQ has been approved. I'll do a final review of the functionality afterwards. I don't have any more comments on the code for now.

@planger
Copy link
Contributor Author

planger commented Aug 16, 2022

CQ: https://dev.eclipse.org/ipzilla/show_bug.cgi?id=24248 (Status: NEW)

@vince-fugnitto vince-fugnitto added the CQ Required (deprecated) issues requiring a CQ (contributor questionnaire) label Aug 18, 2022
@planger planger force-pushed the planger/issues/11521 branch from 5f11e46 to a7992c4 Compare September 7, 2022 12:14
@planger
Copy link
Contributor Author

planger commented Sep 7, 2022

a7992c4 just rebases and resolves conflicts. Still waiting on https://dev.eclipse.org/ipzilla/show_bug.cgi?id=24248

@planger planger force-pushed the planger/issues/11521 branch from a7992c4 to b23a19a Compare September 28, 2022 18:26
@planger
Copy link
Contributor Author

planger commented Sep 28, 2022

@msujew The CQ has now been resolved. So I rebased this change and retested everything. Seems to be still fine after resolving some minor conflicts.

PS: Please note that I had to remove all plugins listed in package.json locally for testing, as the download script trying to download from OpenVSX is currently failing for me (looks like for others too eclipse/openvsx#536).

Edit: PPS: The outage of OpenVSX also seems to break the build in the CI with the same error. So it is certainly not just me.

@planger planger force-pushed the planger/issues/11521 branch from b23a19a to 9baa004 Compare September 29, 2022 18:29
@JonasHelming JonasHelming requested a review from msujew October 11, 2022 19:37
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Alright, looks good to me 👍

  • The built-in link providers work as expected
  • The extension provides custom links once enabled
  • Using the disabling command correctly removes the custom link provider again

On another note: Do you mind adding a breaking change entry for this? Something along the lines of "The AbstractCmdClickTerminalContribution API has been removed in favor of the TerminalLinkProvider interface"?

* Implement the previously mocked support for Terminal Link Providers
* Introduce Theia contribution point for adding link providers
* Migrate existing link matchers to this new contribution point
* Get rid of the usage of the deprecated xterm.registerLinkMatcher API
* Fix UI bug where the link hover doesn't show up
  This happened when the mouse is above the hover as it gets
  visible, leading to xterm canceling the hover right away, because the
  mouse "seemingly" left the link.
  This has been fixed by preventing to hide it   if the mouse left the
  link but is above the hover.
* Turn the hover text into a clickable link

Contributed on behalf of STMicroelectronics.

Fixes eclipse-theia#11521
Fixes eclipse-theia#11507
Fixes eclipse-theia#11491

Change-Id: I01f907d778f4a5f0588202ea28c4c82252ab75dc
Signed-off-by: Philip Langer <[email protected]>
@planger planger force-pushed the planger/issues/11521 branch from 9baa004 to bb17f8e Compare October 12, 2022 16:14
@planger
Copy link
Contributor Author

planger commented Oct 12, 2022

@msujew Thank you for your review! I added a line to the changelog as you suggested, rebased the change on current master and gave it another quick test. 👍

@JonasHelming JonasHelming merged commit cac96ae into eclipse-theia:master Oct 12, 2022
@@ -61,6 +61,7 @@ generating the main code bundle (as before), the second serves to generate a *.c
- [core] changed type of `FrontendApplicationConfig#defaultTheme` from `string` to `DefaultTheme` [#11570](https://github.com/eclipse-theia/theia/pull/11570)
- From now on, the default theme can be dispatched based on the OS theme. Use `DefaultTheme#defaultForOSTheme` to derive the `string` theme ID.
- [plugin-ext] removed `ctrlcmd+shift+l` keybinding for `pluginsView:toggle` [#11608](https://github.com/eclipse-theia/theia/pull/11608)
- [terminal] The `AbstractCmdClickTerminalContribution` API has been removed in favor of the `TerminalLinkProvider` interface [#11552](https://github.com/eclipse-theia/theia/pull/11552) - Contributed on behalf of STMicroelectronics
Copy link
Member

Choose a reason for hiding this comment

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

@planger I believe you meant to add this under 1.31.0 not 1.30.0 which was already released? If so I can update when I update the changelog before the 1.31.0 release :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh right, I'm sorry about that. I've opened a PR to fix that: #11761

@planger planger deleted the planger/issues/11521 branch October 13, 2022 12:16
@vince-fugnitto vince-fugnitto added this to the 1.31.0 milestone Oct 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CQ Required (deprecated) issues requiring a CQ (contributor questionnaire) vscode issues related to VSCode compatibility
Projects
None yet
4 participants