Move mode transition logic to handle_keymap_event()

[lucypero]

Moves mode transition logic to handle_keymap_event()

Fixes this:

#2051 (comment)

Suggestion here:

#2051 (comment)

[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.

[erasin]

I set remaping for close buffer, It back error.

[keys.normal.space]
x = [":buffer-close"]

thread 'main' panicked at 'called Option::unwrap() on a None value', helix-term/src/ui/editor.rs:784:61

[the-mikedavis]

I added a PR to fix this case: #3613

[erasin]

when close the last buffer , It will take error:

thread 'main' panicked at 'invalid HopSlotMap key used', helix-view/src/tree.rs:288:20
stack backtrace:
   0: std::panicking::begin_panic
             at /rustc/4b91a6ea7258a947e59c6522cd5898e7c0a6a88f/library/std/src/panicking.rs:616:12
   1: <slotmap::hop::HopSlotMap<K,V> as core::ops::index::IndexMut<K>>::index_mut
             at /Users/erasin/.cargo/registry/src/github.com-1ecc6299db9ec823/slotmap-1.0.6/src/hop.rs:948:21
   2: helix_view::tree::Tree::get_mut
-             at ./helix-view/src/tree.rs:288:20
   3: <helix_term::ui::editor::EditorView as helix_term::compositor::Component>::handle_event
-             at ./helix-term/src/ui/editor.rs:1268:28
   4: helix_term::compositor::Compositor::handle_event
             at ./helix-term/src/compositor.rs:172:19
   5: helix_term::application::Application::handle_terminal_events
             at ./helix-term/src/application.rs:453:22
   6: helix_term::application::Application::event_loop_until_idle::{{closure}}
             at ./helix-term/src/application.rs:292:21
   7: <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll
             at /rustc/4b91a6ea7258a947e59c6522cd5898e7c0a6a88f/library/core/src/future/mod.rs:91:19
   8: helix_term::application::Application::event_loop::{{closure}}
             at ./helix-term/src/application.rs:268:57
   9: <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll
             at /rustc/4b91a6ea7258a947e59c6522cd5898e7c0a6a88f/library/core/src/future/mod.rs:91:19
  10: helix_term::application::Application::run::{{closure}}
             at ./helix-term/src/application.rs:838:38
  11: <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll
             at /rustc/4b91a6ea7258a947e59c6522cd5898e7c0a6a88f/library/core/src/future/mod.rs:91:19
  12: hx::main_impl::{{closure}}
             at ./helix-term/src/main.rs:143:53

helix/helix-term/src/ui/editor.rs

Line 1254 in 1cbf552

mode => self.command_mode(mode, &mut cx, key),

helix/helix-term/src/ui/editor.rs

Line 1268 in 1cbf552

let view = cx.editor.tree.get_mut(focus);

I don't know how it works, but it's probably caused by this.
Maybe we need refresh something.

[the-mikedavis]

There's an issue for this now: #3626

[erasin]

I check tree.view().count() == 1 and Refetching the view id.
It seems to ok with #3635