Implement increment and decrement operators #515

ascandella · 2016-07-25T22:52:18Z

This adds support for single or numeric-prefix control+x and control+a.

Numeric modes supported:

Decimal
Hex
Octal

Fixes #513

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

src/actions/actions.ts

+    }
+
+    // Advance to the end of the word in case it's longer
+    return Position.FromVSCodePosition(endPos.translate(0, newNum.length - oldWidth));


Not sure about this, feels weird to create an intermediate object just to move a character. Thoughts?

This adds support for single or numeric-prefix control+x and control+a.

rebornix · 2016-07-26T02:46:18Z

src/actions/actions.ts

+    const text = TextEditor.getLineAt(position).text;
+
+    // They may start on a number, so check the current word boundaries first.
+    let wordEnd = position.getCurrentWordEnd(true);


I'm worried about relying on word boundaries as users are allowed to customize word separators, see #487 . Will this be a problem?

Interesting, yes that could be a problem. Is there going to be a different API to get "builtin" word boundary matching? Should I just use a custom regex for this movement?

We can't use getWord as it always reads from user settings or default, I suppose want to want to get is something starts with numbers or + and -, a custom regex may do the trick.

Looks like all of the current Position methods that take regexp's are private -- do you think I should change the visibility, or subclass Position to something like NonCustomizablePosition that does not respect user settings for word boundaries?

If a user is insane enough to define numbers as word separators, these motions make no semantic sense (is 1234 4 separate words or one? If we select the whole thing with visual mode and do ctrl a, what should happen?)

I'm completely fine with this being undefined behavior.

Sounds good to me.

ascandella · 2016-07-26T04:09:29Z

Updated with a NumericString class to handle hex -- I have no idea if that's the right place to put it or if I should just make some private inner class (do those even exist in TS?)

ascandella · 2016-07-26T04:16:52Z

The only thing left that this PR does not implement (trying to understand how useful it is):

given

0x0

hitting ctrl+x

gives you

0xffffffffffffffff

johnfn · 2016-07-26T04:25:57Z

@sectioneight that seems of very little use. It doesn't make a lot of sense to guess how many bits are in the user's variable. 😛

ascandella · 2016-07-26T04:37:39Z

extension.ts

@@ -179,7 +179,7 @@ export async function activate(context: vscode.ExtensionContext) {
    showCmdLine("", modeHandlerToEditorIdentity[new EditorIdentity(vscode.window.activeTextEditor).toString()]);
  });

