-
Notifications
You must be signed in to change notification settings - Fork 752
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
VIM-1341 implement gx command #773
Conversation
Hi there! Great, this is a long-awaited feature. I've added a couple of comments. |
I just saw this. Edit: I'm having trouble finding the comments. I'll check back again in a bit. |
why 'gx' open in browser not work? |
Hi @GhengS, This feature has not yet been merged with the IdeaVim plugin. |
@AlexPl292 I can't find the comments. Would you mind checking the submit review button? I'm guessing because I've made this mistake often with gh's ui. |
operatorArguments: OperatorArguments, | ||
): Boolean { | ||
val wordUnderCursor = exactWordUnderCursor(editor); | ||
logger.info("word: $wordUnderCursor") |
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.
Logging is always great, but we try not to log the raw text from user input. The problem is that this text may have sensitive information like passwords and we don't want to leak it into the log.
I see several cases here, it's better to clean them up.
): Boolean { | ||
val wordUnderCursor = exactWordUnderCursor(editor); | ||
logger.info("word: $wordUnderCursor") | ||
if(!isValidUrl(wordUnderCursor)){ |
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'm thinking about the logic behind links search.
Generally, it looks valid to me, but IJ also has a feature to navigate to issues in Jira or youtrack by issue id. Like // Here is my issue: VIM-123
. This, of course, won't be supported by this logic.
I actually found a fancy util in the platform: IssueNavigationConfiguration.getInstance(project).findIssueLinks(commentText)
. You can feed it a bunch of text and it will return resolved links (Including transformation from VIM-123
to https://youtrack.com/issues/VIM-123
).
I think, it will be better to use this util. This, however, will make the code platform dependent, and we'll have two options here: move this GotoUrlAction
out from vim-engine
or make an abstraction via VimInjector
. The second option would require to add a new function to VimInjector
like VimApplication.resolveLinks
and implement it via the mentioned util in IjVimInjector
.
Oh, I'm really sorry, my eyes were ignoring the |
So I see your comments and have onced over how to address them. I'll put some time into this when I'm not under deadline pressure at my day job. |
@@ -23,7 +23,7 @@ repositories { | |||
dependencies { | |||
compileOnly("com.google.devtools.ksp:symbol-processing-api:1.9.21-1.0.15") | |||
implementation("org.jetbrains.kotlinx:kotlinx-serialization-json-jvm:$kotlinxSerializationVersion") { | |||
// kotlin stdlib is provided by IJ, so there is no need to include it into the distribution | |||
// kotlin stdlib is provided by IJ, so there is no need to include it into the |
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.
Accidental change?
Hi, this PR has been open for some time. Do you have plans to update it? |
@AlexPl292 I would like to do this however it's gonna be a long time to implement this the proper way as Im new to text editors. I think there's a couple options here.
Btw if you have some suggested reading for how to get up to speed quickly I'd gladly read it. |
Hi, I see this PR is almost a year old. I'll close it, but feel free to reopen it in case you'll restart the work on it! |
If anything is not right here please let me know so I can fix it.
I absolutely love this plugin.