Inaccurate redraw event parsing

[vhakulinen]

As briefly mentioned in #72 (as result of #63), our current redraw handling is (partly) incorrect. While it works mostly fine, there are situations where we might end up in incorrect screen state. The underlying problem is that we are not parsing all the arguments for each redraw event type.

My idea for a fix

Currently the parsed type looks something like this:

pub enum RedrawEvent (
    SetTitle(String),
    GridCursorGoto(u64, u64, u64),
    ...
)

Where the only correct type is the GridLine. We need to change all the enum members to contain a vector of data, tho' in some cases it doesn't really make sense (for example, SetTitle). But once that change is made, the prase_redraw_event will need to wrap each match arm into a for loop. E.g., it'll go from something like this:

match cmd {
    "set_title" => {
        let args = unwrap_array!(args[1]);
        let title = unwrap_str!(args[0]);
        RedrawEvent::SetTitle(title.to_string())
    }
    // ...
}

into something like this:

pub struct GridCursorGoto {
    grid: u64,
    row: u64,
    col: u64,
}

pub enum RedrawEvent {
    SetTitle(Vec<String>),
    GirdCursorGoto(Vec<GridCursorgoto>)
    // ...
}

match cmd {
    "set_title" => {
        let data = unwrap_array!(args)[1..].into_iter().map(|v| {
            unwrap_str!(args[0]).to_string()
        }).collect();
        RedrawEvent::SetTitle(data)
    }
    "grid_cursor_goto" => {
        let data = unwrap_array!(args)[1..].into_iter().map(|v| {
            GridCursorGoto {
                grid: unwrap_u64!(args[0]),
                row: unwrap_u64!(args[1]),
                col: unwrap_u64!(args[2]),
            }
        }).collect();
        RedrawEvent::GridCursorGoto(data)
    }
    // ...
}

As you can see, you're gonna get for loops everywhere. We could simplify this easily:

impl From<Value> for GridCursorGoto {
    fn from(args: Vec<Value>) -> Self {
        GridCursorGoto {
            grid: unwrap_u64!(args[0]),
            row: unwrap_u64!(args[1]),
            col: unwrap_u64!(args[2]),
        }
    }
}

fn parse_single_redraw_event(cmd: &str, args: Vec<Value>) -> RedrawEvent {
    match cmd {
        "set_title" => args.map(|v| => unwrap_str!(v))
        "grid_cursor_goto" => args.map(GridCursorGoto::from)
        // ...
    }
}

// Same as the current prase_redraw_event
fn parse_redraw_event(args: Vec<Value>) -> Vec<RedrawEvent> {
    args.into_iter()
        .map(|v| {
            let mut args = unwrap_array!(v);
            let cmd = unwrap_str!(args[0]);
            parse_single_redraw_event(cmd, args[1..])
        });
        .collect()
}

Of course, you'll need to remember to change the UI code too.

@smolck these are my ideas for refactoring of the event parsing code. Just leave a comment here if you're going to work on it. I can provide more feedback if needed, and I'm also open for other suggestions.

[smolck]

@vhakulinen Looks great, thanks! I’ll get working on this for sure! I think what you laid out is enough to get things going, but I’ll let you know if I run into any problems along the way. Once I get things started (hopefully later today) I’ll open a PR to track the progress.

As for handle_redraw_event in ui.rs, should I refactor things to break up that function? It isn’t strictly necessary, but it may make things cleaner and more readable (and since we’d already be refactoring it from the event parsing refactor, it’d probably be the best time to do it).

[vhakulinen]

The UI part needs to get a refactor too, but hold on it for a bit. I'll try to layout some foundation for you to work off from.

[smolck]

Sounds good! Just @ me once you get things laid out. It’ll be nice to get things working and refactored (and to get a cleaner codebase).