Skip to content
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

Make <delete> repeatable in Normal Mode. Fix #394 #514

Merged
merged 3 commits into from
Jul 29, 2016
Merged

Make <delete> repeatable in Normal Mode. Fix #394 #514

merged 3 commits into from
Jul 29, 2016

Conversation

octref
Copy link
Contributor

@octref octref commented Jul 25, 2016

Yay! We love PRs! 🎊

Please include a description of your change & check your PR against this list, thanks:

  • Commit message has a short title & issue references
  • Commits are squashed
  • It builds and tests pass (e.g gulp tslint)

Previously <delete> key works in Normal Mode but is not repeatable.
This PR makes it repeatable, so commands like 5<delete> would work like 5x.

@octref
Copy link
Contributor Author

octref commented Jul 25, 2016

BTW, there are a few unrelated tests failing, but they also fail on the master branch without my changes. So I ignored them.

@@ -1960,6 +1960,11 @@ class ActionDeleteChar extends BaseCommand {
}

@RegisterAction
class ActionDeleteCharWithDeleteKey extends ActionDeleteChar {
keys = ["<delete>"];
}
Copy link
Member

@rebornix rebornix Jul 26, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a difference between delete and x is the way they handle numeric prefix. Nx will perform x N times but N<Del> does nothing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought N<Del> to delete the next 5 chars should be the expected behavior.

So, in Normal Mode, is this

<Del> -> removes next char
N<Del> -> do nothing

what I should do instead?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, that's Vim's behavior and it's awkward, but let's do this.

@@ -61,13 +68,20 @@ suite("Mode Normal", () => {
});

newTest({
title: "Can handle x at end of line",
title: "Can handle 'x' at end of line",
start: ['one tw|o'],
keysPressed: '^llxxxxxxxxx',
end: ['|'],
});

newTest({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also add a case for N<delete> as the behavior is strange in Vim.

@johnfn
Copy link
Member

johnfn commented Jul 26, 2016

Fix up my one suggestion and we should be good to go here! Thanks for the PR, @octref!

@johnfn
Copy link
Member

johnfn commented Jul 26, 2016

Ah, I'm sorry for doing this retroactively, but you caught us in the middle of a refactor.

Would you mind fixing this up by making both delete classes extend from an abstract base class rather than using inheritance? I'm trying to move away from big inheritance trees; see #519.

@octref
Copy link
Contributor Author

octref commented Jul 26, 2016

No worries. Refactoring is good.
In work now. Will do that tonight.

On Tue, Jul 26, 2016 at 10:21 AM, Grant Mathews notifications@github.com
wrote:

Ah, I'm sorry for doing this retroactively, but you caught us in the
middle of a refactor.

Would you mind fixing this up by making both delete classes extend from an
abstract base class rather than using inheritance? I'm trying to move away
from big inheritance trees; see #519
#519.


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

@octref
Copy link
Contributor Author

octref commented Jul 29, 2016

@johnfn
Done. Sorry for the delay. Had some company events for the last few days.

@johnfn johnfn merged commit dfac789 into VSCodeVim:master Jul 29, 2016
@johnfn
Copy link
Member

johnfn commented Jul 29, 2016

LGTM. Thanks, @octref!

@octref octref deleted the MakeDeleteKeyRepeatable branch July 30, 2016 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants