-
Notifications
You must be signed in to change notification settings - Fork 28.7k
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
Adding line and column support for terminal #24832
Adding line and column support for terminal #24832
Conversation
@timbanaveen, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Tyriar to be a potential reviewer. |
@timbanaveen, |
Hi @Tyriar, I have added code for adding support of line and column number in terminal. Please review. |
Hi @Tyriar I have added tests, Please review. Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @timbanaveen, thanks for looking into this 😃
private _winLineAndColumnMatchIndex = 12; | ||
private _unixLineAndColumnMatchIndex = 15; | ||
|
||
private _lineAndColumnClauses = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason this isn't a constant up the top so it's shared between multiple instances of TerminalLinkHandler
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be common for all instances, will move it to top.
testLink('c:/foo:5:3', 'c:/foo', '5', '3'); | ||
testLink('c:/foo:line 5', 'c:/foo', '5'); | ||
testLink('c:/foo:line 5, column 3', 'c:/foo', '5', '3'); | ||
testLink('c:/foo(5)', 'c:/foo', '5'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to have example programs that output each format (and only add them if we have an example).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice suggestion 👍 , will program that.
fragment: Uri.parse(normalizedPath).fragment | ||
}); | ||
|
||
return this._openerService.open(resource); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this._editorService.openEditor
ignore the line/col?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I first tried with openEditor, but it didn't worked. Hence used open service.
|
||
let lineAndColumnMatchIndex = this._winLineAndColumnMatchIndex; | ||
|
||
if (platform.Platform.Windows !== this._platform) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you swap the if? Normally we put the constant expression on the right: this._platform !== platform.Platform.Windows
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, will do that
} | ||
|
||
for (let i = 0; i < lineColumnClauseLength; i++) { | ||
let lineMatchIndex = lineAndColumnMatchIndex + 6 * i; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's 6?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while adding line and column regex, i have appended all regex clauses and each clause have 6 groups hence, after performing regex line and column information from each clause will be present at distance of 6. I think it would be better to have this value as constant up at top.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 to constant
return this._xterm.registerLinkMatcher(this._localLinkRegex, wrappedHandler, { | ||
matchIndex: 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Match index 1 only matches first group of regex, but we need other groups as well which have line and column number information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But matchIndex
is still generally supported right? Custom link matchers need to be able to specify it still.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Tyriar , Yes for custom link matchers MatchIndex is supported, it will follow given matchIndex.
const resource = Uri.file(path.normalize(path.resolve(resolvedLink))); | ||
return this._editorService.openEditor({ resource }).then(() => void 0); | ||
|
||
let normalizedPath = path.normalize(path.resolve(resolvedLink)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be pulled into a formatLocalLinkPath
or similar named function that returns a URI in form <path>#line,col
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that will be better will do that
Related: #24451 |
739f245
to
325d9a7
Compare
Hi @Tyriar , I have added suggested changes, Please review Thanks |
Thanks @timbanaveen, I'll give this a proper look after I've caught up on my issues (just took a week off so I'm pretty backed up). |
…mn-support-terminal
@timbanaveen this is awesome, I made a few minor changes and merging after I make sure tests are all good 🎉 This should make it into the April release (1.12) unless any big issues are found when we test. |
@Tyriar Thanks for the help 🙇♂️ , learned many things from code review and minor changes that you did. |
@timbanaveen people love it btw https://twitter.com/Tyriar/status/856616415909904384 😃 No major issues found during test #25331 |
@Tyriar , Cool it is 😄 , Thanks. Can i share this on Facebook ? |
@timbanaveen go for it 😄 |
Fixes #21472