[WIP][Draft] Refactor ideas (Extension on #61)

[nixypanda]

Extends on #61

This PR is to demonstrate how these ideas actually might look like. I like some aspects of this PR some not so much. The EditEngine seems like a clean-cut and it houses a lot of the core logic that we have, we talk to this object via the EditCommand enum.
I wanted to house all the painting logic in the Painter class but I am not super excited with the interface that it ended up having. I want to look into it a bit more and see if we can get a better interface.
Lastly, the structure with the trait LineEditor is implemented in an inheritance-y way very similar to how a lot of people do in any OO lang. I really want these to compose. I want to chase the first way that I outlined in the issue that I created, but it will require more time investment from my side to look if this is possible or not.

Let me know what are your thoughts on this.

P.S. I am making this as a draft pull as I would like to get your feedback before I proceed any further than this.

Note (to reviewers): This is a massive PR I have tried to organize commits in a way that are self-contained. The best way to approach reviewing (IMO) is by first looking at the commit messages to get a high-level gist of what I was trying to do. And then look at stuff one commit at a time.

(Edit): ----------
This PR is just to showcase one approach. I don't intend for this to be merged. I think this serves as a base on which discussions can be made. Then if there are parts that the authors like, then I can work on them individually.

[sholderbach]

Wow!

I was pondering if we could manage the necessary repaints with a respective flag

enum InvalidationLevel {
  /// The fastest case
  Cursor, 
  /// as soon as something is typed or changed
  BufferOnly,
  /// Delayable more expensive version in the future for validation and syntax aware stuff (only makes sense if it doesn't interact with the prompt)
  BufferWithAnnotation,
  /// e.g. switching vi modes, Clock, they MUST be fixed sized and not shift buffer around, thus the rest would not have to be recomputed as well
  PromptStatus,
  /// On clear or resize, or other operations where the content changes starting at the Prompt
  Full
}

I haven't fully checked yet if we truly have to poll the termina with painting outside regular repaints for the unicode and wrapping detection.

[sholderbach]

So i found some time to compare the two versions so here are my two cents. (I did not try to implement a new feature with your version so grains of salt warranted)

• Extracting the Painter seems to absolute clean up some clutter and makes changes to output code centralized. Full endorsement from my side.
  • Future thought: Maybe the Painter could also do the bookkeeping on the terminal positions regarding where the current prompt started/where the current offsets are (in this case maybe named TerminalOutput) (need to be called on resize)
  • With a good design it is probably reasonable to hand out exclusive references of the Painter to (plugin) components.
• The EditEngine slims down the main struct by moving the run_edit_commands outside.
  • The question is with additional features coming in who owns components like completion engines, hinting and who would own visual elements (with output requirements) that require response to edit commands e.g. a list of possible completions, the history search etc.
  • Big Plus: If this exists somewhat separate from the main I/O loop it is probably feasible to run exact unit tests on stuff like vim key sequences with a modular keybinding/vim decoder. Getting all intricacies of vim behavior consistent with what people expect will probably require a bunch of tests.
  • Something that would need to be clarified over time is what actions should be implemented there and what on the underlying LineBuffer
• As we haven't implemented much in the ways of completions, syntax highlighting etc. it is probably not yet fully clear what representation will be best shared with those components (e.g. LineBuffer view or cloned string(s)) so there might be a good reason for an independent LineBuffer

Very subjective: I'm not currently convinced of the solution of having the main entry-point type "subclassed" on the key-map. I would probably prefer a more compositional approach of having swappable decoders like you describe in your first suggestion.

Pros for subclassing solution:

• You can build a simpler but more custom key event loop.
• Less points to check on modes
• We don't have to carry around potentially unused data structures and assuming in the release build everything gets properly inlined probably more room for performance optimized artifact

Contra

• At least for me the code duplication in the current state seemed to make maintenance harder than properly factored mode checks and dispatch.
• As there is no equivalent to protected visibility for trait methods, factoring even more into the trait would likely expose functionality that shouldn't be automatically pub
• minor: presenting two or more versions the consumer has to pick from might lead to slight inconveniences when providing docs and reasonable defaults without the consumer caring. Also we are slightly less helpful if somebody wants to change keybindings on the fly.

Yes the current state around the key handling is messy and on one side we don't have fully encountered the complexity around vi mode yet but on the other side we also havent't included a bunch of additional features that would all have to tie back into several core components of the line editor including key input.

So I can't foresee if this proposed API would shrink into nicely factored bits over time or rather grow. I have to admit that I don't have enough experience with Rust API patterns for that, so probably JT @jonathandturner should weigh in on that.

[nixypanda]

Extracting the Painter seems to absolute clean up some clutter and makes changes to output code centralized. Full endorsement from my side.

I like the idea of extracting out the Painter. Though I am not super happy with the API it offers presently. Though this is something we can improve further down the road.

The EditEngine slims down the main struct by moving the run_edit_commands outside.

Yeah, initially I thought that this was a good idea. I had not thought of completions, hinting, (or generally the plugin architecture?) outlined in #63. Thinking more about it, if we pull everything into the edit engine, would entail it having way more responsibilities for any struct/class and more importantly the API it will expose will be massive (making any changes to it require significant effort).

On the other hand, if we keep the edit engine core and make everything extensible by nu shell scripting we can do something like https://github.com/Nyquase/vi-mode/blob/master/vi-mode.zsh#L36-L63. Now my knowledge of this kind of architecture (where you have a core and you expose functionality through a scripting language) is absolutely limited. I don't really know how this will work here. I will need to read more to be able to comment on this.

Another idea here would be to pull out stuff from it into to line_buffer as I believe you previously mentioned. That also does a bit of clean-up without altering the present structure a lot. (Todo: I Will add a PR that does this and attach it here).

The question is with additional features coming in who owns components like completion engines, hinting and who would own visual elements (with output requirements) that require a response to edit commands e.g. a list of possible completions, the history search etc.

I think completion engine and hinting can potentially be done without having a lot of interaction with any of the components. This is based on the assumption that these components don't worry about painting the screen.

I'm not currently convinced of the solution of having the main entry-point type "subclassed" on the key-map. I would probably prefer a more compositional approach of having swappable decoders like you describe in your first suggestion.

I am in 100% agreement with you on this. Swappable decoders is what I want to achieve, but I was not able to do so cleanly. Some ways to go about it probably include communication between these decoders and the main struct via some enum.

[nixypanda]

I think we can close this one, as mentioned I will try to pick up some of these items in pieces trying to achieve the first design where we are using composition over inheritence.

[nixypanda]

Closing this as, this is being taken care of in smaller pieces in separate PRs, will link to this PR from the smaller pieces.