kilo: revisiting the PAGE_UP and PAGE_DOWN keys

Back when I was implementing The PAGE_UP and PAGE_DOWN keys, I offered my own refactored implementation.

I've now just completed the steps to implement vertical scrolling in the text viewer phase of the project, I discover that my implementation no longer has the same effect once there is a file to be scrolled through. Fortunately, there is an easy fix:

    case PAGE_UP: 
        editor_move_cursor(0, -get_screen_height());
        break;
 
    case PAGE_DOWN:
        editor_move_cursor(0, get_screen_height());
        break;

As a side note, I found getting the scrolling code correct was surprisingly fiddly, due at least partially to my slightly different structure and very different naming conventions. I feel like there's some major structural refactoring that needs to be done, but I feel I'm already close to a place where many more changes will get me to the place where I can't map between the original version and my refactored version anymore. So I'm going to hold off on making any big changes at least until I work through the rest of the code.

kilo: refactoring access to g_editor_state

I've been continuing working through the steps to implement kilo, but I've been increasingly unhappy with the accesses to the global g_editor_state variable all over the code; it's ugly and makes too much of the code dependent on the precise declaration of that struct.

So I decided to refactor so that all accesses to the editor state are through aptly named functions. I'm a lot happier with the result. As an example, here's the current implementation of editor_draw_rows:

void editor_draw_rows(term_buffer* tb)
{
    for (int y = 0; y < get_screen_height(); y++)
    {
        if (y < get_file_lines())
        {
            int len = get_line_size();
            if (len > get_screen_width())
              len = get_screen_width();
            tb_append(tb, get_line_chars(), len);
        }
        else if (y != get_screen_height()/3)
            tb_append_str(tb, get_tilde_str());
        else if (get_file_lines() == 0)
            append_welcome_message(tb);
        
        tb_append_str(tb, get_clear_row_str());
        if (y + 1 < get_screen_height())
            tb_append_str(tb, get_rn_str());
    }
}

kilo commentary: Page Up and Page Down buttons

Oh, dear. Way back when I started this commentary, I said I didn't want to be critical, but how the Page Up and Page Down buttons are implemented is pretty ugly:

    case PAGE_UP:
    case PAGE_DOWN:
      {
        int times = E.screenrows;
        while (times--)
          editorMoveCursor(c == PAGE_UP ? ARROW_UP : ARROW_DOWN);
      }
      break;

Particularly when the Home and End keys are implemented more like I would expect:

    case HOME_KEY:
      E.cx = 0;
      break;
    case END_KEY:
      E.cx = E.screencols - 1;
      break;

My code:

    case PAGE_UP: 
        editor_set_cursor_row(0);
        break;
 
    case PAGE_DOWN:
        editor_set_cursor_row(g_editor_state.screenrows-1);
        break;

The code for editorReadKey (the final version, at least for this chapter, is here) has also become pretty ugly. Even in this relatively short implementation, all these nested ifs and elses make it very hard to see how the control logic works.

That said, I don't want to take time right now to work out a cleaner solution than the one I have right now (I'm more interested in the other parts of this code) so I'm going to leave this the way I have it. Probably the best answer is to break down and use a third-party library that has already solved this problem.

kilo commentary: Moving the cursor (steps 43-45)

In spite of a hiatus that's now more than a year long, I haven't forgotten about my kilo project. Steps 43-45 involve code to keep track of where the cursor is on the screen, and allow the user to move it around. Here's the original code that does that:

void editorMoveCursor(char key) {
  switch (key) {
    case 'a':
      E.cx--;
      break;
    case 'd':
      E.cx++;
      break;
    case 'w':
      E.cy--;
      break;
    case 's':
      E.cy++;
      break;
  }
}
void editorProcessKeypress() {
  char c = editorReadKey();
  switch (c) {
    case CTRL_KEY('q'):
      write(STDOUT_FILENO, "\x1b[2J", 4);
      write(STDOUT_FILENO, "\x1b[H", 3);
      exit(0);
      break;
    case 'w':
    case 's':
    case 'a':
    case 'd':
      editorMoveCursor(c);
      break;
  }
}

The code is basically decoding the w, s, a, and d characters twice: once to determine that the editorMoveCursor function should be called, and another to determine how the cursor should be moved. This is (IMO) another bug waiting to happen, in a large project these two points in the code could easily get out of sync. There's no reason not to do something like this (which is also my current solution):

void editor_move_cursor(int dcx, int dcy)
{
    g_editor_state.cx += dcx;
    g_editor_state.cy += dcy;
}


void editor_process_keypress(char c)
{
    string_const clear_screen = get_clear_screen_str();
    string_const home_cursor  = get_home_cursor_str();

    switch (c)
    {
    case CTRL_KEY('q'):
        write_str(clear_screen);
        write_str(home_cursor);
        exit(0);
        break;

    case 'w': editor_move_cursor( 0, -1); break;
    case 's': editor_move_cursor( 0,  1); break;
    case 'a': editor_move_cursor(-1,  0); break;
    case 'd': editor_move_cursor( 1,  0); break;
    }
}

Note that now only editor_process_keypress needs to know what keys move the cursor, and editor_move_cursor gets parameters that are directly interpretable.