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

Add s/S/x/X actions, various synonyms, mode hooks, userspace docs; optionally differentiate w & e motions; fix process hooks & redo on Mac; automate updating firmware size table in readme #1

Merged
merged 19 commits into from
Jun 21, 2021

Conversation

t33chong
Copy link
Contributor

@t33chong t33chong commented Jun 9, 2021

This is by far the most comprehensive and well-documented implementation of Vim keybindings for QMK that I've found - thanks a ton for open sourcing it!

I've been integrating it into my personal keymap over the past few days, and along the way I've added some functionality that I thought you and other users might be able to make use of as well:

Actions

  • Added normal mode s - change the character to the right of the cursor.
  • Added normal mode S - change the line (synonym for cc).
  • Added visual and visual line mode s - change selection (synonym for c).
  • Added visual and visual line mode x - delete selection (synonym for d).
  • Added normal mode X - delete the character to the left of the cursor.
  • Added W, E, and B synonyms for their lowercase equivalents, as well as ^ for 0.
  • Fixed redo action on Mac - this was originally mapped to LCMD(KC_Y), but as far as I can tell, LCMD(LSFT(KC_Z)) is more commonly used.

Motions

  • Changed w motion to skip to beginning of next word by sending VMOTION(KC_RIGHT), then tapping VMOTION(KC_RIGHT), VMOTION(KC_LEFT) on release if enabled with VIM_W_BEGINNING_OF_WORD macro.

User hooks

  • Fixed existing process_normal_mode_user and process_visual_mode_user hooks - they weren't actually being called by their corresponding non-_user functions like process_insert_mode_user was.
  • Added process_visual_line_mode_user hook, as keybindings defined in process_visual_mode_user were not being processed in visual line mode.
  • Added insert_mode_user, normal_mode_user, visual_mode_user, and visual_line_mode_user hooks, which are called upon entering the relevant mode, to allow users to set custom state like RGB lighting layer indicators (see my keymap for example use cases).

Readme

  • Updated docs to reflect the aforementioned changes.
  • Added instructions for configuration at the userspace level.
  • Added a Python script + Dockerfile that contributors can use to update the table of firmware sizes for each feature macro automatically.

Caveat: I haven't written a whole lot of C, so there might be better ways to accomplish some of the things I've added here. Please scrutinize, be picky and don't hold back - your feedback will keep this codebase excellent and make me a better C programmer!

It's also entirely possible that I may have overlooked certain careful considerations you've made and introduced unexpected behavior; for example, I noticed that the readme explicitly mentioned that the x action had only been implemented in normal mode. If there was a good reason you originally left it out of the visual modes, and I've unwittingly broken something, please let me know! Similarly, I only tested this on macOS, so I can't confirm whether my changes to the various actions and motions work on other operating systems.

Finally, if there's anything here that doesn't fit with your vision for the project, or that you think might not have broad appeal among potential users, I'm more than happy either to put it behind an optional feature flag (i.e. a #define in config.h) or to move it to my personal keymap/userspace. One thing I suspect might fit into this category is my change to the w motion, since the sequence of motions it's composed of adds a bit of cursor flicker (at least on my system), which admittedly could be distracting.

@andrewjrae andrewjrae added the enhancement New feature or request label Jun 9, 2021
@andrewjrae
Copy link
Owner

Nice! This sounds very good, I'm happy that Mac support is being tested and fixed.

I don't have much time right now to properly review but I'll try to take a look this weekend.

Thanks for the contribution!

@t33chong t33chong changed the title Add s/S/x actions, mode hooks, userspace docs; differentiate w & e motions; fix process hooks & redo on Mac Add s/S/x actions, various synonyms, mode hooks, userspace docs; differentiate w & e motions; fix process hooks & redo on Mac Jun 10, 2021
Copy link
Owner

@andrewjrae andrewjrae left a comment

Choose a reason for hiding this comment

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

Looks pretty good all around! Only thing to fix is some repercussions of the new w, see the large comment on the code for more on that.

The only other thing I'd mention is that I was avoiding putting in x other modes since I personally don't use them there, and I've been trying to keep compile size down as much as possible. New entries in a switch statement add up faster than you might expect. Same logic goes for s and S, I just don't tend to use them anymore now that I use vim snipe.

That all being said, I think the size currently required is already so big that the extra 200 bytes aren't the biggest deal, and I shouldn't be expecting everyone to be following the same vim habits as me. I also want to stop having so much ifdef'd code, as it just gets very messy very fast. So for now I think adding features like this don't need ifdef options, but I would like to come up with a way to be able to configure what features are enabled with a much higher granularity. It would probably take some hefty macro magic, and maybe some scripting, so I've been avoiding it.

Anyways, that whole rant to say that if you want to, it would be good to update the feature table's firmware sizes. However, I've been thinking about writing a script to do that as it is rather tedious, so don't sweat it if you don't want to.

src/actions.h Outdated
@@ -18,7 +20,7 @@
// Other commands
#define VIM_PASTE VCMD(KC_V)
#define VIM_UNDO VCMD(KC_Z)
#define VIM_REDO VCMD(KC_Y)
#define VIM_REDO VCMD(VIM_REDO_KEYCODE)
Copy link
Owner

Choose a reason for hiding this comment

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

This might be a little confusing as there is now a VIM_REDO_KEYCODE and a VIM_REDO which is also a keycode. Honestly, I would be fine with just moving up the VIM_REDO define to the ifdef above.

register_motion(qk_mods | VIM_W, record);
if (record->event.pressed) {
register_code16(qk_mods | VIM_W);
} else {
Copy link
Owner

@andrewjrae andrewjrae Jun 12, 2021

Choose a reason for hiding this comment

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

This is generally a good solution to the holding problem, but it makes viw act like vaw. This is because the object action finishes on the press, returning to visual mode, then w is released and the selection moves to the start of the next word.

The other issue I noticed is that cw, dw etc. act like ce, since it also finishes it's action on press.

There's two simple solutions to these issues, you could make actions finish on release, which should be okay, the delay is probably not terribly noticeable. However, it allows for a strange ability to hold any of the motion keys during an action, ie dj down multiple lines and delete on release. But maybe that's a feature not a bug hehe, I don't think anyone would accidentally hold down long enough to delete more than they intended.

The other solution is to just make all the motions aside from hjkl not hold-able. I personally never hold w, b, or e, but perhaps that's a more important feature for other people. Frankly, I could go either way, but I think I'm leaning towards making them just be tapping keys, as I think it's generally easier to develop with and should break less stuff (you'd probably have to patch dot repeat if you made actions go on release).

After thinking some more, I think you should probably go for the no hold approach, unless you really think we should support holding w. If you do think it's greatly important it can be held, just make sure all the other features are happy, you'll need to make dot repeat act on release at the very least.

EDIT:
I've given this some more thought, and decided that being able to hold w is somewhat important since we can't do f or t motions. I think for now, simply adding the better w motion as a macro option is the way to go (with a warning that some things break). And down the line we can take a look at making things work on release (or if you have the time you can do it now, but my gut feeling is that I'd prefer it in a different PR since this one would get pretty big.

@andrewjrae
Copy link
Owner

I've had second thoughts on making w non-holdable as it is the fastest way to get around lines since we lack f and t motions. See the updated comment for more.

@t33chong
Copy link
Contributor Author

@andrewjrae Thanks for the feedback! I've removed the VIM_REDO_KEYCODE macro definition in favor of defining VIM_REDO separately under both conditions of the #ifdef VIM_FOR_MAC clause as you suggested. I've also retained the original w motion and required the user to define the VIM_W_BEGINNING_OF_WORD macro in order to enable the new w motion - happy to change the name if you can think of a clearer one. I agree with your assessment that being able to hold w is important since it facilitates quick navigation!

Besides this, some new unrelated things have been added, with the readme and original PR comment updated accordingly:

  • X action (backspace)
  • process_visual_line_mode_user hook

Updating the readme with the new firmware sizes is on my to do list for tomorrow, but I wanted to get these changes pushed first!

I'm getting a bit too sleepy at the moment to explain my initial attempt at implementing the second w solution that you originally suggested (and the issues I was in the middle of debugging before changing course), but in case you're curious, you can see the discarded commit here.

@t33chong t33chong changed the title Add s/S/x actions, various synonyms, mode hooks, userspace docs; differentiate w & e motions; fix process hooks & redo on Mac Add s/S/x/X actions, various synonyms, mode hooks, userspace docs; optionally differentiate w & e motions; fix process hooks & redo on Mac Jun 16, 2021
@t33chong
Copy link
Contributor Author

@andrewjrae I just updated the readme with the new firmware sizes, and added a Python script + Dockerfile that contributors can use to automate this in the future. I wasn't sure how you arrived at the figure of 1842 B, so I removed the sentence immediately following the table, but I can add it back if you'd like!

@t33chong t33chong changed the title Add s/S/x/X actions, various synonyms, mode hooks, userspace docs; optionally differentiate w & e motions; fix process hooks & redo on Mac Add s/S/x/X actions, various synonyms, mode hooks, userspace docs; optionally differentiate w & e motions; fix process hooks & redo on Mac; automate updating firmware size table in readme Jun 21, 2021
Copy link
Owner

@andrewjrae andrewjrae left a comment

Choose a reason for hiding this comment

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

The docker / python scripting is super useful, thanks so much for setting that up @t33chong! Everything else looks good to go too, so I'll go ahead and merge this.

Thanks again for contributing!

@andrewjrae andrewjrae merged commit aeaf460 into andrewjrae:master Jun 21, 2021
@t33chong
Copy link
Contributor Author

Awesome, happy to help make future feature development easier! Likewise, thank you again for this project - it was the last missing piece of the puzzle for me, and I can now throw away my mouse 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants