Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement command mode #90

Open
kneasle opened this issue Feb 6, 2021 · 9 comments
Open

Implement command mode #90

kneasle opened this issue Feb 6, 2021 · 9 comments

Comments

@kneasle
Copy link
Owner

kneasle commented Feb 6, 2021

To make implementation easier, the different modes in Sapling are implemented as a state machine (technically a DFA). So each 'mode' should have a struct which implements editor::state::State (like editor::normal_mode::State).

The most useful part of this trait is the 'transition function', which is run every time the user presses a key. It also returns Box<dyn state::State>, which allows for state transitions on key presses (i.e. if the user presses : in normal mode, it should return the default command mode state to enter command mode).

So implementing command mode basically boils down to the following:

  • Create a new State for command mode (would make sense to put it in editor/command_mode.rs).
  • Add an extra keybinding for : in normal mode, which switches the editor into command mode (see quitting the editor with q for how to do this).
  • Move w and q from insert mode into command mode, and if you want you can also add a command to write the .dot code for the Dag to either the log or a file.

If you want more pointers, then just ask 😁 - the state transition code is quite unintuitive.

@stokhos
Copy link
Contributor

stokhos commented Feb 8, 2021

I found this line of code in normal_mode.rs.

mut self: Box<Self>,

Are we overload the self with our own pointer here?
But the self: Box<Self> is immutable.
self: Box<Self>,

How can we use the mut reference here? I thought, the it would be self: Box<Self> in normal_mode, but somehow this code works smoothly, can you talk a little about this?

self.keystroke_buffer.push(key);

self is a pointer, but you did deref it, is this because self Deref coercion happened here?

@stokhos
Copy link
Contributor

stokhos commented Feb 8, 2021

How can I add : as valid key? I couldn't find a variant inside Key for colon.

@kneasle
Copy link
Owner Author

kneasle commented Feb 8, 2021

How can I add : as valid key? I couldn't find a variant inside Key for colon.

I think it's Key::Char(':')

How can we use the mut reference here? I thought, the it would be self: Box<Self> in normal_mode, but somehow this code works smoothly, can you talk a little about this?
...
self is a pointer, but you did deref it, is this because self Deref coercion happened here?

Oh! I hadn't really noticed that Box<Self> isn't mutable - I guess we're allowed to do what we want State::transition owns the Box and therefore the contents. I suspect that Box<T> derefs to &mut T, which causes this behaviour (and this is fine because Cloneing a Box also clones the contents). This is beyond my understanding of Rust 🤷 - I just hit it until it compiles 😆.

@stokhos
Copy link
Contributor

stokhos commented Feb 8, 2021

I tried to add mut in side the trait defintion. And it doens't compile. Surprised me

@kneasle
Copy link
Owner Author

kneasle commented Feb 8, 2021

Ah yes, apparently you have to put it in the implementation, not the trait - it seems that mut arguments aren't allowed it traits...

@stokhos
Copy link
Contributor

stokhos commented Feb 9, 2021

Learned something new, I feel great.

@kneasle
Copy link
Owner Author

kneasle commented Feb 9, 2021

Cool 😁. To be honest, the current code is incredibly janky and hard to understand - I was doing some unrelated work on Sapling last night and realised a way to make it much easier to understand. Would it cause much chaos if I were to change the type signatures for State::transition? I don't think much change would be required on your part - the signature would be something like fn transition(&mut self, /* same args as before */) -> Option<Box<dyn State>>, which would be a whole lot easier to understand.

@stokhos
Copy link
Contributor

stokhos commented Feb 9, 2021

Would it cause much chaos if I were to change the type signatures for State::transition?

No, go ahead. I haven't done much.

@stokhos stokhos mentioned this issue Feb 10, 2021
@xa888s
Copy link

xa888s commented Jun 10, 2021

Ah yes, apparently you have to put it in the implementation, not the trait - it seems that mut arguments aren't allowed it traits...

This is because Box<Self> is bound to self, which means that it is owned by the function, so the function can make the binding mutable it if it wants. Mutability of an owned value binding isn't part of its type, so it wouldn't make sense to have it be part of the trait definition. If that didn't make sense here's an example of what the mut some_owned_data sugar actually does.

// before
fn cool_function(mut some_owned_data: SomeCoolType) {}

// after
fn cool_function(some_owned_data: SomeCoolType) {
    // here it is shadowed by a mutable binding of the same name
    let mut some_owned_data = some_owned_data;
}

// these are both equivalent

For self the above desugaring doesn't compile (self cannot be shadowed), so I suspect the compiler does some magic with the mut to allow it to work.

EDIT: changing the mutability of a binding is an implementation detail

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants