-
Notifications
You must be signed in to change notification settings - Fork 28.8k
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
Allow custom link matchers to be registered in terminal API #18454
Comments
@isidorn FYI. |
Would also be awesome if I could reuse this link detection logic in the debug world since I have a bunch of feature requests for improving link detection in the REPL |
This needs to be in xterm.js due to the sensitivity and also the amount of DOM manipulation there. I put out a proposal on the project for how we could do this the other day. xtermjs/xterm.js#455 |
I'll likely be working on this in February. @dbaeumer @isidorn I want the plain terminal to have reasonable link detection (#7321), do you think you will need the ability to explicitly register link patterns if xterm.js always tries to match common link patterns? You can read my proposed solution here xtermjs/xterm.js#455 |
I think I would need to provide these patterns. For the task execution what users want is to click on the file part of a problem and open the file in the editor. Since the problem matcher of a task matches the file part as well I would need to communicate the matching part to the xterm to ensure the same patterns are used. |
@dbaeumer I've exposed the xterm.js functions on const matcherId = terminalInstance.registerLinkMatcher(/abc/, console.log, 0) |
As for exposing on the API, we can wrap the return export interface Terminal {
/**
* Registers a link matcher on the terminal, enabling custom links to be created and handled.
* @param regex The regular expression to search within a row for, specifically this searches
* the textContent of the row. Note that you will need to use \s to match a space ' ' character.
* @param callback The callback that is triggered when the link is triggered.
* @param matchIndex The index of the link from the regex.match(text) call. This defaults to 0
* (for regular expressions without capture groups).
* @return A disposable which removes the link matcher from the terminal.
*/
registerLinkMatcher(regex: RegExp, callback: (url: string) => void, matchIndex?: number): Disposable;
} @jrieken any feedback would be appreciated 😃 |
@Tyriar thanks. Will look into how to best integrate this. |
hm... we are generally not doing this things on instances but global, like Also, wrt the callback? We usually don't play it that way but just use |
The reason for it not being global is because the main use case for this is the tasks framework, which will conditionally register link handlers based on the task running. Conditional registration is because link matchers will only be valid under certain tasks (for example tasks that are run on a directory may not fully quality the file). There is a line reader as well but for performance reasons you can only add lines using this, the regex will only check rows that haven't updated in 200ms. The proposed api is very similar to the upstream api (which I'm also proposing) that will handle everything, that however needs to be a generic callback function. The issue with using a Uri as the handler is that the link is whatever you clicked, this could be something like 'tsconfig.json' in the case of some tasks which isn't a valid Uri as it needs more context. This could however be a function to map the link match to a Uri. @dbaeumer am I right here as your use case is the one we need to cover? |
We should not build API if we only have a single use-case. Question for other use-cases which might drive this API in another direction:
|
Great questions! TL;DR
This is always on (and in Insiders now), you can override this detection by using regex that matches that row. For example
This would be possible under the tasks framework, or if the commands were triggered via extensions. The issue here is each individual row has no context as to what process/cwd generated the text.
Yes in conjunction with terminal.onData(row => {
if (row.match(/$ ls\n/)) {
//registerLinkMatcher(...)
}
});
// Dispose the link matcher when the new prompt is seen
How the link matchers are scoped is an interesting thought. The reason they're not global right now is because very specific link matchers like an I also proposed a change in xtermjs/xterm.js#538 (comment) that would register link matchers on a per-row basis but having per-row and global would probably make sense to save memory. The http link matcher for example would be global which would save |
@Tyriar I would love to get EDIT: so I looked at #25331 and it says that
Is that a regression? I can file a new issue. EDIT2: |
@michaelchiche yeah there's another issue for following the cwd |
This issue is being closed to keep the number of issues in our inbox on a manageable level, we are closing issues that have been on the backlog for a long time but have not gained traction: We look at the number of votes the issue has received and the number of duplicate issues filed. If you disagree and feel that this issue is crucial: We are happy to listen and to reconsider. If you wonder what we are up to, please see our roadmap and issue reporting guidelines. Thanks for your understanding and happy coding! |
Out of scope since tasks is not an extension. |
Another cool feature would be to have link detection in the terminal so that users can navigate the output. Since link detection can be very specific to the command executed it might best come from the creator of a ITerminalInstance. An implementation idea would be:
The text was updated successfully, but these errors were encountered: