Add undo checkpoint command #2115

AntonioLucibello · 2022-04-15T09:33:22Z

adds command to manually commit undo state

AntonioLucibello · 2022-04-15T09:41:33Z

First time contributing to any open source project, I thought I was opening a pull request to my own fork (hence why I closed and reopened this pr in the span of 2 minutes)

EpocSquadron · 2022-04-15T11:30:47Z

helix-term/src/commands.rs

@@ -319,6 +319,7 @@ impl MappableCommand {
        redo, "Redo change",
        earlier, "Move backward in history",
        later, "Move forward in history",
+        add_undo_checkpoint, "Add undo checkpoint",


I feel like commit_undo_checkpoint might read slightly better.

or even commit_undo_state, but I figured calling it commit would be confusing if git integration was added in the future

I think diff and possibly blame are likely to end up in the core but any sort of git commit integration would probably live as a plugin. Since we already use "commit" when talking about history in the implementation I think commit_undo_checkpoint is reasonable

EpocSquadron · 2022-04-15T11:33:45Z

This is great, welcome to the fold!

Kakoune binds this in insert mode to Ctrl-u, which I think we should do as well. Can you add it to the default keybinds?

AntonioLucibello · 2022-04-15T12:28:03Z

Ctrl-u is already used for kill_to_line_start, should I map it to Ctrl-U? Or perhaps to Ctrl-c for "checkpoint" or "commit" (though to be fair that shortcut is very often used for copy)

AntonioLucibello · 2022-04-15T12:44:09Z

I've added it to the keybinds as Ctrl-U in helix-term\src\keymap\default.rs, inside the let insert = keymap!({...}) block, but it doesn't seem to be working. Do I have to add it in some other place too?

edit: nevermind, it seems to be a problem with uppercase characters since Ctrl-i works fine

should I go ahead and map it to Ctrl-c?

EpocSquadron · 2022-04-15T19:07:12Z

Personally I prefer maybe ctrl-s or maybe alt-u?

AntonioLucibello · 2022-04-15T19:26:57Z

I'm in favor of Alt-u: it's similar enough to Ctrl-u and has the added advantage that Alt can be pressed easily with the left thumb while typing, unlike Ctrl which requires Extreme Left Pinky Contortion™

Considering that this command would be pressed in insert mode, I like that Alt doesn't interrupt the flow of typing the same way Ctrl does

archseer · 2022-04-20T02:13:44Z

@pickfire opinions on this ^

I'd prefer to map the existing Ctrl-u to something else tbh and keep consistency with kakoune. Insert mode readline mappings were a mistake in my opinion, they don't fit with the helix/kakoune model.

pickfire · 2022-04-21T00:13:46Z

Hmm, for my own opinion I would be prefer to keep the C-u since I use it often, kakoune keys for insert mode isn't really that good (IMHO only their normal mode keys is good, insert mode keys is bad) that I think that we need to keep consistency with kakoune for insert mode related stuff, at least vim/readline keys for insert is better I think, so both A-u and C-s is better I think, if it is me I probably would have go with A-u.

One pain point when I started using kakoune is that I always have to exit the insert mode to be able to do quick modification, especially when I typo a word wrongly and want to quickly erase it, which I missed from vim where you can just C-u and C-w to do quick edit.

archseer · 2022-04-21T01:37:45Z

kakoune keys for insert mode isn't really that good

This is deliberate, there should be no movement in insert mode. mawww/kakoune#334 (comment)

archseer · 2022-04-21T01:40:02Z

I'm considering removing the readline mappings but keeping the commands. That way users can re-introduce the mappings in their configs but it makes it clear that these aren't the preferred mappings.

pickfire · 2022-04-21T03:09:35Z

This is deliberate, there should be no movement in insert mode. mawww/kakoune#334 (comment)

Yes, I have seen that when I started using kakoune, I still think they are useful, I had to manually specify that in kakoune and I ended up lazy to declare those (because I don't like tweaking config) so sometimes I still fallback to neovim when using kakoune because of it, I only use kakoune for multi-line edits mainly, because in insert mode, readline keys are still more powerful compared to kakoune insert mode which doesn't do anything useful.

I think these are the parts where helix can make up to the part where kakoune falls shore, one is this where it is missing useful keys in insert mode, the other one being having to hold shift when extending keys (where we use v).

But mainly I think what you said is correct, movement keys in insert mode isn't really that useful I think, like ctrl-f and ctrl-b which moves the cursor which can open up some other issues with undo or repeat, but I don't consider ctrl-u and ctrl-w movement keys given that they behave more like shortcut utilities to let you quickly delete words, if that is the case then arrow keys (do we remove the arrow keys?) and backspace are also considered movement keys, and I feel like they are in the same category, but yeah, maybe we could move the ctrl-u and ctrl-w to ctrl-backspace and alt-backspace? But I still think the vim keys seemed good.

archseer · 2022-04-22T02:14:01Z

movement keys in insert mode isn't really that useful I think

Yeah I agree. We'll keep ctrl-u and ctrl-w for now. That means we still need to decide on a keymap for this PR then

EpocSquadron · 2022-04-22T03:39:55Z

I feel like alt-u makes sense while being easy for users to adapt if we change it to ctrl-u down the road.

pickfire · 2022-04-22T11:01:56Z

Like @archseer discussed on the chat

I'll probably open up a poll to see how many users use it. I also found space+w to save useful but we removed that 😛

deletion (ctrl-w, ctrl-u) are useful but I don't like the others that are only for movement. Movement should be done in normal mode

I think we probably need some sort of place to poll active users.

I also agree that those only for movements (like ctrl-f) are not useful, I don't use any of those, but IIRC on kakoune someone did ask for one of it like ctrl-e, maybe we might need it on the poll.

pickfire

Thanks for working on this @AntonioLucibello and aiding in the discussion of getting it alt-u while keeping the old key ctrl-u which I think is useful. \o/

Not sure if we are going to do a poll or something but I am going to let other maintainers merge this.

pickfire · 2022-04-24T15:28:37Z

Hmm, while reading the tutor I wonder if ctrl-s would be a good key since it's used in normal mode. That would be it consistent with normal mode. https://github.com/helix-editor/helix/pull/2133/files#diff-0751fcdc9d0b2b023e5c02ee4941114ca1706d04e1ba73c2f4c5ba287c847074R648-R650

the-mikedavis · 2022-04-25T00:56:59Z

ctrl-s might clash with the keybinding for signature help: #1755

although ctrl-s hasn't been committed to in that PR yet

I think ctrl-s would be an especially nice binding for this though since it mirrors the key in normal mode.

archseer · 2022-04-27T16:15:29Z

I think ctrl-s would be an especially nice binding for this though since it mirrors the key in normal mode.

Good point. Sorry for the bike shedding, let's use ctrl-s here!

added undo checkpoint command

2bc6225

AntonioLucibello closed this Apr 15, 2022

AntonioLucibello deleted the undo branch April 15, 2022 09:33

AntonioLucibello restored the undo branch April 15, 2022 09:35

AntonioLucibello reopened this Apr 15, 2022

AntonioLucibello changed the title ~~added undo checkpoint command~~ Add undo checkpoint command Apr 15, 2022

EpocSquadron reviewed Apr 15, 2022

View reviewed changes

changed add_undo_checkpoint to commit_undo_checkpoint

0ce3994

mapped commit_undo_checkpoint to Alt-u

d660652

AntonioLucibello force-pushed the undo branch from c789e25 to d660652 Compare April 15, 2022 21:01

pickfire approved these changes Apr 22, 2022

View reviewed changes

pickfire mentioned this pull request Apr 24, 2022

Extend tutor file #2133

Merged

pickfire mentioned this pull request Apr 25, 2022

Add lsp signature help #1755

Merged

5 tasks

Update default.rs

12623ad

archseer merged commit ac2ea80 into helix-editor:master May 2, 2022

paveloom mentioned this pull request May 4, 2022

Using :write after any command that produces changes introduces a phantom change #2192

Closed

the-mikedavis mentioned this pull request May 5, 2022

Undoing change in insert mode does nothing when first opening the file #2409

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add undo checkpoint command #2115

Add undo checkpoint command #2115

AntonioLucibello commented Apr 15, 2022 •

edited

Loading

AntonioLucibello commented Apr 15, 2022

EpocSquadron Apr 15, 2022

AntonioLucibello Apr 15, 2022

the-mikedavis Apr 15, 2022

EpocSquadron commented Apr 15, 2022

AntonioLucibello commented Apr 15, 2022 •

edited

Loading

AntonioLucibello commented Apr 15, 2022 •

edited

Loading

EpocSquadron commented Apr 15, 2022

AntonioLucibello commented Apr 15, 2022 •

edited

Loading

archseer commented Apr 20, 2022

pickfire commented Apr 21, 2022 •

edited

Loading

archseer commented Apr 21, 2022

archseer commented Apr 21, 2022

pickfire commented Apr 21, 2022 •

edited

Loading

archseer commented Apr 22, 2022

EpocSquadron commented Apr 22, 2022

pickfire commented Apr 22, 2022

pickfire left a comment

pickfire commented Apr 24, 2022

the-mikedavis commented Apr 25, 2022

archseer commented Apr 27, 2022

Add undo checkpoint command #2115

Add undo checkpoint command #2115

Conversation

AntonioLucibello commented Apr 15, 2022 • edited Loading

AntonioLucibello commented Apr 15, 2022

EpocSquadron Apr 15, 2022

Choose a reason for hiding this comment

AntonioLucibello Apr 15, 2022

Choose a reason for hiding this comment

the-mikedavis Apr 15, 2022

Choose a reason for hiding this comment

EpocSquadron commented Apr 15, 2022

AntonioLucibello commented Apr 15, 2022 • edited Loading

AntonioLucibello commented Apr 15, 2022 • edited Loading

EpocSquadron commented Apr 15, 2022

AntonioLucibello commented Apr 15, 2022 • edited Loading

archseer commented Apr 20, 2022

pickfire commented Apr 21, 2022 • edited Loading

archseer commented Apr 21, 2022

archseer commented Apr 21, 2022

pickfire commented Apr 21, 2022 • edited Loading

archseer commented Apr 22, 2022

EpocSquadron commented Apr 22, 2022

pickfire commented Apr 22, 2022

pickfire left a comment

Choose a reason for hiding this comment

pickfire commented Apr 24, 2022

the-mikedavis commented Apr 25, 2022

archseer commented Apr 27, 2022

AntonioLucibello commented Apr 15, 2022 •

edited

Loading

AntonioLucibello commented Apr 15, 2022 •

edited

Loading

AntonioLucibello commented Apr 15, 2022 •

edited

Loading

AntonioLucibello commented Apr 15, 2022 •

edited

Loading

pickfire commented Apr 21, 2022 •

edited

Loading

pickfire commented Apr 21, 2022 •

edited

Loading