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.

> i usually keep my xterms at 80x24 and so I almost never enjoy the split
> mode.  Having a horizontal split in tog instead is a game changer when
> using smaller/not wide terminals.

I run my terms pretty large but I do prefer hsplits so I'm really
looking forward to this too.

> > We use a couple new envvars to configure splits:
> > 
> > TOG_VIEW_SPLIT_MODE=(h|v)
> >   v: force vertical split
> >   h: force horizontal split
> >   default/unset: if COLUMNS >= 120 we open a vsplit, else hsplit
> > TOG_VIEW_SPLIT_HEIGHT=n such that 1 <= n <= LINES - 3
> >   default/unset: 60% of LINES
> 
> This is the only thing I'm not too sure about.  While stuff like colors
> is highly subjective and thus having a mean to customize it it's
> warranted, these kind of settings are a bit too much IMHO.  I'd prefer
> to have a sane default "hardcoded" behaviour and eventually change it if
> people complains.
> 
> But lets hear what other thinks about this too.

Stefan also isn't sold on this idea, but I think I like his suggestion
to make it a relative value (i.e., % of LINES).

> > So if the user does nothing the only change in existing behaviour is
> > that we'll open a horizontal split when COLUMNS < 120 (i.e., when we
> > would normally open child views in fullscreen mode now).
> > 
> > Horizontal splits are supported in log -> diff, tree -> blame, ref ->
> > log, and blame -> commit. So all the main views will open their primary
> > child view in a horizontal split, but we haven't yet got some of the
> > secondary child views (e.g., 't' or 'r' key maps from log view).
> > 
> > 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.

> > The diff's already quite large though (sorry!) and I think it's ready
> > for more eyes if not use to see what other bugs I've introduced and if
> > the interface is friendly enough. One thing in particular that I'm not
> > sure about is, if the user sets an invalid TOG_VIEW_SPLIT_HEIGHT, we
> > error out. But perhaps it'd be better to fallback to the default value
> > instead. This is trivial though and can be refined later.
> 
> If an environment variables has a wrong format I'd just ignore it.
> 
> If one runs with TOG_VIEW_SPLIT_HEIGHT=banana it's just their fault and
> they can't expect it to work.  On the other hand, returning a fatal
> error upon split also isn't nice IMHO.

Yes, I agree. We'll just ignore garbage values and fallback to default.

