Separation of concerns

[matthias-t]

See src/command/mod.rs. One problem is that in its signature the treat_event function accepts State::Exit, but in the implementation it causes it to panic. I suggest removing the Exit variant and that treat_event return an Option<State>.

[matthias-t]

Also, shouldn't the state be in src/state/ ?

[IGI-111]

Just finished a refactor that should make this a bit cleaner.

The separation of the command handling and the actual editor state manipulation is deliberate. It's a classic three tier architecture. However the naming is a tad misleading. I think renaming the state module to data would make it clearer.

[matthias-t]

I understand the separation, I just think it could've been done better. azul gives a good overview of how it could be done. Correct me if I'm wrong, but command should handle input, not store state. That state belongs into data:

https://github.com/IGI-111/Smith/blob/b2c4723d9f5580fcc6cc0cc67fbff3e2b12dcd61/src/command/mod.rs#L8-L14

Here's a more complete representation of the editor's state:

type Position = usize;

struct State {
    edit: Edit,
    foot: Option<Foot>,
}

enum Foot {
    Message(String),
    Prompt(String, Edit),
}

struct Edit {
    text: Text,
    cursor: Position,
    selection: Option<Selection>,
    record: Record,
}

struct Selection {
    // the selection ends at the cursor position
    origin: Position,
    active: bool,
}

The handle function should not alter the view, which it currently does, but only modify the State. Its signature should be changed accordingly. https://github.com/IGI-111/Smith/blob/819160a04f3fa9a055cc0506e6b3cc1a6e4dc272/src/command/mod.rs#L20

pub fn handle<T>(event: Event, state: State) -> Option<State>

Then, the view should take the state and draw the editor, when it is called here: https://github.com/IGI-111/Smith/blob/819160a04f3fa9a055cc0506e6b3cc1a6e4dc272/src/main.rs#L51

[IGI-111]

Ah yes, putting it like that I can see what you mean. There's good reason for the mode state machine to live in state, if only because we should be able to process commands without rendering side effects.

we don't need to store whether the selection is active, because Termion tells us when the mouse button is held, and whenever it is held the selection is active.

I would disagree with that however. Coupling mode behavior to specific inputs is a bad idea. Maybe in the future we'll want to alter the selection through the keyboard, or at least not by holding a button.

[matthias-t]

You're right, I updated my suggestion.

[matthias-t]

Are you working on this or should I?

[IGI-111]

I though about it a little bit but I'm busy with other stuff at the moment. If you want to do the refactor go for it.