-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
fix multi line in 'at' and 'it' commands #1454
Conversation
const stack : any = []; | ||
const matchedTags : any = []; | ||
|
||
tags.forEach((tag) => { |
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 just use a simple for of
loop here?
// We now know where the opening tag is | ||
this.openStart = opening; | ||
this.openEnd = close + 1; | ||
const stack : any = []; |
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 give the proper typings here, rather than using any
? I think it's { name: string, type: "close" | "open", startPos: number, endPos: number }[]
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.
(And our code formatting conventions are const stack: type
)
Overall looks good to me. My biggest concern is that we have to parse the entire document every time we do a cit. How is the perf of that in a file such as actions.ts? (And you thought that file was huge for no reason. Of course not, it's to test perf concerns! 😉 ) |
@johnfn Hah... The performance was a concern of mine too, but I did some tests in very large html files (170,000+ characters) and saw no noticeable delay. |
@jrenton I think there was one other comment about using a for of loop, it is in our coding style guide This is so close that I would like to see it in the next release! |
Is there anything that I could do to help expedite this fix getting merged? |
@theamazingfedex There are some underlying bugs within VSCodeVim itself outside of this issue relating to assumptions made in regards to changing/deleting text (at least the last I checked). They show themselves through various bugs that still exist with this, including:
Technically this should be able to be merged, but I kind of gave up when I kept running into bugs that were related to underlying issues with the plugin itself. |
So just to be clear, the underlying algorithm for finding the corresponding tags works fine, the issues are mainly related to where the cursor ends up after? |
@Chillee Yes, mostly 😄 |
Do you think they're fixable (just a massive pain), or is there some fundamental limitation with our current code structure? Looking over your code for a bit, I think I know how to fix all 3 of the bugs you named, at least. They're kind of pain, but I think it's possible. |
There aren't any fundamental limitations to getting tag text objects to work. It's just more work. :) |
Further work on tag matching (based off of #1454)
Fixes #971, #1108, #1232, and #1300.
Remaining existing issues:
cit
does not enter insert mode when tag is empty, and it also doesn't return to the correct position when you runcit
in the closing tag.