Modularization of the main struct #134

nixypanda · 2021-08-15T14:25:44Z

This is like a 2nd take at the ambitious ticket #61 which was opend way back. But this time the results are more close to what we (me, @jntrnr and @sholderbach ) had in mind. (Note: Well what I inferred what @jntrnr and @sholderbach meant).

High level summary

EditorMode related stuff is completely decoupled from the main stuct and is behind a contract set by a trait (InputParser)
Adds another layer of Events (enum) called Reedline (subject to change). The idea is simple that the InputParser's parse the input we get from the users and turn that into events that Reedline understands.

Potential benefits

Completely decouples EditMode stuff and we can now develop them completely independently of what happens to the main struct (as long as we keep the contract)
The addition of events can help us in creating stuff like multiple streams from different sync producers that are eventually parsed by the main struct. This should help in easy dev. of widgets like clock etc. (Though that is still a far fetched dream)

Todo

Reintegration of vi mode is still pending
Tests

Note to reviewers

I think the best way to review this pull request is

Look at the commit summary to get an idea of what is going on.
Look at the whole thing together. Ignore most of the stuff and focus on the src/engine.rs and see if all the changes it's the internal structure and how it interacts with the new interfaces, a reasonable position to be in.
After that focusing on the individual aspects in detail.
I will try to add helpful comments on places where most attention is required.

Note: This is a massive PR, it would be completely understandable if you feel like it does not align with the project vision or if there is a possibility it can be broken down into smaller ones. I have opened this to get opinions from the maintainers.

- clock - vi stuff - requirement to do full repaint state management

src/engine.rs

nixypanda · 2021-08-22T01:49:47Z

src/engine.rs

+struct PromptWidget {
+    prompt: Box<dyn Prompt>,
+    offset: (u16, u16),
+    origin: (u16, u16),
+}


I am not so sure about this, the idea was similar to the previous change (pulling out the terminal size). Maybe eventually all these can be clubbed into a struct like UiState and all we do is update it.

If we embrace immutability then we can just have a previous state and present state and then the painter just does a diff and figures out what to do. This way all its (painter) operations become decoupled. (Kinda like what elm, iced, react do). If this sounds something I can do a bit more research and draft a proposal for it.

src/engine.rs

nixypanda · 2021-08-22T02:02:53Z

src/engine.rs

+    Enter,
+    Mouse, // Fill in details later
+    Resize(u16, u16),
+    EditInsert(EditCommand), // HACK: Special handling for insert


As mentioned it's a simple hack for now, I would not like a special case for InsertCommand

nixypanda · 2021-08-22T02:03:24Z

src/engine.rs

+    CtrlD, // Don't know a better name for this
+    CtrlC, // Don't know a better name for this


Let me know what the names for these operations are.

src/engine.rs

nixypanda · 2021-08-22T02:08:34Z

src/engine.rs

+trait InputParser {
    fn parse_event(&self, event: Event) -> ReedlineEvent;
+    fn update_keybindings(&mut self, keybindings: Keybindings);
+    fn edit_mode(&self) -> EditMode;
 }


This is a makeshift interface for now. It should remain the same in spirit only (i.e. it parses Events to ReedlineEvents) but the interface will definitely need updating.

Note: Now the user of this lib can develop their own InputParser's without any help from the lib. All they need to do is adhere to this interface and we will be done. (prompt handling might pose a caveat in this though)

nixypanda · 2021-08-22T02:09:41Z

src/engine.rs

-        let editor_event = match event {
-            Event::Key(KeyEvent { code, modifiers }) => match (modifiers, code) {
-                (KeyModifiers::NONE, KeyCode::Tab) => ReedlineEvent::HandleTab,
-                (KeyModifiers::CONTROL, KeyCode::Char('d')) => ReedlineEvent::CtrlD,
-                (KeyModifiers::CONTROL, KeyCode::Char('c')) => ReedlineEvent::CtrlC,
-                (KeyModifiers::CONTROL, KeyCode::Char('l')) => ReedlineEvent::ClearScreen,
-                (KeyModifiers::NONE, KeyCode::Char(c))
-                | (KeyModifiers::SHIFT, KeyCode::Char(c)) => {
-                    ReedlineEvent::EditInsert(EditCommand::InsertChar(c))
-                }
-                (KeyModifiers::NONE, KeyCode::Enter) => ReedlineEvent::Enter,
-                _ => {
-                    if let Some(binding) = self.find_keybinding(modifiers, code) {
-                        ReedlineEvent::Edit(binding)
-                    } else {
-                        ReedlineEvent::Edit(vec![])
-                    }
-                }
-            },
-
-            Event::Mouse(_) => ReedlineEvent::Mouse,
-            Event::Resize(width, height) => ReedlineEvent::Resize(width, height),
-        };
-
-        Ok(editor_event)


And finally, we achieve complete segregation of even parsing into a separate struct.

- Rename: input_parsing to edit_mode - Rename: EmacsInputParser to Emacs - Rename: ViInputParser to Vi - Rename: InputParser to EditMode - Add docs: EditMode - Add docs: Vi - Add docs: Emacs

nixypanda · 2021-08-24T05:10:54Z

Next Steps:

Resolve hacks

In order to achieve this segregation, I took a few shortcuts that need resolution

Special handling for the InsertChar command
Setup proper keybindings for VI (This IMO requires some thought as with vi there is stuff like nmap, imap, vmap, etc what all do we want to support here?)

These hacks don't have any impact on end-user. Have a little bit of impact on code quality but is easily resolvable, once we have an agreed-upon solution.

Pull non-editor commands out of `EditCommand` to `ReedlineEvent`

The following values of the enum EditCommand make more sense in ReedlineEvent

EditCommand::Clear
EditCommand::Up
EditCommand::Down
EditCommand::AppendToHistory
EditCommand::PreviousHistory
EditCommand::NextHistory
EditCommand::SearchHistory

Moreover, I think PreviousHistory and NextHistory are not even required now with the introduction of Up and Down commands.

sophiajt · 2021-08-24T06:44:18Z

@sherubthakur - did you want to do those next steps in this PR or a follow-up?

If you want to do them in a follow-up I can review this one with those remaining items in mind.

nixypanda · 2021-08-24T07:07:02Z

@sherubthakur - did you want to do those next steps in this PR or a follow-up?

If you want to do them in a follow-up I can review this one with those remaining items in mind.

I would suggest reviewing this assuming that I will do a follow-up PR (on both items)

My intention is that the hacks that I mentioned are resolved in this PR only. At least the InsertChar one. Vi one can be addressed later on.
Pulling the non-editor commands is up for discussion with the wider team and I will pick this up as a follow-up.

sophiajt

Looks good - just had some nits about constructor return types

src/engine.rs

sophiajt · 2021-08-28T18:10:13Z

Looks good

nixypanda added 15 commits August 15, 2021 17:07

Remove vagueness from position method

9dc4baf

Pull terminal size into the main struct

dbb1fd1

WIP: prompt widget

780973d

Remove a bunch of stuff

622ddd0

- clock - vi stuff - requirement to do full repaint state management

Remove edit mode from event matcher

360a6c6

Extract out event handling into a function

cebe859

Add reedline specific enum

c05f98e

Rebase fixes

16a323c

Update docs

8a7739d

Re-add clock functionality

ff9dd54

Add event-parser

5aca7c8

Pull out Input Parsing out of the main struct

44ea63c

Move input parsing stuff into a directory

4807bee

Move vi stuff into input parsing

762c475

Re-introduce Vi-Mode

371d7d2

nixypanda force-pushed the refactor-evetns branch from f0775f0 to 371d7d2 Compare August 22, 2021 01:35

nixypanda commented Aug 22, 2021

View reviewed changes

nixypanda added 5 commits August 24, 2021 09:51

Remove EditMode enum

9b003b7

Emacs mode uses non-default keybindings

9296047

Basic hygine for edit_mode

07e4a54

- Rename: input_parsing to edit_mode - Rename: EmacsInputParser to Emacs - Rename: ViInputParser to Vi - Rename: InputParser to EditMode - Add docs: EditMode - Add docs: Vi - Add docs: Emacs

Remove update_keybindings interface

86a86bc

Vi sends Repaint events on mode switch

98daa0b

nixypanda marked this pull request as ready for review August 24, 2021 04:48

sophiajt reviewed Aug 28, 2021

View reviewed changes

src/engine.rs Outdated Show resolved Hide resolved

src/engine.rs Outdated Show resolved Hide resolved

Rename new to create

3bf185c

sophiajt merged commit aebbc24 into nushell:main Aug 28, 2021

nixypanda mentioned this pull request Sep 3, 2021

Pull some events from EditCommand to ReedlineEvent #138

Merged

nixypanda mentioned this pull request Sep 24, 2021

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modularization of the main struct #134

Modularization of the main struct #134

nixypanda commented Aug 15, 2021 •

edited

Loading

nixypanda Aug 22, 2021

nixypanda Aug 22, 2021

nixypanda Aug 22, 2021

nixypanda Aug 22, 2021

nixypanda Aug 22, 2021

nixypanda Aug 22, 2021

nixypanda commented Aug 24, 2021

sophiajt commented Aug 24, 2021

nixypanda commented Aug 24, 2021 •

edited

Loading

sophiajt left a comment

sophiajt commented Aug 28, 2021

		CtrlD, // Don't know a better name for this
		CtrlC, // Don't know a better name for this

Modularization of the main struct #134

Modularization of the main struct #134

Conversation

nixypanda commented Aug 15, 2021 • edited Loading

High level summary

Potential benefits

Todo

Note to reviewers

nixypanda Aug 22, 2021

Choose a reason for hiding this comment

nixypanda Aug 22, 2021

Choose a reason for hiding this comment

nixypanda Aug 22, 2021

Choose a reason for hiding this comment

nixypanda Aug 22, 2021

Choose a reason for hiding this comment

nixypanda Aug 22, 2021

Choose a reason for hiding this comment

nixypanda Aug 22, 2021

Choose a reason for hiding this comment

nixypanda commented Aug 24, 2021

Resolve hacks

Pull non-editor commands out of EditCommand to ReedlineEvent

sophiajt commented Aug 24, 2021

nixypanda commented Aug 24, 2021 • edited Loading

sophiajt left a comment

Choose a reason for hiding this comment

sophiajt commented Aug 28, 2021

nixypanda commented Aug 15, 2021 •

edited

Loading

Pull non-editor commands out of `EditCommand` to `ReedlineEvent`

nixypanda commented Aug 24, 2021 •

edited

Loading