From: Omar Polo Subject: Re: tog: horizontal split To: Mark Jamsek Cc: gameoftrees@openbsd.org Date: Mon, 27 Jun 2022 09:27:51 +0200 Mark Jamsek wrote: > On 22-06-26 04:38pm, Omar Polo wrote: > > Mark Jamsek wrote: > > > The below diff adds support for horizontal splits in tog. > > > > first of all, this is awesome. > > Thanks, Omar! Comments inline. Basically I agree with everything except > the `view->nlines - 1; /* border */` line (explained below). And I'll > make these changes this evening. yeah, that was just a mistake on my part, sorry :/ > > > [...] > > > There are a couple bugs that need squishing: > > > - resizing with an open hsplit sometimes kills the border and there > > > seems to be an OB1 that makes the top split misrender a line > > > - the double-width bug in the log view has returned where we overwrite > > > the vertical border by one column; strangely enough if you open the > > > commit in ja.git in a vsplit then press F, tab, tab, F, it renders > > > correctly > > > > part of the issues i think it's that we don't properly resize the > > parent. when resizing during a horizontal split the child view is > > resized and re-rendered, the parent view not. > > Thanks for the suggestion; I'll see if I can come up with something. I was wrong, the view is resized correctly but the sizes are not taken into account correctly. oh, and yes, there is also an off by one :) > > > [...] > > > @@ -708,9 +751,16 @@ view_splitscreen(struct tog_view *view) > > > { > > > const struct got_error *err = NULL; > > > > > > - view->begin_y = 0; > > > - view->begin_x = view_split_begin_x(0); > > > - view->nlines = LINES; > > > + if (view->mode == TOG_VIEW_SPLIT_HRZN) { > > > + err = view_split_begin_y(&view->begin_y, view->nlines); > > > + if (err) > > > + return err; > > > + view->begin_x = 0; > > > + } else { > > > + view->begin_x = view_split_begin_x(0); > > > + view->begin_y = 0; > > > + } > > > + view->nlines = LINES - view->begin_y; > > > view->ncols = COLS - view->begin_x; > > > view->lines = LINES; > > > view->cols = COLS; > > > @@ -718,10 +768,13 @@ view_splitscreen(struct tog_view *view) > > > if (err) > > > return err; > > > > > > + if (view->parent && view->mode == TOG_VIEW_SPLIT_HRZN) > > > + view->parent->nlines = view->lines - view->nlines - 1; I think that subtracting one here is wrong. The border is drawn on the *parent* view, so we don't need to subtract one here. nlines should account also for the border (just as ncols do) > > > [...] > > > + if (hs) { > > > + err = view_splitscreen(view->child); > > > + if (err) > > > + return err; > > > + view_border(view->child); likewise, we should call view_border on the _parent_ view, not on the child. With these two tweaks the border remains when resizing the term, but there is still an accounting issue somewhere because parent_view->ncols stays at 80 even after the resize... no idea where that happens > > > + update_panels(); > > > + doupdate(); > > > + show_panel(view->child->panel); > > > + nlines = view->nlines; > > > + ncols = view->ncols; > > > + } > > > } else if (view->parent == NULL) > > > ncols = COLS; > > > [...] > > > +/* Split view horizontally at y and offset view->state->selected line. */ > > > +static const struct got_error * > > > +view_init_hsplit(struct tog_view *view, int y) > > > +{ > > > + const struct got_error *err = NULL; > > > + > > > + view->nlines = y; > > > + err = view_resize(view); > > > + if (err) > > > + return err; > > > + > > > > since the border is drawn *on top* of the window and in this case it's > > would override only a line, you can avoid changing view->nlines here, it > > doesn't have any issues like drawing on top of a column has. > > This change isn't exactly for drawing the border but for later math as > we use nlines to determine how far to scroll. So if we don't add the > - 1 we will scroll offscreen when the split is active. To see what > I mean, if you remove the `view->nlines = y - 1;` line: > > $ TOG_VIEW_SPLIT_MODE=h tog > return (open commit) > tab (to log view) > j (to end of log split and the selection cursor will scroll offscreen) agreed, sorry for suggesting the wrong thing ^^ > > > [...] > > > -static void > > > -ref_scroll_down(struct tog_ref_view_state *s, int maxscroll) > > > +static const struct got_error * > > > +ref_scroll_down(struct tog_view *view, int maxscroll) > > > { > > > + struct tog_ref_view_state *s = &view->state.ref; > > > struct tog_reflist_entry *next, *last; > > > int n = 0; > > > > > > @@ -6608,13 +6868,16 @@ ref_scroll_down(struct tog_ref_view_state *s, int maxs > > > next = TAILQ_FIRST(&s->refs); > > > > > > last = s->last_displayed_entry; > > > - while (next && last && n++ < maxscroll) { > > > - last = TAILQ_NEXT(last, entry); > > > - if (last) { > > > + while (next && n++ < maxscroll) { > > > + if (last) > > > + last = TAILQ_NEXT(last, entry); > > > + if (last || (view->mode == TOG_VIEW_SPLIT_HRZN)) { > > > s->first_displayed_entry = next; > > > next = TAILQ_NEXT(next, entry); > > > } > > > } > > > + > > > + return NULL; > > > > maybe this is a leftover from a previous attempt? why making this > > return error when it unconditionally returns NULL? > > We need this signature for the function pointer in > offset_selection_down(). We could drop the pointer and call each view's > routine in the case block if you think that's better? It just means > duplicating the line offset bit in each case, but then we can keep this > void. woops, you're right, i missed that this needs to return an error to keep the signature for the function pointer, it's fine as-is then. disregard my comments here.