Hitting `v` in select mode does not collapse selections

[thomie]

We have the following functions:

Mapped to keys.select.esc:

pub fn exit_select_mode(cx: &mut Context) {
    if EvilCommands::is_enabled() {
        // In evil mode, selections are possible in the selection/visual mode only.
        EvilCommands::collapse_selections(cx, CollapseMode::ToHead);
    }

    if cx.editor.mode == Mode::Select {
        cx.editor.mode = Mode::Normal;
    }
}

Mapped to keys.select.v and keys.insert.esc (and keys.normal.esc, but that's a no-op):

fn normal_mode(cx: &mut Context) {
    cx.editor.enter_normal_mode();
}

enter_normal_mode does 3 things:

• set editor.mode = Mode::Normal
• call try_restore_indent
• call restore_cursor

try_restore_indent and restore_cursor are only relevant when exiting insert mode (perhaps a better name for enter_normal_mode would be exit_insert_mode).

Conclusion

We should map keys.select.v to exit_select_mode instead of normal_mode.

[thomie]

Hmm, that's not the whole story.

enter_normal_mode is also called:

• when switching windows
• when switching buffers

So there is another way to reproduce the evil-helix bug: when first making a selection and then switching windows/buffers, if you later return to the window/buffer you will be in normal mode but the selection is still there.

So we need to modify enter_normal_mode. There's no avoiding it.

[usagi-flow]

That's an important issue you're mentioning here. I've been bothered since the beginning how I avoid selections in normal mode at local places in the code instead of at one or few central places.

I wanted to avoid the slight overhead of the collapsing in enter_normal_mode() and also didn't want to modify upstream code. But I'm afraid you're right: collapsing selections in enter_normal_mode() might be unavoidable indeed (if is_evil of course).

[thomie]

Or keep the selections but don't render them in normal mode?

Not sure, just thinking out loud.

[usagi-flow]

It's a good idea, but I fear that not showing selections while they're there might end up causing more problems than solutions. Helix doesn't exactly have this concept of hidden/secondary selections, as far as I'm aware.

I had a brief look at the function, and especially when it's called:

The good news is that enter_normal_mode() called in situations like switching buffers/windows. The bad news is that I didn't manage to collapse (with a very simplistic/dirty implementation, however).

diff --git a/helix-view/src/editor.rs b/helix-view/src/editor.rs
index e0165bb6..3b569bc5 100644
--- a/helix-view/src/editor.rs
+++ b/helix-view/src/editor.rs
@@ -2250,6 +2250,16 @@ pub fn enter_normal_mode(&mut self) {
             doc.set_selection(view.id, selection);
             doc.restore_cursor = false;
         }
+
+        log::warn!("enter_normal_mode() called; evil: {}", self.evil);
+        // if self.evil {
+        let text = doc.text().slice(..);
+        let selection = doc.selection(view.id).clone().transform(|range| {
+            let pos = range.cursor(text);
+            Range::new(pos, pos)
+        });
+        doc.set_selection(view.id, selection);
+        // }
     }

     pub fn current_stack_frame(&self) -> Option<&StackFrame> {

I hope the function doesn't get called too late - i.e. after switching to the target buffer/window.

PS: For some reason, self.evil also evaluated to false... I should check what's up with that field; it's probably a little detail I oversaw.

[thomie]

PS: For some reason, self.evil also evaluated to false... I should check what's up with that field; it's probably a little detail I oversaw.

Yeah that field is unused and can be deleted.

You should be able to use self.config().evil.