-  'rfbducw['.split('').forEach(key => {
+  'rfbducw[ax'.split('').forEach(key => {


is there any specific ordering to the strings here? should we alphabetize them? reverse-alphabetize? put our favorite ones first?

hah! there's no order, but yeah, this thing is pretty dumb. honestly i think we should parse them directly out of package.json and DRY this up, but that's a job for another day.

johnfn · 2016-07-26T05:38:47Z

src/number/numericString.ts

@@ -0,0 +1,36 @@
+export class NumericString {


I think you could actually write this whole thing very concisely (less than 10 lines) with 3 separate regexes. Just do grouped matches to grab the values and then send them through parseInt. Something like this:

8: /^0([0-7]+),

10: /^(\d+)/,

16: /0x([\da-fA-F]+)/

}```

Good call, updated with what I think you meant

Yeah, LGTM, though maybe I slightly exaggerated how short it was going to be.

johnfn · 2016-07-26T06:03:49Z

src/mode/modeHandler.ts

@@ -503,8 +503,6 @@ export class ModeHandler implements vscode.Disposable {
  }

  async handleKeyEvent(key: string): Promise<Boolean> {
-    if (key === "<c-r>") { key = "ctrl+r"; } // TODO - temporary hack for tests only!


Did someone fix this?

See https://github.com/VSCodeVim/Vim/pull/515/files#diff-bf5fd3f226adf3a0b4a4f0224184aa7fR165

johnfn · 2016-07-26T06:20:23Z

src/actions/actions.ts

+
+    for (let { start, end, word } of Position.IterateWords(position.getWordLeft(true))) {
+      // '-' doesn't count as a word, but is important to include in parsing the number
+      if (text[start.character - 1] === '-') {


Use word here rather than text, start and end.

I can't, because I'm testing outside of the returned word (since - isn't included in the word boundaries)

Oh geez, that actually seems like it could have some bad interactions with people who DONT use - as a word delimiter. It's also really annoying. Oh well, it's fine for now.

johnfn · 2016-07-26T06:27:24Z

Ok, sweet. Just remove the hack fix and we should be good to go.

ascandella · 2016-07-26T06:28:09Z

Remove which hack fix? I think I already undid the changes to tokenization.

johnfn · 2016-07-26T06:29:53Z

Yup, you're right, I must have been looking at an old version of the code.

Sweet. LGTM!

johnfn · 2016-07-26T06:31:39Z

(Thanks for putting up with all my crazy requests! 😁 )

ascandella · 2016-07-26T06:35:16Z

haha all good, I'm the same way on code reviews at work
On Mon, Jul 25, 2016 at 11:31 PM Grant Mathews [email protected]
wrote:

(Thanks for putting up with all my crazy requests! 😁 )

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#515 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAF7PwOcRzfyUespOaVu9jsA-RFYvqLbks5qZanNgaJpZM4JUoIG
.

ascandella reviewed Jul 25, 2016
View reviewed changes

ascandella force-pushed the control-a branch 2 times, most recently from 46adb9d to faa43d3 Compare July 26, 2016 01:52

Implement increment and decrement operators

3c46d78

This adds support for single or numeric-prefix control+x and control+a.

ascandella force-pushed the control-a branch from faa43d3 to 3c46d78 Compare July 26, 2016 02:44

rebornix reviewed Jul 26, 2016
View reviewed changes

Add numeric string helper

b89d6a0

Unit tests for numericString

2332b38

ascandella reviewed Jul 26, 2016
View reviewed changes

ascandella added 3 commits July 25, 2016 22:13

Handle octal

2a846d9

Integration test for octal

7d81a84

Guard control-a behind flag

5605169

johnfn reviewed Jul 26, 2016
View reviewed changes

Use a better algorithm

6bf1fa8

johnfn reviewed Jul 26, 2016
View reviewed changes

ascandella added 4 commits July 25, 2016 23:10

Extract IterateWords to Position

00abdaf

Use destructuring

3113a54

Handle negatives

62ae67f

Flag it up

bee4c45

Re-add the hacks, even hackier this time

7af50a4

johnfn reviewed Jul 26, 2016
View reviewed changes

Remove outdated comment

7ab4e6b

johnfn merged commit 00f094a into VSCodeVim:master Jul 26, 2016

ascandella deleted the control-a branch July 26, 2016 06:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement increment and decrement operators #515

Implement increment and decrement operators #515

ascandella commented Jul 25, 2016 •

edited

Loading

ascandella Jul 25, 2016

rebornix Jul 26, 2016

ascandella Jul 26, 2016

rebornix Jul 26, 2016 •

edited

Loading

ascandella Jul 26, 2016

johnfn Jul 26, 2016

ascandella Jul 26, 2016

ascandella commented Jul 26, 2016

ascandella commented Jul 26, 2016

johnfn commented Jul 26, 2016

ascandella Jul 26, 2016

johnfn Jul 26, 2016

johnfn Jul 26, 2016 •

edited

Loading

ascandella Jul 26, 2016

johnfn Jul 26, 2016

johnfn Jul 26, 2016

ascandella Jul 26, 2016

johnfn Jul 26, 2016

ascandella Jul 26, 2016

johnfn Jul 26, 2016

johnfn commented Jul 26, 2016

ascandella commented Jul 26, 2016

johnfn commented Jul 26, 2016

johnfn commented Jul 26, 2016

ascandella commented Jul 26, 2016

Implement increment and decrement operators #515

Implement increment and decrement operators #515

Conversation

ascandella commented Jul 25, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rebornix Jul 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ascandella commented Jul 26, 2016

ascandella commented Jul 26, 2016

johnfn commented Jul 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnfn Jul 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnfn commented Jul 26, 2016

ascandella commented Jul 26, 2016

johnfn commented Jul 26, 2016

johnfn commented Jul 26, 2016

ascandella commented Jul 26, 2016

ascandella commented Jul 25, 2016 •

edited

Loading

rebornix Jul 26, 2016 •

edited

Loading

johnfn Jul 26, 2016 •

edited

Loading