> Just a few nits spotted while reading the diff.  I still don't know the
> cause of the bugs, will try to look into this with more attention later :)
> 
> > diff refs/heads/main refs/heads/dev/hsplit
> > blob - 728ba5ee9e20edbde20ed7f0c2ff1cbabad89c0b
> > blob + 4df10bd264fa7c164c9bb9b3b233bdce245a1cc4
> > --- tog/tog.1
> > +++ tog/tog.1
> > @@ -76,8 +76,14 @@ Switch focus between views.
> >  .It Cm F
> >  Toggle fullscreen mode for a split-screen view.
> >  .Nm
> > -will automatically use split-screen views if the size of the terminal
> > -window is sufficiently large.
> > +will automatically use vertical split-screen views if the width of the
> > +terminal window is sufficiently large, otherwise views will be split
> > +horizontally.
> > +This behavior can be customized with
> > +TOG_VIEW_SPLIT_MODE and TOG_VIEW_SPLIT_HEIGHT values.
> > +See
> > +.Sx ENVIRONMENT
> > +for details.
> >  .El
> >  .Pp
> >  Global options must precede the command name, and are as follows:
> > @@ -560,6 +566,36 @@ work tree, use the repository path associated with thi
> >  .El
> >  .El
> >  .Sh ENVIRONMENT
> > +Depending on the COLUMNS environment variable,
> > +.Nm
> > +will open child views in horizontal or vertical split-screens.
> > +By default, a view will be opened in a vertical split if COLUMNS \(>= 120.
> > +Otherwise, the view will be opened in a horizontal split.
> > +This behavior can be configured with the following environment variables.
> > +.Bl -tag -width TOG_VIEW_SPLIT_HEIGHT
> > +.It Ev TOG_VIEW_SPLIT_MODE
> > +Open views in a horizontal or vertical split.
> > +Value can be
> > +.Qq h
> > +for horizontal
> > +or
> > +.Qq v
> > +for vertical.
> > +If not set,
> > +.Nm
> > +will default to the above described behavior.
> > +.It Ev TOG_VIEW_SPLIT_HEIGHT
> > +Height of the child view opened in a horizontal split.
> > +Value must be an integer in the range 1 \(<=
> > +.Sy n
> > +\(<= LINES - 3.
> > +.Xr expr 1
> > +can be used to set terminal-relative values; for example, 50% of the terminal
> > +height can be approximated with
> > +.Li TOG_VIEW_SPLIT_HEIGHT=$(expr $LINES \e* 50 / 100) .
> > +If not set,
> > +horizontal splits will default to approximately 60% of LINES.
> > +.El
> >  .Bl -tag -width TOG_COLORS
> >  .It Ev TOG_COLORS
> >  .Nm
> > blob - e63f4a7c65678ce2cdbd63fb24abb940c2551358
> > blob + 3a75f96aaa9c44818fc58dfc7cb0339c0b8d1e0f
> > --- tog/tog.c
> > +++ tog/tog.c
> > @@ -64,6 +64,8 @@
> >  #define	MAX(_a,_b) ((_a) > (_b) ? (_a) : (_b))
> >  #endif
> > 
> > +#define ABS(_n)		((_n) >= 0 ? (_n) : -(_n))
> 
> you can also use abs(3) instead of a macro.

All this time I've been carrying around ABS! Thanks :)

> > +
> >  #define CTRL(x)		((x) & 0x1f)
> > 
> >  #ifndef nitems
> > @@ -105,6 +107,14 @@ enum tog_view_type {
> >  	TOG_VIEW_REF,
> >  };
> > 
> > +enum tog_view_mode {
> > +	TOG_VIEW_SPLIT_NONE,
> > +	TOG_VIEW_SPLIT_VERT,
> > +	TOG_VIEW_SPLIT_HRZN
> > +};
> > +
> >
> > +#define HSPLIT_SCALE	0.4  /* default horizontal split scale */
> 
> I'd set this to 0.3.  Highly subjective, but I like a bit better with
> 0.3 on a 80x24 terminal.

Yes, very subjective, but I think you're right here too.

> > +
> >  #define TOG_EOF_STRING	"(END)"
> > 
> >  struct commit_queue_entry {
> > @@ -499,9 +509,10 @@ struct tog_view {
> >  	TAILQ_ENTRY(tog_view) entry;
> >  	WINDOW *window;
> >  	PANEL *panel;
> > -	int nlines, ncols, begin_y, begin_x;
> > +	int nlines, ncols, begin_y, begin_x; /* based on split height/width */
> >  	int maxx, x; /* max column and current start column */
> >  	int lines, cols; /* copies of LINES and COLS */
> > +	int nscrolled, offset; /* lines scrolled and hsplit line offset */
> >  	int ch, count; /* current keymap and count prefix */
> >  	int focussed; /* Only set on one parent or child view at a time. */
> >  	int dying;
> > @@ -519,6 +530,7 @@ struct tog_view {
> >  	 */
> >  	int focus_child;
> > 
> > +	enum tog_view_mode mode;
> >  	/* type-specific state */
> >  	enum tog_view_type type;
> >  	union {
> > @@ -670,6 +682,7 @@ view_open(int nlines, int ncols, int begin_y, int begi
> > 
> >  	view->ch = 0;
> >  	view->count = 0;
> > +	view->nscrolled = 0;
> 
> not really an issue, but calloc already zeroes the struct.  There are
> others zero assignments there tho.

I'll remove these assignments.

> >  	view->type = type;
> >  	view->lines = LINES;
> >  	view->cols = COLS;
> > @@ -701,6 +714,36 @@ view_split_begin_x(int begin_x)
> >  	return (COLS - MAX(COLS / 2, 80));
> >  }
> > 
> > +/*
> > + * Find start line for horizontal split. If TOG_VIEW_SPLIT_HEIGHT is set and
> > + * between 1 and lines - 3 (inclusive), subtract value from lines and assign
> > + * to *y, else assign lines * HSPLIT_SCALE to *y.
> > + */
> > +static const struct got_error *
> > +view_split_begin_y(int *y, int lines)
> > +{
> > +	const char	*errstr;
> > +	char		*height = NULL;
> > +	int		 h = 0;
> > +
> > +	height = getenv("TOG_VIEW_SPLIT_HEIGHT");
> > +
> > +	/*
> > +	 * XXX Should we error out here or fallback to default if
> > +	 * an invalid TOG_VIEW_SPLIT_HEIGHT value has been set?
> > +	 */
> 
> if we agree on ignoring a malformed TOG_VIEW_SPLIT_HEIGHT then you can
> also simplify this furter and make this function just return an int.

This will be much simpler too. I think we should do this.

> > +	if (height) {
> > +		h = strtonum(height, 1, lines - 3, &errstr);
> > +		if (errstr != NULL)
> > +			return got_error_msg(GOT_ERR_RANGE,
> > +			    "invalid TOG_VIEW_SPLIT_HEIGHT");
> > +		*y = lines - h;
> > +	} else
> > +		*y = lines * HSPLIT_SCALE;
> > +
> > +	return NULL;
> > +}
> > +
> >  static const struct got_error *view_resize(struct tog_view *);
> > 
> >  static const struct got_error *
> > @@ -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;
> > +
> >  	if (mvwin(view->window, view->begin_y, view->begin_x) == ERR)
> >  		return got_error_from_errno("mvwin");
> >
> 
> I'd leave this as it was before:

ok, will do!

> > ..snip..
> 
> we need something like this also when quitting the split and falling
> back to a single log view.  To reproduce try (in a 80x24 term) RET to
> open a diff in a split, TAB to focus the log, C-l to reload the commits,
> TAB again to focus the diff, q to quit the diff.

I'll fix this. Thanks for pointing it out.

> > +		}
> > +
> > +		/*
> > +		 * XXX This is ugly and needs to be moved into the above
> > +		 * logic but "works" for now and my attempts at moving it
> > +		 * break either 'tab' or 'F' key maps in horizontal splits.
> > +		 */
> 
> :)

:)

> 
> > +		if (hs) {
> > +			err = view_splitscreen(view->child);
> > +			if (err)
> > +				return err;
> > +			view_border(view->child);
> > +			update_panels();
> > +			doupdate();
> > +			show_panel(view->child->panel);
> > +			nlines = view->nlines;
> > +			ncols = view->ncols;
> > +		}
> >  	} else if (view->parent == NULL)
> >  		ncols = COLS;
> > 
> > @@ -803,7 +919,7 @@ view_resize(struct tog_view *view)
> >  	view->lines = LINES;
> >  	view->cols = COLS;
> > 
> > -	return NULL;
> > +	return err;
> 
> I'd keep `return NULL' here.  IMHO it makes clearer that if we reach
> this point there are no errors, otherwise you have to read more
> carefully the function to see whether err is set sometime before and not
> bailed out.  (even if it means having one more byte :P)

That makes sense, and I like the premise too. Previously I tend to
"future-proof" against changes in the routine so I don't have to worry
about changing it later if the logic necessitates it, but that's just
being lazy. Plus, making it easier to read is more important. I'll
change this; thanks, Omar.

> > 
> >  static const struct got_error *
> > @@ -819,15 +935,29 @@ view_close_child(struct tog_view *view)
> >  	return err;
> >  }
> > 
> > -static const struct got_error *
> > +static void
> >  view_set_child(struct tog_view *view, struct tog_view *child)
> >  {
> >  	view->child = child;
> >  	child->parent = view;
> > 
> > -	return view_resize(view);
> > +	return;
> >  }
> > 
> > +static int
> > +view_needs_focus_indication(struct tog_view *view)
> > +{
> > +	if (view_is_parent_view(view)) {
> > +		if (view->child == NULL || view->child->focussed)
> > +			return 0;
> > +		if (!view_is_splitscreen(view->child))
> > +			return 0;
> > +	} else if (!view_is_splitscreen(view))
> > +		return 0;
> > +
> > +	return view->focussed;
> > +}
> > +
> >  static void
> >  tog_resizeterm(void)
> >  {
> > @@ -919,6 +1049,9 @@ get_compound_key(struct tog_view *view, int c)
> >  	return c;
> >  }
> > 
> > +static const struct got_error *offset_selection_down(struct tog_view *);
> > +static void offset_selection_up(struct tog_view *);
> > +
> >  static const struct got_error *
> >  view_input(struct tog_view **new, int *done, struct tog_view *view,
> >      struct tog_view_list_head *views)
> > @@ -1001,11 +1134,31 @@ view_input(struct tog_view **new, int *done, struct to
> >  			view->focussed = 0;
> >  			view->parent->focussed = 1;
> >  			view->parent->focus_child = 0;
> > -			if (!view_is_splitscreen(view))
> > +			if (!view_is_splitscreen(view) &&
> > +			    view->mode == TOG_VIEW_SPLIT_HRZN) {
> > +				if (view->parent->type == TOG_VIEW_LOG) {
> > +					err = request_log_commits(view->parent);
> > +					if (err)
> > +						return err;
> > +				}
> >  				err = view_fullscreen(view->parent);
> > +				if (err)
> > +					return err;
> > +				offset_selection_up(view->parent);
> > +			}
> >  		}
> >  		break;
> >  	case 'q':
> > +		if (view->parent && view->mode == TOG_VIEW_SPLIT_HRZN) {
> > +			if (view->parent->type == TOG_VIEW_LOG) {
> > +				/* might need more commits to fill fullscreen */
> 
> to simplify things, can't we run this only once at a higher level?  Say,
> every time a split changes or the windows is resized we call
> request_log_commits (or only if the old size is different from the
> current.)  it would avoid keeping this logic all around the places.
> 
> (i'm still not familiar on how the flow in tog at a higher level is and
> thus not sure if this is currently feasible.)

That would be a lot better. I'm also not entirely sure but I think it
should be doable.

> > +				err = request_log_commits(view->parent);
> > +				if (err)
> > +					break;
> > +			}
> > +			offset_selection_up(view->parent);
> > +			view->parent->mode = TOG_VIEW_SPLIT_NONE;
> > +		}
> >  		err = view->input(new, view, ch);
> >  		view->dying = 1;
> >  		break;
> > @@ -1034,13 +1187,24 @@ view_input(struct tog_view **new, int *done, struct to
> >  				err = view_fullscreen(view);
> >  			} else {
> >  				err = view_splitscreen(view);
> > -				if (!err)
> > +				if (!err && view->mode != TOG_VIEW_SPLIT_HRZN)
> >  					err = view_resize(view->parent);
> >  			}
> >  			if (err)
> >  				break;
> >  			err = view->input(new, view, KEY_RESIZE);
> >  		}
> > +		if (err)
> > +			break;
> > +		if (view->type == TOG_VIEW_LOG) {
> > +			err = request_log_commits(view);
> > +			if (err)
> > +				break;
> > +		}
> > +		if (view->parent)
> > +			err = offset_selection_down(view->parent);
> > +		if (!err)
> > +			err = offset_selection_down(view);
> >  		break;
> >  	case KEY_RESIZE:
> >  		break;
> > @@ -1069,38 +1233,6 @@ view_input(struct tog_view **new, int *done, struct to
> >  	return err;
> >  }
> > 
> > -static void
> > -view_vborder(struct tog_view *view)
> > -{
> > -	PANEL *panel;
> > -	const struct tog_view *view_above;
> > -
> > -	if (view->parent)
> > -		return view_vborder(view->parent);
> > -
> > -	panel = panel_above(view->panel);
> > -	if (panel == NULL)
> > -		return;
> > -
> > -	view_above = panel_userptr(panel);
> > -	mvwvline(view->window, view->begin_y, view_above->begin_x - 1,
> > -	    got_locale_is_utf8() ? ACS_VLINE : '|', view->nlines);
> > -}
> > -
> > -static int
> > -view_needs_focus_indication(struct tog_view *view)
> > -{
> > -	if (view_is_parent_view(view)) {
> > -		if (view->child == NULL || view->child->focussed)
> > -			return 0;
> > -		if (!view_is_splitscreen(view->child))
> > -			return 0;
> > -	} else if (!view_is_splitscreen(view))
> > -		return 0;
> > -
> > -	return view->focussed;
> > -}
> > -
> >  static const struct got_error *
> >  view_loop(struct tog_view *view)
> >  {
> > @@ -1143,7 +1275,8 @@ view_loop(struct tog_view *view)
> >  			if (view->parent) {
> >  				view->parent->child = NULL;
> >  				view->parent->focus_child = 0;
> > -
> > +				/* Restore fullscreen line height. */
> > +				view->parent->nlines = view->parent->lines;
> >  				err = view_resize(view->parent);
> >  				if (err)
> >  					break;
> > @@ -1907,7 +2040,8 @@ draw_commits(struct tog_view *view)
> >  	s->last_displayed_entry = s->first_displayed_entry;
> >  	ncommits = 0;
> >  	while (entry) {
> > -		if (ncommits >= limit - 1)
> > +		if (ncommits >= (view->mode == TOG_VIEW_SPLIT_HRZN ?
> > +		    view->nlines - 1 : view->lines - 1))
> 
> i think that here we should always use nlines.  lines should only be a
> copy of the value of LINES last time we looked at it.

Yes, lines is always a copy of LINES and nlines is the split-dependent
value. So lines would only be the height of a hsplit when in fullscreen
mode (i.e., 'F' key map was pressed), but is also the height of a view
(i.e., same as nlines) when either not split or in vsplit mode, which is
why I made this check. But now you point this out, I think you're right
because we always adjust nlines when we resize after 'F' is pressed so
the check is redundant.  Good catch! I'll change this too.

> >  			break;
> >  		if (ncommits == s->selected)
> >  			wstandout(view->window);
> > @@ -1922,7 +2056,7 @@ draw_commits(struct tog_view *view)
> >  		entry = TAILQ_NEXT(entry, entry);
> >  	}
> > 
> > -	view_vborder(view);
> > +	view_border(view);
> >  done:
> >  	free(id_str);
> >  	free(refs_str);
> > @@ -1997,6 +2131,19 @@ trigger_log_thread(struct tog_view *view, int wait)
> >  }
> > 
> >  static const struct got_error *
> > +request_log_commits(struct tog_view *view)
> > +{
> > +	struct tog_log_view_state	*state = &view->state.log;
> > +	const struct got_error		*err = NULL;
> > +
> > +	state->thread_args.commits_needed = view->nscrolled;
> > +	err = trigger_log_thread(view, 1);
> > +	view->nscrolled = 0;
> > +
> > +	return err;
> > +}
> > +
> > +static const struct got_error *
> >  log_scroll_down(struct tog_view *view, int maxscroll)
> >  {
> >  	struct tog_log_view_state *s = &view->state.log;
> > @@ -2021,10 +2168,11 @@ log_scroll_down(struct tog_view *view, int maxscroll)
> > 
> >  	do {
> >  		pentry = TAILQ_NEXT(s->last_displayed_entry, entry);
> > -		if (pentry == NULL)
> > +		if (pentry == NULL && view->mode != TOG_VIEW_SPLIT_HRZN)
> >  			break;
> > 
> > -		s->last_displayed_entry = pentry;
> > +		s->last_displayed_entry = pentry ?
> > +		    pentry : s->last_displayed_entry;;
> > 
> >  		pentry = TAILQ_NEXT(s->first_displayed_entry, entry);
> >  		if (pentry == NULL)
> > @@ -2032,11 +2180,16 @@ log_scroll_down(struct tog_view *view, int maxscroll)
> >  		s->first_displayed_entry = pentry;
> >  	} while (++nscrolled < maxscroll);
> > 
> > +	if (view->mode == TOG_VIEW_SPLIT_HRZN)
> > +		view->nscrolled += nscrolled;
> > +	else
> > +		view->nscrolled = 0;
> > +
> >  	return err;
> >  }
> > 
> >  static const struct got_error *
> > -open_diff_view_for_commit(struct tog_view **new_view, int begin_x,
> > +open_diff_view_for_commit(struct tog_view **new_view, int begin_y, int begin_x,
> >      struct got_commit_object *commit, struct got_object_id *commit_id,
> >      struct tog_view *log_view, struct got_repository *repo)
> >  {
> > @@ -2044,7 +2197,7 @@ open_diff_view_for_commit(struct tog_view **new_view,
> >  	struct got_object_qid *parent_id;
> >  	struct tog_view *diff_view;
> > 
> > -	diff_view = view_open(0, 0, 0, begin_x, TOG_VIEW_DIFF);
> > +	diff_view = view_open(0, 0, begin_y, begin_x, TOG_VIEW_DIFF);
> >  	if (diff_view == NULL)
> >  		return got_error_from_errno("view_open");
> > 
> > @@ -2609,7 +2762,133 @@ show_log_view(struct tog_view *view)
> >  	return draw_commits(view);
> >  }
> > 
> > +static void
> > +log_move_cursor_up(struct tog_view *view, int page, int home)
> > +{
> > +	struct tog_log_view_state *s = &view->state.log;
> > +
> > +	if (s->selected_entry->idx == 0)
> > +		view->count = 0;
> > +	if (s->first_displayed_entry == NULL)
> > +		return;
> > +
> > +	if ((page && TAILQ_FIRST(&s->commits.head) == s->first_displayed_entry)
> > +	    || home)
> > +		s->selected = home ? 0 : MAX(0, s->selected - page - 1);
> > +
> > +	if (!page && !home && s->selected > 0)
> > +		--s->selected;
> > +	else
> > +		log_scroll_up(s, home ? s->commits.ncommits : MAX(page, 1));
> > +
> > +	select_commit(s);
> > +	return;
> > +}
> > +
> >  static const struct got_error *
> > +log_move_cursor_down(struct tog_view *view, int page)
> > +{
> > +	struct tog_log_view_state	*s = &view->state.log;
> > +	struct commit_queue_entry	*first;
> > +	const struct got_error		*err = NULL;
> > +
> > +	first = s->first_displayed_entry;
> > +	if (first == NULL) {
> > +		view->count = 0;
> > +		return err;
> 
> likewise, I'd say "return NULL" since we know there isn't an error.  it
> makes it clearer to the reader.

I agree. Will fix.

> > +	}
> > +
> > +	if (s->thread_args.log_complete &&
> > +	    s->selected_entry->idx >= s->commits.ncommits - 1)
> > +		return err;
> > +
> > +	if (!page) {
> > +		if (s->selected < MIN(view->nlines - 2,
> > +		    s->commits.ncommits - 1))
> > +			++s->selected;
> > +		else
> > +			err = log_scroll_down(view, 1);
> > +	} else if (s->thread_args.log_complete) {
> > +		if (s->last_displayed_entry->idx == s->commits.ncommits - 1)
> > +			s->selected += MIN(s->last_displayed_entry->idx -
> > +			    s->selected_entry->idx, page + 1);
> > +		else
> > +			err = log_scroll_down(view, MIN(page,
> > +			    s->commits.ncommits - s->selected_entry->idx - 1));
> > +		s->selected = MIN(view->nlines - 2, s->commits.ncommits - 1);
> > +	} else {
> > +		err = log_scroll_down(view, page);
> > +		if (err)
> > +			return err;
> > +		if (first == s->first_displayed_entry && s->selected <
> > +		    MIN(view->nlines - 2, s->commits.ncommits - 1)) {
> > +			s->selected = MIN(s->commits.ncommits - 1, page);
> > +		}
> > +	}
> > +	if (err)
> > +		return err;
> > +
> > +	/*
> > +	 * We might necessarily overshoot in horizontal
> > +	 * splits; if so, select the last displayed commit.
> > +	 */
> > +	s->selected = MIN(s->selected,
> > +	    s->last_displayed_entry->idx - s->first_displayed_entry->idx);
> > +
> > +	select_commit(s);
> > +
> > +	if (s->thread_args.log_complete &&
> > +	    s->selected_entry->idx == s->commits.ncommits - 1)
> > +		view->count = 0;
> > +
> > +	return err;
> > +}
> > +
> > +/*
> > + * Get splitscreen dimensions. If TOG_VIEW_SPLIT_MODE isn't set and COLS > 119,
> > + * open vertical split, else open horizontal split.  If set to 'v' or 'h',
> > + * assign start column or line of the new split to *x or *y, respectively, and
> > + * assign view mode to view->mode.
> > + */
> > +static const struct got_error *
> > +view_get_split(struct tog_view *view, int *y, int *x)
> > +{
> > +	const struct got_error	*err = NULL;
> > +	char			*mode;
> > +
> > +	mode = getenv("TOG_VIEW_SPLIT_MODE");
> > +
> > +	if (!mode || mode[0] != 'h') {
> > +		view->mode = TOG_VIEW_SPLIT_VERT;
> > +		*x = view_split_begin_x(view->begin_x);
> > +	}
> > +
> > +	if (!*x && (!mode || mode[0] != 'v')) {
> > +		view->mode = TOG_VIEW_SPLIT_HRZN;
> > +		err = view_split_begin_y(y, view->lines);
> > +	}
> > +
> > +	return err;
> > +}
> > +
> > +/* 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)

> > +	view->nlines = y - 1;  /* border */
> > +	err = offset_selection_down(view);
> > +
> > +	return err;
> > +}
> > +
> > +static const struct got_error *
> >  input_log_view(struct tog_view **new_view, struct tog_view *view, int ch)
> >  {
> >  	const struct got_error *err = NULL;
> > @@ -2617,17 +2896,14 @@ input_log_view(struct tog_view **new_view, struct tog_
> >  	struct tog_view *diff_view = NULL, *tree_view = NULL;
> >  	struct tog_view *ref_view = NULL;
> >  	struct commit_queue_entry *entry;
> > -	int begin_x = 0, n, nscroll = view->nlines - 1;
> > +	int begin_x = 0, begin_y = 0, n, nscroll = view->nlines - 1;
> > 
> >  	if (s->thread_args.load_all) {
> >  		if (ch == KEY_BACKSPACE)
> >  			s->thread_args.load_all = 0;
> >  		else if (s->thread_args.log_complete) {
> >  			s->thread_args.load_all = 0;
> > -			log_scroll_down(view, s->commits.ncommits);
> > -			s->selected = MIN(view->nlines - 2,
> > -			    s->commits.ncommits - 1);
> > -			select_commit(s);
> > +			err = log_move_cursor_down(view, s->commits.ncommits);
> 
> for one, I like the semplifications log_move_cursor_down/up brings here.
> 
> if you're concerned about the size of the diff you could factor this
> change out and commit it before.  At least I would OK such a diff :)

I did intend to commit this in stages if ok. Thanks, Omar :)

> >  		}
> >  		return NULL;
> >  	}
> > @@ -2661,21 +2937,11 @@ input_log_view(struct tog_view **new_view, struct tog_
> >  	case '<':
> >  	case ',':
> >  	case CTRL('p'):
> > -		if (s->selected_entry->idx == 0)
> > -			view->count = 0;
> > -		if (s->first_displayed_entry == NULL)
> > -			break;
> > -		if (s->selected > 0)
> > -			s->selected--;
> > -		else
> > -			log_scroll_up(s, 1);
> > -		select_commit(s);
> > +		log_move_cursor_up(view, 0, 0);
> >  		break;
> >  	case 'g':
> >  	case KEY_HOME:
> > -		s->selected = 0;
> > -		s->first_displayed_entry = TAILQ_FIRST(&s->commits.head);
> > -		select_commit(s);
> > +		log_move_cursor_up(view, 0, 1);
> >  		view->count = 0;
> >  		break;
> >  	case CTRL('u'):
> > @@ -2685,35 +2951,14 @@ input_log_view(struct tog_view **new_view, struct tog_
> >  	case KEY_PPAGE:
> >  	case CTRL('b'):
> >  	case 'b':
> > -		if (s->first_displayed_entry == NULL)
> > -			break;
> > -		if (TAILQ_FIRST(&s->commits.head) == s->first_displayed_entry)
> > -			s->selected = MAX(0, s->selected - nscroll - 1);
> > -		else
> > -			log_scroll_up(s, nscroll);
> > -		select_commit(s);
> > -		if (s->selected_entry->idx == 0)
> > -			view->count = 0;
> > +		log_move_cursor_up(view, nscroll, 0);
> >  		break;
> >  	case 'j':
> >  	case KEY_DOWN:
> >  	case '>':
> >  	case '.':
> >  	case CTRL('n'):
> > -		if (s->first_displayed_entry == NULL)
> > -			break;
> > -		if (s->selected < MIN(view->nlines - 2,
> > -		    s->commits.ncommits - 1))
> > -			s->selected++;
> > -		else {
> > -			err = log_scroll_down(view, 1);
> > -			if (err)
> > -				break;
> > -		}
> > -		select_commit(s);
> > -		if (s->thread_args.log_complete &&
> > -		    s->selected_entry->idx == s->commits.ncommits - 1)
> > -			view->count = 0;
> > +		err = log_move_cursor_down(view, 0);
> >  		break;
> >  	case 'G':
> >  	case KEY_END: {
> > @@ -2745,29 +2990,9 @@ input_log_view(struct tog_view **new_view, struct tog_
> >  	case KEY_NPAGE:
> >  	case CTRL('f'):
> >  	case 'f':
> > -	case ' ': {
> > -		struct commit_queue_entry *first;
> > -		first = s->first_displayed_entry;
> > -		if (first == NULL) {
> > -			view->count = 0;
> > -			break;
> > -		}
> > -		err = log_scroll_down(view, nscroll);
> > -		if (err)
> > -			break;
> > -		if (first == s->first_displayed_entry &&
> > -		    s->selected < MIN(view->nlines - 2,
> > -		    s->commits.ncommits - 1)) {
> > -			/* can't scroll further down */
> > -			s->selected += MIN(s->last_displayed_entry->idx -
> > -			    s->selected_entry->idx, nscroll + 1);
> > -		}
> > -		select_commit(s);
> > -		if (s->thread_args.log_complete &&
> > -		    s->selected_entry->idx == s->commits.ncommits - 1)
> > -			view->count = 0;
> > +	case ' ':
> > +		err = log_move_cursor_down(view, nscroll);
> >  		break;
> > -	}
> >  	case KEY_RESIZE:
> >  		if (s->selected > view->nlines - 2)
> >  			s->selected = view->nlines - 2;
> > @@ -2782,30 +3007,45 @@ input_log_view(struct tog_view **new_view, struct tog_
> >  		}
> >  		break;
> >  	case KEY_ENTER:
> > -	case '\r':
> > +	case '\r': {
> >  		view->count = 0;
> >  		if (s->selected_entry == NULL)
> >  			break;
> > -		if (view_is_parent_view(view))
> > -			begin_x = view_split_begin_x(view->begin_x);
> > -		err = open_diff_view_for_commit(&diff_view, begin_x,
> > +
> > +		/* get dimensions--don't split till initialisation succeeds */
> > +		if (view_is_parent_view(view)) {
> > +			err = view_get_split(view, &begin_y, &begin_x);
> > +			if (err)
> > +				break;
> > +		}
> > +
> > +		err = open_diff_view_for_commit(&diff_view, begin_y, begin_x,
> >  		    s->selected_entry->commit, s->selected_entry->id,
> >  		    view, s->repo);
> >  		if (err)
> >  			break;
> > +
> > +		if (view->mode == TOG_VIEW_SPLIT_HRZN) {  /* safe to split */
> > +			err = view_init_hsplit(view, begin_y);
> > +			if (err)
> > +				break;
> > +		}
> > +
> >  		view->focussed = 0;
> >  		diff_view->focussed = 1;
> > +		diff_view->mode = view->mode;
> > +		diff_view->nlines = view->lines - begin_y;
> > +
> >  		if (view_is_parent_view(view)) {
> >  			err = view_close_child(view);
> >  			if (err)
> >  				return err;
> > -			err = view_set_child(view, diff_view);
> > -			if (err)
> > -				return err;
> > +			view_set_child(view, diff_view);
> >  			view->focus_child = 1;
> >  		} else
> >  			*new_view = diff_view;
> >  		break;
> > +	}
> >  	case 't':
> >  		view->count = 0;
> >  		if (s->selected_entry == NULL)
> > @@ -2823,9 +3063,7 @@ input_log_view(struct tog_view **new_view, struct tog_
> >  			err = view_close_child(view);
> >  			if (err)
> >  				return err;
> > -			err = view_set_child(view, tree_view);
> > -			if (err)
> > -				return err;
> > +			view_set_child(view, tree_view);
> >  			view->focus_child = 1;
> >  		} else
> >  			*new_view = tree_view;
> > @@ -2913,9 +3151,7 @@ input_log_view(struct tog_view **new_view, struct tog_
> >  			err = view_close_child(view);
> >  			if (err)
> >  				return err;
> > -			err = view_set_child(view, ref_view);
> > -			if (err)
> > -				return err;
> > +			view_set_child(view, ref_view);
> >  			view->focus_child = 1;
> >  		} else
> >  			*new_view = ref_view;
> > @@ -3416,7 +3652,7 @@ draw_file(struct tog_view *view, const char *header)
> >  	else
> >  		s->last_displayed_line = s->first_displayed_line;
> > 
> > -	view_vborder(view);
> > +	view_border(view);
> > 
> >  	if (s->eof) {
> >  		while (nprinted < view->nlines) {
> > @@ -4596,7 +4832,7 @@ draw_blame(struct tog_view *view)
> >  	free(line);
> >  	s->last_displayed_line = lineno;
> > 
> > -	view_vborder(view);
> > +	view_border(view);
> > 
> >  	return NULL;
> >  }
> > @@ -5042,7 +5278,7 @@ show_blame_view(struct tog_view *view)
> > 
> >  	err = draw_blame(view);
> > 
> > -	view_vborder(view);
> > +	view_border(view);
> >  	return err;
> >  }
> > 
> > @@ -5052,7 +5288,7 @@ input_blame_view(struct tog_view **new_view, struct to
> >  	const struct got_error *err = NULL, *thread_err = NULL;
> >  	struct tog_view *diff_view;
> >  	struct tog_blame_view_state *s = &view->state.blame;
> > -	int begin_x = 0, nscroll = view->nlines - 2;
> > +	int begin_y = 0, begin_x = 0, nscroll = view->nlines - 2;
> > 
> >  	switch (ch) {
> >  	case '0':
> > @@ -5244,11 +5480,14 @@ input_blame_view(struct tog_view **new_view, struct to
> >  		err = got_object_open_as_commit(&commit, s->repo, id);
> >  		if (err)
> >  			break;
> > -		pid = STAILQ_FIRST(
> > -		    got_object_commit_get_parent_ids(commit));
> > -		if (view_is_parent_view(view))
> > -		    begin_x = view_split_begin_x(view->begin_x);
> > -		diff_view = view_open(0, 0, 0, begin_x, TOG_VIEW_DIFF);
> > +		pid = STAILQ_FIRST(got_object_commit_get_parent_ids(commit));
> > +		if (view_is_parent_view(view)) {
> > +			err = view_get_split(view, &begin_y, &begin_x);
> > +			if (err)
> > +				break;
> > +		}
> > +
> > +		diff_view = view_open(0, 0, begin_y, begin_x, TOG_VIEW_DIFF);
> >  		if (diff_view == NULL) {
> >  			got_object_commit_close(commit);
> >  			err = got_error_from_errno("view_open");
> > @@ -5261,15 +5500,21 @@ input_blame_view(struct tog_view **new_view, struct to
> >  			view_close(diff_view);
> >  			break;
> >  		}
> > +		if (view->mode == TOG_VIEW_SPLIT_HRZN) {
> > +			err = view_init_hsplit(view, begin_y);
> > +			if (err)
> > +				break;
> > +		}
> > +
> >  		view->focussed = 0;
> >  		diff_view->focussed = 1;
> > +		diff_view->mode = view->mode;
> > +		diff_view->nlines = view->lines - begin_y;
> >  		if (view_is_parent_view(view)) {
> >  			err = view_close_child(view);
> >  			if (err)
> >  				break;
> > -			err = view_set_child(view, diff_view);
> > -			if (err)
> > -				break;
> > +			view_set_child(view, diff_view);
> >  			view->focus_child = 1;
> >  		} else
> >  			*new_view = diff_view;
> > @@ -5639,9 +5884,10 @@ tree_scroll_up(struct tog_tree_view_state *s, int maxs
> >  	}
> >  }
> > 
> > -static void
> > -tree_scroll_down(struct tog_tree_view_state *s, int maxscroll)
> > +static const struct got_error *
> > +tree_scroll_down(struct tog_view *view, int maxscroll)
> >  {
> > +	struct tog_tree_view_state *s = &view->state.tree;
> >  	struct got_tree_entry *next, *last;
> >  	int n = 0;
> > 
> > @@ -5652,13 +5898,16 @@ tree_scroll_down(struct tog_tree_view_state *s, int ma
> >  		next = got_object_tree_get_first_entry(s->tree);
> > 
> >  	last = s->last_displayed_entry;
> > -	while (next && last && n++ < maxscroll) {
> > -		last = got_tree_entry_get_next(s->tree, last);
> > -		if (last) {
> > +	while (next && n++ < maxscroll) {
> > +		if (last)
> > +			last = got_tree_entry_get_next(s->tree, last);
> > +		if (last || (view->mode == TOG_VIEW_SPLIT_HRZN && next)) {
> >  			s->first_displayed_entry = next;
> >  			next = got_tree_entry_get_next(s->tree, next);
> >  		}
> >  	}
> > +
> > +	return NULL;
> >  }
> > 
> >  static const struct got_error *
> > @@ -5708,7 +5957,7 @@ done:
> >  }
> > 
> >  static const struct got_error *
> > -blame_tree_entry(struct tog_view **new_view, int begin_x,
> > +blame_tree_entry(struct tog_view **new_view, int begin_y, int begin_x,
> >      struct got_tree_entry *te, struct tog_parent_trees *parents,
> >      struct got_object_id *commit_id, struct got_repository *repo)
> >  {
> > @@ -5722,7 +5971,7 @@ blame_tree_entry(struct tog_view **new_view, int begin
> >  	if (err)
> >  		return err;
> > 
> > -	blame_view = view_open(0, 0, 0, begin_x, TOG_VIEW_BLAME);
> > +	blame_view = view_open(0, 0, begin_y, begin_x, TOG_VIEW_BLAME);
> >  	if (blame_view == NULL) {
> >  		err = got_error_from_errno("view_open");
> >  		goto done;
> > @@ -5987,7 +6236,7 @@ show_tree_view(struct tog_view *view)
> >  	err = draw_tree_entries(view, parent_path);
> >  	free(parent_path);
> > 
> > -	view_vborder(view);
> > +	view_border(view);
> >  	return err;
> >  }
> > 
> > @@ -6018,9 +6267,7 @@ input_tree_view(struct tog_view **new_view, struct tog
> >  			err = view_close_child(view);
> >  			if (err)
> >  				return err;
> > -			err = view_set_child(view, log_view);
> > -			if (err)
> > -				return err;
> > +			view_set_child(view, log_view);
> >  			view->focus_child = 1;
> >  		} else
> >  			*new_view = log_view;
> > @@ -6044,9 +6291,7 @@ input_tree_view(struct tog_view **new_view, struct tog
> >  			err = view_close_child(view);
> >  			if (err)
> >  				return err;
> > -			err = view_set_child(view, ref_view);
> > -			if (err)
> > -				return err;
> > +			view_set_child(view, ref_view);
> >  			view->focus_child = 1;
> >  		} else
> >  			*new_view = ref_view;
> > @@ -6127,7 +6372,7 @@ input_tree_view(struct tog_view **new_view, struct tog
> >  			view->count = 0;
> >  			break;
> >  		}
> > -		tree_scroll_down(s, 1);
> > +		tree_scroll_down(view, 1);
> >  		break;
> >  	case CTRL('d'):
> >  	case 'd':
> > @@ -6147,7 +6392,7 @@ input_tree_view(struct tog_view **new_view, struct tog
> >  				view->count = 0;
> >  			break;
> >  		}
> > -		tree_scroll_down(s, nscroll);
> > +		tree_scroll_down(view, nscroll);
> >  		break;
> >  	case KEY_ENTER:
> >  	case '\r':
> > @@ -6169,6 +6414,8 @@ input_tree_view(struct tog_view **new_view, struct tog
> >  			s->selected_entry =
> >  			    parent->selected_entry;
> >  			s->selected = parent->selected;
> > +			if (s->selected > view->nlines - 3)
> > +				offset_selection_down(view);
> >  			free(parent);
> >  		} else if (S_ISDIR(got_tree_entry_get_mode(
> >  		    s->selected_entry))) {
> > @@ -6186,24 +6433,36 @@ input_tree_view(struct tog_view **new_view, struct tog
> >  		} else if (S_ISREG(got_tree_entry_get_mode(
> >  		    s->selected_entry))) {
> >  			struct tog_view *blame_view;
> > -			int begin_x = view_is_parent_view(view) ?
> > -			    view_split_begin_x(view->begin_x) : 0;
> > +			int begin_x = 0, begin_y = 0;
> > 
> > -			err = blame_tree_entry(&blame_view, begin_x,
> > +			if (view_is_parent_view(view)) {
> > +				err = view_get_split(view, &begin_y, &begin_x);
> > +				if (err)
> > +					break;
> > +			}
> > +
> > +			err = blame_tree_entry(&blame_view, begin_y, begin_x,
> >  			    s->selected_entry, &s->parents,
> >  			    s->commit_id, s->repo);
> >  			if (err)
> >  				break;
> > +
> > +			if (view->mode == TOG_VIEW_SPLIT_HRZN) {
> > +				err = view_init_hsplit(view, begin_y);
> > +				if (err)
> > +					break;
> > +			}
> > +
> >  			view->count = 0;
> >  			view->focussed = 0;
> >  			blame_view->focussed = 1;
> > +			blame_view->mode = view->mode;
> > +			blame_view->nlines = view->lines - begin_y;
> >  			if (view_is_parent_view(view)) {
> >  				err = view_close_child(view);
> >  				if (err)
> >  					return err;
> > -				err = view_set_child(view, blame_view);
> > -				if (err)
> > -					return err;
> > +				view_set_child(view, blame_view);
> >  				view->focus_child = 1;
> >  			} else
> >  				*new_view = blame_view;
> > @@ -6544,7 +6803,7 @@ done:
> >  }
> > 
> >  static const struct got_error *
> > -log_ref_entry(struct tog_view **new_view, int begin_x,
> > +log_ref_entry(struct tog_view **new_view, int begin_y, int begin_x,
> >      struct tog_reflist_entry *re, struct got_repository *repo)
> >  {
> >  	struct tog_view *log_view;
> > @@ -6561,7 +6820,7 @@ log_ref_entry(struct tog_view **new_view, int begin_x,
> >  			return NULL;
> >  	}
> > 
> > -	log_view = view_open(0, 0, 0, begin_x, TOG_VIEW_LOG);
> > +	log_view = view_open(0, 0, begin_y, begin_x, TOG_VIEW_LOG);
> >  	if (log_view == NULL) {
> >  		err = got_error_from_errno("view_open");
> >  		goto done;
> > @@ -6596,9 +6855,10 @@ ref_scroll_up(struct tog_ref_view_state *s, int maxscr
> >  	}
> >  }
> > 
> > -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.

> >  }
> > 
> >  static const struct got_error *
> > @@ -6847,7 +7110,7 @@ show_ref_view(struct tog_view *view)
> >  		re = TAILQ_NEXT(re, entry);
> >  	}
> > 
> > -	view_vborder(view);
> > +	view_border(view);
> >  	return err;
> >  }
> > 
> > @@ -6893,7 +7156,7 @@ input_ref_view(struct tog_view **new_view, struct tog_
> >  	struct tog_ref_view_state *s = &view->state.ref;
> >  	struct tog_view *log_view, *tree_view;
> >  	struct tog_reflist_entry *re;
> > -	int begin_x = 0, n, nscroll = view->nlines - 1;
> > +	int begin_y = 0, begin_x = 0, n, nscroll = view->nlines - 1;
> > 
> >  	switch (ch) {
> >  	case 'i':
> > @@ -6925,19 +7188,32 @@ input_ref_view(struct tog_view **new_view, struct tog_
> >  		view->count = 0;
> >  		if (!s->selected_entry)
> >  			break;
> > -		if (view_is_parent_view(view))
> > -			begin_x = view_split_begin_x(view->begin_x);
> > -		err = log_ref_entry(&log_view, begin_x, s->selected_entry,
> > -		    s->repo);
> > +		if (view_is_parent_view(view)) {
> > +			err = view_get_split(view, &begin_y, &begin_x);
> > +			if (err)
> > +				break;
> > +		}
> > +
> > +		err = log_ref_entry(&log_view, begin_y, begin_x,
> > +		    s->selected_entry, s->repo);
> > +		if (err)
> > +			break;
> > +
> > +		if (view->mode == TOG_VIEW_SPLIT_HRZN) {
> > +			err = view_init_hsplit(view, begin_y);
> > +			if (err)
> > +				break;
> > +		}
> > +
> >  		view->focussed = 0;
> >  		log_view->focussed = 1;
> > +		log_view->mode = view->mode;
> > +		log_view->nlines = view->lines - begin_y;
> >  		if (view_is_parent_view(view)) {
> >  			err = view_close_child(view);
> >  			if (err)
> >  				return err;
> > -			err = view_set_child(view, log_view);
> > -			if (err)
> > -				return err;
> > +			view_set_child(view, log_view);
> >  			view->focus_child = 1;
> >  		} else
> >  			*new_view = log_view;
> > @@ -6958,9 +7234,7 @@ input_ref_view(struct tog_view **new_view, struct tog_
> >  			err = view_close_child(view);
> >  			if (err)
> >  				return err;
> > -			err = view_set_child(view, tree_view);
> > -			if (err)
> > -				return err;
> > +			view_set_child(view, tree_view);
> >  			view->focus_child = 1;
> >  		} else
> >  			*new_view = tree_view;
> > @@ -7021,7 +7295,7 @@ input_ref_view(struct tog_view **new_view, struct tog_
> >  			view->count = 0;
> >  			break;
> >  		}
> > -		ref_scroll_down(s, 1);
> > +		ref_scroll_down(view, 1);
> >  		break;
> >  	case CTRL('d'):
> >  	case 'd':
> > @@ -7041,7 +7315,7 @@ input_ref_view(struct tog_view **new_view, struct tog_
> >  			view->count = 0;
> >  			break;
> >  		}
> > -		ref_scroll_down(s, nscroll);
> > +		ref_scroll_down(view, nscroll);
> >  		break;
> >  	case CTRL('l'):
> >  		view->count = 0;
> > @@ -7174,7 +7448,117 @@ done:
> >  	return error;
> >  }
> > 
> > +/*
> > + * If view was scrolled down to move the selected line into view when opening a
> > + * horizontal split, scroll back up when closing the split/toggling fullscreen.
> > + */
> >  static void
> > +offset_selection_up(struct tog_view *view)
> > +{
> > +	switch (view->type) {
> > +	case TOG_VIEW_BLAME: {
> > +		struct tog_blame_view_state *s = &view->state.blame;
> > +		if (s->first_displayed_line == 1) {
> > +			s->selected_line = MAX(s->selected_line - view->offset,
> > +			    1);
> > +			break;
> > +		}
> > +		if (s->first_displayed_line > view->offset)
> > +			s->first_displayed_line -= view->offset;
> > +		else
> > +			s->first_displayed_line = 1;
> > +		s->selected_line += view->offset;
> > +		break;
> > +	}
> 
> I'd avoid the braces when we're not introducing variables.

Too much copypasta from offset_selection_down() :)
I'll remove these :)

> > +	case TOG_VIEW_LOG: {
> > +		log_scroll_up(&view->state.log, view->offset);
> > +		view->state.log.selected += view->offset;
> > +		break;
> > +	}
> > +	case TOG_VIEW_REF: {
> > +		ref_scroll_up(&view->state.ref, view->offset);
> > +		view->state.ref.selected += view->offset;
> > +		break;
> > +	}
> > +	case TOG_VIEW_TREE: {
> > +		tree_scroll_up(&view->state.tree, view->offset);
> > +		view->state.tree.selected += view->offset;
> > +		break;
> > +	}
> > +	default:
> > +		break;
> > +	}
> > +
> > +	view->offset = 0;
> > +	return;
> > +}
> > +
> > +/*
> > + * If the selected line is in the section of screen covered by the bottom split,
> > + * scroll down offset lines to move it into view and index its new position.
> > + */
> > +static const struct got_error *
> > +offset_selection_down(struct tog_view *view)
> > +{
> > +	const struct got_error	*err = NULL;
> > +	const struct got_error	*(*scrolld)(struct tog_view *, int);
> > +	int			*selected = NULL;
> > +	int			 header, offset;
> > +
> > +	switch (view->type) {
> > +	case TOG_VIEW_BLAME: {
> > +		struct tog_blame_view_state *s = &view->state.blame;
> > +		header = 2;
> > +		scrolld = NULL;
> > +		if (s->selected_line > view->nlines - header) {
> > +			offset = ABS(view->nlines - s->selected_line - header);
> > +			s->first_displayed_line += offset;
> > +			s->selected_line -= offset;
> > +			view->offset = offset;
> > +		}
> > +		break;
> > +	}
> > +	case TOG_VIEW_LOG: {
> > +		struct tog_log_view_state *s = &view->state.log;
> > +		scrolld = &log_scroll_down;
> > +		header = 2;
> > +		selected = &s->selected;
> > +		break;
> > +	}
> > +	case TOG_VIEW_REF: {
> > +		struct tog_ref_view_state *s = &view->state.ref;
> > +		scrolld = &ref_scroll_down;
> > +		header = 2;
> > +		selected = &s->selected;
> > +		break;
> > +	}
> > +	case TOG_VIEW_TREE: {
> > +		struct tog_tree_view_state *s = &view->state.tree;
> > +		scrolld = &tree_scroll_down;
> > +		header = 4;
> > +		selected = &s->selected;
> > +		break;
> > +	}
> > +	default:
> > +		selected = NULL;
> > +		scrolld = NULL;
> > +		header = 0;
> > +		break;
> > +	}
> > +
> > +	if (selected && *selected > view->nlines - header) {
> > +		offset = ABS(view->nlines - *selected - header);
> > +		view->offset = offset;
> > +		if (scrolld && offset) {
> > +			err = scrolld(view, offset);
> > +			*selected -= offset;
> > +		}
> > +	}
> > +
> > +	return err;
> > +}
> > +
> > +static void
> >  list_commands(FILE *fp)
> >  {
> >  	size_t i;
> 
> 

-- 
Mark Jamsek <fnc.bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68