Implement quoted text objects

[ascandella]

Fixes #471, #287

[ascandella]

I feel a bit bad (but not too bad) about copy-pasting the body of these loops. Really what I want is to have some thing that takes an iterator and returns Either<position **or** notfound> and then pass that iterator to the loops. But, I'm a total TypeScript noob and I didn't see an obvious way to do it.

[rebornix]

I'm a little worried about the escape char, this can't handle \\", right?

[ascandella]

Correct, that is a limitation.

I looked at the algorithm used by other vim emulation stacks and this seemed to be the best one. I think we could handle that case by having another look-behind for another escape character? Or are there other pathological cases we'd still be missing?

[rebornix]

We have handled escape char while parsing subsitute commands https://github.com/VSCodeVim/Vim/blob/master/src/cmd_line/subparsers/substitute.ts . The logic is reading char one by one and handling every \. I suppose we can do the same thing here?

For now I don't see any other case but I'll investigate.

[rebornix]

Everything looks good to me except the \ escaping, it won't be too complex to handle as we already did that in substitute parsing :)

[ascandella]

@rebornix updated with handling of escaped backslashes

[rebornix]

@sectioneight I'm really sorry that I didn't think carefully about the escaping before I made suggestions. Checking double \ before " is somewhat not perfect, for example "abc\\\"def" will pass the validation

currentChar === char &&
(corpus[i - 1] !== this.escapeChar || corpus[i - 2] === this.escapeChar)

but it's not a true closing ". Now it turns out to be a text lexing problem, remind me of old school days...

[ascandella]

Ah, right, of course it's not that easy :)

What do you guys think? Should we parse the whole line or stick with the
simpler (with known limitations) version?
On Wed, Jul 20, 2016 at 7:23 AM Peng Lyu [email protected] wrote:

@sectioneight https://github.com/sectioneight I'm really sorry that I
didn't think carefully about the escaping before I made suggestions.
Checking double \ before " is somewhat not perfect, for example
"abc\"def" will pass the validation

currentChar === char &&
(corpus[i - 1] !== this.escapeChar || corpus[i - 2] === this.escapeChar)

but it's not a true closing ". Now it turns out to be a text lexing
problem, remind me of old school days...

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#483 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAF7P_es3k0heZUEREix08jSpRxb-gLVks5qXi95gaJpZM4JQYQq
.

[rebornix]

I have a dirty but quick way to handle this, while iterating the text, skip all escaped chars then check whether it's a "

let i = 0;
while (i < text.length) {
  if (text[i] === "\") {
    i = i+2;
  }

  if (text[i] == "\"") {
    return i;
  }
}

I didn't handle corner cases carefully but I think you can get the idea.

Besides, I don't have a strong opinion about postpoing the escaping handling or not, if we don't have a perfect solution right now, we can fix it later. It's up to you and @johnfn .

[ascandella]

OK, just pushed a new commit that pre-parses the line and handles your edge case properly. I'm not sure about performance implications, however -- is that something to be concerned about, or is that prematurely optimizing?

[ascandella]

This one is ready to go

[rebornix]

seems this.corpus is never used?

[rebornix]

only have some tiny suggestions. The logic and code look good to me so I'm willing to see it merged.

[ascandella]

@rebornix great feedback, updated.

[johnfn]

Can you use position objects and getRight()/getLeft() here?

[ascandella]

For sure.

[johnfn]

The one big thing that we're missing here is i" in a situation like this:

"one " two |"three" (where | is the start of the block cursor)

You currently select the inner " two " rather than the "three" to the right. I'm pretty sure that Vim actually determines which of the two potential quote sections is inside the quotes rather than outside and selects that one. Could you add in that case?

This should be good to merge with that.

[ascandella]

Sounds good. Master tests are currently broken, I'm going to wait until that's resolved to push anything else here.

[ascandella]

@johnfn updated. Note that there are test cases that currently fail, as master is currently red.

[johnfn]

Perfect! Thanks for the awesome work, @sectioneight!