Move mode transition logic to handle_keymap_event()

[lucypero]

Moves mode transition logic to handle_keymap_event()

Fixes this:

https://github.com/helix-editor/helix/issues/2051#issue-1198579421

Suggestion here:

https://github.com/helix-editor/helix/issues/2051#issuecomment-1140848285

[icanhazcheeze]

What is the particular importance of tracking which command moved us into insert mode?

[archseer]

What is the particular importance of tracking which command moved us into insert mode?

This is described in the comment, there's different ways of entering insert mode. For example afoo, ifoo, ofoo, Ofoo all insert in different positions. If we're repeating the edit via ., that needs to be replicated.

[lucypero]

@archseer Changes made.

It still doesn't seem like the dot . operator works as it should. When I press . after executing this keymap:

[keys.normal]
C = ["collapse_selection", "extend_to_line_end", "change_selection"]

It doesn't quite do what I expect. it seems like it is only executing the last command, and it inserts what I typed in insert mode. Is this the expected behavior? Isn't it supposed to execute the first two commands too?

Edit: Right, so looking at it here, the dot operator only tracks the last command. Should it not track every command made on the last keymap?

pub struct EditorView {
    pub keymaps: Keymaps,
    on_next_key: Option<Box<dyn FnOnce(&mut commands::Context, KeyEvent)>>,
    last_insert: (commands::MappableCommand, Vec<InsertEvent>),
    pub(crate) completion: Option<Completion>,
    spinners: ProgressSpinners,
}

Maybe last_insert should be a Vec<commands::MappableCommand>.

[the-mikedavis]

That's expected behavior. collapse_selection and extend_to_line_end are motions (repeat with A-.) rather than changes (repeat with .)

[lucypero]

@archseer Please review the PR when possible. This bug is still present in Helix and it prevents using certain mappings because it crashes the application.

[archseer]

There's over a hundred PRs in the queue so it takes me a while.

The problem is that dot is supposed to only replay the last insert (and any side effect of the insert, for example inserting on the next line), not the whole sequence of movement. How should we handle this? itest<Esc> fits, but itest<Esc>ww would need to stop at the first time we leave insert mode. But then what about itest<Esc>wwiasd<Esc> which is technically two inserts?

The two simple solutions are:

• Make custom sequence commands unrepeatable. This would be consistent but it drops some nice usecases.
• Always repeat the whole sequence. This would produce inconsistent results if the user creates a weird mapping, but at least it would allow repetition.

We could also detect the last insert sequence and only repeat that, but it would be weird to the user if only half of the mapping is repeated.

[lucypero]

@archseer

Always repeat the whole sequence. This would produce inconsistent results if the user creates a weird mapping, but at least it would allow repetition.

I am totally for this option. I don't see why it shouldn't be like that. It just repeats all the actions. Just like Vim. It would be really useful and intuitive.

If you are not sure about that, we could just go with the first option for now just to fix the crashes:

Make custom sequence commands unrepeatable. This would be consistent but it drops some nice usecases.

What do you say?

[archseer]

Just like Vim.

Right, but we follow Kakoune where repeat is split into repeating edits, and repeating movement.

Let's always repeat the whole sequence for now, that way we at least resolve the panic for the time being.