Code Organisation/Design Discussion: Making Editing Mode do more work

[nixypanda]

Hey

Firstly, love the work you are doing on nushell and I hope one day this would be my primary shell. One of the things that are stopping me is the vi-mode. If I am correct you are experimenting with reedline to see if we can have something better for the end-user. I have a bit of interest in this particular project.

I was playing around with the idea - where we make the core part of reedline as a library that an edit mode plugin/module can make use of. Initially, I wanted to do break the Reedline struct in the following manner.

pub struct Reedline {
    painter: Painter,
    edit_mode: EditMode,
    prompt: Prompt,
    editing_engine: EditingEngine,
}

where the components would have had the following responsibilities
Painter: Handles all painting interactions.
EditMode: This should parse the KeyEvents that we get into a Vec<EditCommand>. This is more than an enum in this design
Prompt: -
EditingEngine: Handles the list of EditCommand.

The primary motivation was to have EditMode structured in a way where emcas and vi stuff can reside in a completely separate struct. Where they do not know about each other. The metric for code design that I was using was "how easy is it to get rid of this feature (vi editing mode or emacs editing mode)".

Eventually, I figured that the edit mode controls more stuff than just translation of KeyEvents to Vec<EditCommand>, like deciding on how to paint the prompt (VI: Normal, Visual, Insert), etc. So I thought of making a trait that will define the interface we will need from a line editor with an editing mode. ViReedline (or EmacsReedline).

Here is the trait in question. We can get a Painter object using a methond and all the methods can then have a default so we will not need to focus on stuff other than read_line function in it's implementors

trait LineEditor {
    fn print_line(&self);
    fn print_events(&self);
    fn print_crlf(&self);
    fn print_history(&self);
    fn clear_screen(&self);
    fn read_line(&self, prompt: Box<dyn Prompt>) -> Signal;
}

and the (VI) struct that will eventually implement it.

#[derive(Eq, PartialEq, Clone, Copy)]
enum Mode {
    Normal,
    Insert,
}

pub struct ViLineEditor {
    painter: Painter,
    // keybindings: Keybindings,
    mode: Mode,
    partial_command: Option<char>,
    edit_engine: EditEngine,
    need_full_repaint: bool,
}

The emacs counterpart would be less complex than this (as far as I can see)

pub struct EmacsLineEditor {
    painter: Painter,
    keybindings: Keybindings,
    edit_engine: EditEngine,
}

Let me know if this is a direction that makes sense to you, or if you would like to see how the codebase would look like if we chase this direction. I can then cook up a PR and share the same with you.

[sophiajt]

@sherubthakur - thanks for the detailed design!

So it seems like the way this would work would be kind of "pluggable" logic that where some part of the Engine as we have it today lives in a separate part of the code. We did a little of this in the last stream with ViEngine, but this sounds like a deeper change that would also move the emacs logic into its own engine as well.

If I understand correctly, they would still use a similar EditCommand, though maybe that is something that's also changed a bit?

Something related is that I think you mention is that other parts of the engine should also be pluggable. @fdncred mentioned to me the other day how we should make sure to make completions pluggable, so that we could design new completion popups and widgets without having to change a bunch of the code in reedline. This makes sense, and is another good area we could make more generic with clever use of traits.

I'd say if it sounds like I understand the gist of what you're suggesting, let's go for it!

[sholderbach]

Some things I will just throw around to think about:

• How much of the text editing actions should be implemented on LineBuffer or implemented in your proposed EditEngine. If this would become too much of a thin wrapper, we might want to collapse reasonable parts.
• Thinking about the vi mode we probably want to be able to express most of the actions as building blocks for selecting a span and then executing either just a cursor move, cutting/copying, a replacement, or some special function for upper-casing etc. This would also be great for programmatic interfaces when doing completions or running expansions on snippets, paths or in editor shell expansion.
• When we display hints or completions inline should this be part of the line buffer, do we want to maintain a separate list of all available or displayed hints (think type hints, completion on the current typed token, history based hint for the whole line) in the EditEngine
• We may want to maintain the core command line buffer while having separate editing functionality on search strings (in the reverse/forward search mode) and aborting a search may preserve the previous line edit.
• Having nice modal dialogs for reverse search and also completions should play seamlessly with the keybinding actions (challenge can be seen with feat: migrate and extend history search #60) so the line editor should be able to switch modes that then affect the chosen actions and the repaint logic.

[fdncred]

@jonathandturner I tried to codify my very high level thoughts here about plugable type things and document an over-arching theme about what would make a line-editor great. It's higher level than this and higher level than your TODO.txt but I wonder how many of them the reedline community agrees with and would like to see.

[nixypanda]

How much of the text editing actions should be implemented on LineBuffer or implemented in your proposed EditEngine. If this would become too much of a thin wrapper, we might want to collapse reasonable parts.

When I pulled this abstraction out, a lot of what it was doing is something that the line-buffer should do (IMO). The way I have structured it currently, it has more than a few responsibilities which I don't like.

[nixypanda]

The following PRs together achieved this --

1. Extracting operations on buffer into line buffer
2. Extracting Paint operations out of main buffer
3. Unification of History operations
4. Extraction of core editing functionality
5. Extraction to core editor - undo/redo
6. Extraction to core editor - up/down
7. Extraction of EditMode

There are some other PRs that help with some minor fixes. These are the major ones that help in achieving the vision outlined in this issue.
I think we can close this issue.

[sophiajt]

And.... Done