Implement additional text object commands #450

ascandella · 2016-07-15T06:06:15Z

This allows for e.g. 'ci(' and 'da<' commands.

Fixes #449

ascandella · 2016-07-15T06:07:09Z

To be clear, any and all feedback, no matter how minor or how major, is appreciated. I'd like to continue to contribute to the project, and I'd like to write idiomatic TypeScript, but I'm nearly certain this code is not optimal as-is.

ascandella · 2016-07-15T06:08:16Z

src/actions/actions.ts

          stackHeight++;
        }

-        if (char === this.pairings[charToMatch].match) {
+        if (char === toFind.match) {


All of the callers of this function (this.nextBracket) are already passing an object from this.pairings[char] as the toFind, so I think this is more correct. This also allows callers of this function to use characters that don't exist in this.pairings, such as " and '

Good catch! That must have been vestigial.

I would also suggest adding ' and " back to our pairings list, perhaps adding a flag so that we don't match them with %.

This pairings bothers me a lot when I implement MoveToUnclosedRoundBracketBackward. And the first hack I did is moving the bracket matching to a separate method called nextBracket in b0a2038 but now it seems nextBracket is no longer a good name as we might match quotes. Something called nextPairedChar looks better to me.

Besides, your PR is good enough for this fix but you may want to take a look at all text objects commands, maybe we can make this abstract class MoveToMatchingBase more generic?

Yes, I plan on making the class generic enough so that it can handle a<, i) etc (and may add them to this PR now that I have some better confidence I'm going down the right track).

Will come up with some better naming, and add '/" into pairings

johnfn · 2016-07-15T07:51:45Z

This is looking good so far, and I'm pretty hyped to have this functionality! 😀

The only thing I'd suggest to change would be how you use inheritance to share the matching functionality with MoveToMatchingBase. You could just extract the function out into a new file and our class hierarchy would become 1 less deep, which would be nice.

ascandella · 2016-07-15T15:48:33Z

Ok, I think I have most of what I need to get this PR to a merge-able state.

One open question that I left at the top: is there a way to return something to indicate that this didn't match, so that for example an unsuccessful ci" won't switch to insert mode?

ascandella · 2016-07-15T19:03:07Z

Needs a few more tests, but I think this is almost there.

johnfn · 2016-07-15T22:29:50Z

Ahh. Your question about a failed ci" is actually something I haven't quite got around to yet. For now could you just mark it with a TODO and return a range of length 0? It's not perfect, I admit.

johnfn · 2016-07-15T22:33:05Z

src/actions/actions.ts


  public async execAction(position: Position, vimState: VimState): Promise<Position> {
    const text = TextEditor.getLineAt(position).text;
    const charToMatch = text[position.character];
-    const toFind = this.pairings[charToMatch];
+    const toFind = PairMatcher.pairings[charToMatch];


What do you think about making these static methods rather than instance methods? There isn't really any associated state we need to carry along, is there?

Yea, I originally had it being stateful (where you initialized a PairMatcher with the character), but that seemed wasteful as well. Wasn't sure about preferences for static/non-static. Will change to static.

Yeah. I prefer static to indicate stateless, though unfortunately there are places where I don't adhere to that.

ascandella · 2016-07-15T22:54:39Z

By "return a range of length 0" do you mean an IMovement with the same start/stop? I tried that, but it still didn't "cancel" the insert/change.

johnfn · 2016-07-15T23:58:44Z

Yeah, there's no way to cancel, so we just return an empty selection. It's not great- we'll have to fix it later.

ascandella · 2016-07-16T00:14:55Z

Yea, I understand that there isn't currently a cancel. Maybe there's a bug in my code, but when I return a {start: pos, stop: pos} (which I think is what you meant), a ci" still deletes the first character.

rebornix · 2016-07-16T02:40:13Z

@sectioneight these commands were on my list before I worked on Window/Tab and CJK stuff. Good to see you can help on this, they are quite important.

ascandella · 2016-07-16T05:05:14Z

Disregard my previous comment, ci" without a match is not deleting any characters (only changing to insert mode, which is sub-par but not a dealbreaker)

ascandella · 2016-07-16T05:23:41Z

NOTE: There is one limitation to this that I just discovered.

If you have:

"foo"

and you have your cursor on the closing " mark, and type e.g. ci", it won't recognize the match while vim will.

I don't consider it a blocker, but if there's an obvious way to fix this I'd be open to doing it now.

johnfn · 2016-07-16T06:46:12Z

Ahh, yes. When I initially said that i" and i' would be harder, this was my concern. I'm not convinced there's an easier way than parsing the entire line.

(Oops, fat fingered the close button!)

rebornix · 2016-07-16T08:04:22Z

We are using inclusive parameter quite often in Position, I suppose that's also what we need for this generic function.

I'd like to see this PR merged as I don't want to see it Open for too long. But just make sure we don't release this feature without a fix, people are likely to run i" while the cursor is on a ".

ascandella · 2016-07-16T15:59:52Z

To be clear, the limitation is only on the closing quote -- ci" on an opening quote behaves as expected, as does ci( on the opening or closing parens, as well as inside. Because of the observed behavior, I'm pretty certain it's due to nextPairedChar not correctly handling the closing and opening chars being the same.

ascandella · 2016-07-16T16:11:29Z

My current thinking: handling quotes is going to take some more work, and I want to balance making forward progress with not introducing buggy code. I'm going to remove the single- and double-quote functionality from this PR and work out the proper algorithm to handle them (since it's really a different problem than handling the rest).

That way we can (hopefully) get this PR merged this weekend and I'll spend some more time working through the edge cases of properly handling quotes.

ascandella · 2016-07-16T16:21:06Z

Ok, PR updated, I think the functionality contained is complete (although I marked it as "partially supported" on the roadmap until at least one other person tries it out and says it works).

This allows for e.g. 'ci(', 'ca"', 'di<' commands.

johnfn · 2016-07-16T17:48:04Z

Awesome work! This looks good to me.

ascandella reviewed Jul 15, 2016
View reviewed changes

ascandella changed the title ~~Implement inside-paren and inside-doublequote movement~~ [WIP] Implement inside-paren and inside-doublequote movement Jul 15, 2016

ascandella force-pushed the change-inside-word branch 4 times, most recently from 1ca5c4e to 5a50f21 Compare July 15, 2016 07:06

ascandella force-pushed the change-inside-word branch 2 times, most recently from 8f6fa0d to 80a876e Compare July 15, 2016 19:02

ascandella force-pushed the change-inside-word branch from 80a876e to b40fb99 Compare July 15, 2016 19:03

ascandella changed the title ~~[WIP] Implement inside-paren and inside-doublequote movement~~ [WIP] Implement additional text object commands Jul 15, 2016

ascandella force-pushed the change-inside-word branch from b40fb99 to 98ac31d Compare July 15, 2016 19:05

johnfn reviewed Jul 15, 2016
View reviewed changes

ascandella force-pushed the change-inside-word branch from 98ac31d to 0c44908 Compare July 15, 2016 22:57

ascandella force-pushed the change-inside-word branch from 0c44908 to 8f523d6 Compare July 16, 2016 00:22

ascandella changed the title ~~[WIP] Implement additional text object commands~~ Implement additional text object commands Jul 16, 2016

ascandella force-pushed the change-inside-word branch 2 times, most recently from 8bacbcd to f82f8c8 Compare July 16, 2016 00:37

ascandella force-pushed the change-inside-word branch 2 times, most recently from 2d02f18 to f051e79 Compare July 16, 2016 05:03

ascandella force-pushed the change-inside-word branch from f051e79 to 42c71e1 Compare July 16, 2016 05:08

johnfn closed this Jul 16, 2016

johnfn reopened this Jul 16, 2016

ascandella force-pushed the change-inside-word branch 2 times, most recently from fe239b4 to 58bd2b5 Compare July 16, 2016 17:04

ascandella added 2 commits July 16, 2016 10:16

Implement additional text object commands

96813bb

This allows for e.g. 'ci(', 'ca"', 'di<' commands.

Remove quote matching for now

5dd396c

ascandella force-pushed the change-inside-word branch from 58bd2b5 to 5dd396c Compare July 16, 2016 17:17

johnfn merged commit 910666d into VSCodeVim:master Jul 16, 2016

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement additional text object commands #450

Implement additional text object commands #450

ascandella commented Jul 15, 2016 •

edited

Loading

ascandella commented Jul 15, 2016

ascandella Jul 15, 2016

johnfn Jul 15, 2016

rebornix Jul 15, 2016

ascandella Jul 15, 2016

johnfn commented Jul 15, 2016

ascandella commented Jul 15, 2016

ascandella commented Jul 15, 2016

johnfn commented Jul 15, 2016

johnfn Jul 15, 2016

ascandella Jul 15, 2016

johnfn Jul 15, 2016

ascandella commented Jul 15, 2016

johnfn commented Jul 15, 2016

ascandella commented Jul 16, 2016

rebornix commented Jul 16, 2016 •

edited

Loading

ascandella commented Jul 16, 2016

ascandella commented Jul 16, 2016

johnfn commented Jul 16, 2016 •

edited

Loading

rebornix commented Jul 16, 2016

ascandella commented Jul 16, 2016 •

edited

Loading

ascandella commented Jul 16, 2016

ascandella commented Jul 16, 2016

johnfn commented Jul 16, 2016

Implement additional text object commands #450

Implement additional text object commands #450

Conversation

ascandella commented Jul 15, 2016 • edited Loading

ascandella commented Jul 15, 2016

ascandella Jul 15, 2016

Choose a reason for hiding this comment

johnfn Jul 15, 2016

Choose a reason for hiding this comment

rebornix Jul 15, 2016

Choose a reason for hiding this comment

ascandella Jul 15, 2016

Choose a reason for hiding this comment

johnfn commented Jul 15, 2016

ascandella commented Jul 15, 2016

ascandella commented Jul 15, 2016

johnfn commented Jul 15, 2016

johnfn Jul 15, 2016

Choose a reason for hiding this comment

ascandella Jul 15, 2016

Choose a reason for hiding this comment

johnfn Jul 15, 2016

Choose a reason for hiding this comment

ascandella commented Jul 15, 2016

johnfn commented Jul 15, 2016

ascandella commented Jul 16, 2016

rebornix commented Jul 16, 2016 • edited Loading

ascandella commented Jul 16, 2016

ascandella commented Jul 16, 2016

johnfn commented Jul 16, 2016 • edited Loading

rebornix commented Jul 16, 2016

ascandella commented Jul 16, 2016 • edited Loading

ascandella commented Jul 16, 2016

ascandella commented Jul 16, 2016

johnfn commented Jul 16, 2016

ascandella commented Jul 15, 2016 •

edited

Loading

rebornix commented Jul 16, 2016 •

edited

Loading

johnfn commented Jul 16, 2016 •

edited

Loading

ascandella commented Jul 16, 2016 •

edited

Loading