Skip to content

added "keystroke a: insert after current position" action#712

Merged
jeremypw merged 8 commits intoelementary:masterfrom
clobrano-forks:issue614/vim-emulation-a-keystroke
Apr 24, 2020
Merged

added "keystroke a: insert after current position" action#712
jeremypw merged 8 commits intoelementary:masterfrom
clobrano-forks:issue614/vim-emulation-a-keystroke

Conversation

@clobrano
Copy link
Contributor

@clobrano clobrano commented Oct 3, 2019

Support for keystroke "a", which VIM maps into "insert after current position", was missing from the handle_key_press function.

Fixed adding the case using the Gdk.Key.i as example.

closes #614

Support for keystroke "a", which maps into "insert after current position" was
missing from handle_key_press function.

Fixed adding the case using the Gdk.Key.i as example.

closes elementary#614
@cassidyjames cassidyjames requested a review from a team October 3, 2019 18:51
mode = Mode.INSERT;
view.move_cursor (Gtk.MovementStep.VISUAL_POSITIONS, 1, false);
debug ("Vim Emulation: INSERT Mode!");
return true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this not implemented further down (where handling of Gdk.Key.A is implemented?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seemed to me that the cases where ordered alphabetically, so I applied the patch for Gdk.Key.a at the top (also Gdk.Key.i and Gdk.Key.I are not close)

Copy link
Collaborator

Choose a reason for hiding this comment

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

But Gdk.Key.A is processed in a different switch statement. The first one is related to setting the correct mode - not actions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I got it now. I'll update the patch

Previously, code was in a "change mode" section, while "a" keystroke is
also a movement action, so moved closed to the "A" keystroke
implementation (code review).
@jeremypw
Copy link
Collaborator

I am not a Vim user so could I confirm that the expected action is to start inserting text after the character that is on the right of the cursor? The effect of pressing <Esc>a is to move the cursor one character to the right (and then enter INSERT mode).

@clobrano
Copy link
Contributor Author

Uhm, I was double checking the documentation to confirm, but while the usual use case is indeed "start inserting text after 1 character", it is possible to prefix the command with a number N, then the effect is to start inserting text after N characters.
If @mcclurgm's implementation already has this feature fully implemented, then better keep his PR (patched as discussed) and just reject this.

@jeremypw
Copy link
Collaborator

As far as I can tell, @mcclurgm 's implementation of the a command is the same as yours. That PR has the disadvantage of changing several things at once and has not passed CI. I am inclined to approve yours and then the other PR can be update and preferably split into smaller PRs.

jeremypw
jeremypw previously approved these changes Oct 15, 2019
@clobrano
Copy link
Contributor Author

I see, could you give me some more time to see if a complete implementation is already doable?

Allow to start editing after [count] character, being [count] a number typed
just after pressing the "a" key.
@clobrano
Copy link
Contributor Author

It is possible :)

@mcclurgm
Copy link
Contributor

Great! I'll remove this change from my PR then.
@jeremypw would you suggest that I split it up into individual command changes or should I batch them by subject somehow? I don't know if it would be better for you to review a lot of tiny changes or a couple larger ones.

@jeremypw
Copy link
Collaborator

@mcclurgm It is better to split into the smallest PRs that make sense to merge alone. Small focussed "bitesize" PRs are likely to get reviewed and merged much more quickly.

@mcclurgm
Copy link
Contributor

Alright, I'll start working on that. Thanks for the suggestion!

@jeremypw
Copy link
Collaborator

@mcclurgm Are you making any progress on splitting this?

@mcclurgm
Copy link
Contributor

Not really sorry, I got a little stuck and then school started again. I'll try to get back on it. I did get one simple PR #737.

@mcclurgm
Copy link
Contributor

@clobrano are you sure that adding a number before a inserts after N characters? According to the docs I found, it inserts the text after one character, N times. I tried this in vim itself and that's what it did.

@clobrano
Copy link
Contributor Author

clobrano commented Mar 20, 2020

I think you're right, I don't know why I was convinced it worked differently. However, this makes things way harder than expected, using a number code should

  • take into a different buffer any sequence typed after a until <esc> is pressed
  • copied the buffer N times
  • go back to normal mode

Do we want the full feature for a keystroke? Honestly, I've never used this variant.

@mcclurgm
Copy link
Contributor

I think that's correct. I think this may be doable using the action string, which is used to implement the . command.

It looks like this sort of behavior isn't accounted for in the code at all though. It would need some design to figure out how to make it reuse the buffer N times though, I'm not sure how that would fit in with the current code. Maybe we could make it check for a number when you press escape?

@clobrano
Copy link
Contributor Author

clobrano commented Mar 20, 2020

I should look into this again, I don't even have Elementary installed anymore, sorry.

Maybe we could make it check for a number when you press escape?

I can't follow, why checking for a number?

@mcclurgm
Copy link
Contributor

Oh--sorry. When I said check for a number, I was thinking of the number variable. When we start inserting it should contain the number of times to do that insert, so when we leave insert mode it should tell us what to do. Unless there's something I'm missing. Does that make sense?

@clobrano
Copy link
Contributor Author

Yes, you are right, basically replying the last buffer for N-1 times, since the first sequence is already written.
I'll look into the action string, then, but I can't be sure when, sorry.

@mcclurgm
Copy link
Contributor

Maybe it makes sense to merge this as is, and then deal with the multiple-insert issue later once all the different insert options are in the codebase?

@clobrano
Copy link
Contributor Author

It might sounds lazy, but I agree. At least the basic feature would be in and it's quite used

@cassidyjames cassidyjames requested a review from jeremypw April 22, 2020 23:08
Copy link
Collaborator

@jeremypw jeremypw left a comment

Choose a reason for hiding this comment

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

No adverse effects found. <Esc>Na where N is a number moves the cursor N places to the right and enters insert mode. (Omission of N same as N = 1).

@jeremypw jeremypw merged commit b492e4a into elementary:master Apr 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Vim emulation does not parse keystroke "a"

4 participants