Mark Jamsek <mark@jamsek.com> wrote:
> On 22-06-26 04:38pm, Omar Polo wrote:
> > Mark Jamsek <mark@jamsek.com> 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.