On Thu, Sep 15, 2022 at 11:03:48PM +1000, Mark Jamsek wrote:
> I tried a few different things, most notably scroll bars and stsp's
> suggestion. The scroll bars were interesting but ultimately look out of
> place in a TUI app, imo. So I settled on a highlighted comment and
> percentage indicator: "scroll down for more...[mn%]"
> 
> I think this both provides a good cue to the user and fits the general
> tog aesthetic. If you'd like to see the scroll bar version, let me know
> and I'll shoot through the diff.
> 
> As previously mentioned, I'd like to add backwards search via the '?'
> key map to tog, so I've only documented H and F1 but runtime help is
> still accessible via ? for now.

ok stsp@

This is a great start. I have some comments below, which can be dealt
with later if/when you feel like it.

>  static const struct got_error *
> -search_next_diff_view(struct tog_view *view)
> +search_set_view(struct tog_view *view, FILE **f, off_t **line_offsets,
> +    size_t *nlines, int **first, int **last, int **match, int **selected)
>  {
> -	struct tog_diff_view_state *s = &view->state.diff;
> +	*f = NULL;
> +	*first = *last = *match = *selected = NULL;
> +	*line_offsets = NULL;
> +
> +	switch (view->type) {
> +	case (TOG_VIEW_DIFF): {
> +		struct tog_diff_view_state *s = &view->state.diff;
> +
> +		*f = s->f;
> +		*nlines = s->nlines;
> +		*match = &s->matched_line;
> +		*first = &s->first_displayed_line;
> +		*last = &s->last_displayed_line;
> +		*selected = &s->selected_line;
> +		break;
> +	}
> +	case (TOG_VIEW_BLAME): {
> +		struct tog_blame_view_state *s = &view->state.blame;
> +
> +		*f = s->blame.f;
> +		*nlines = s->blame.nlines;
> +		*line_offsets = s->blame.line_offsets;
> +		*match = &s->matched_line;
> +		*first = &s->first_displayed_line;
> +		*last = &s->last_displayed_line;
> +		*selected = &s->selected_line;
> +		break;
> +	}
> +	case (TOG_VIEW_HELP): {
> +		struct tog_help_view_state *s = &view->state.help;
> +
> +		*f = s->f;
> +		*nlines = s->nlines;
> +		*line_offsets = s->line_offsets;
> +		*match = &s->matched_line;
> +		*first = &s->first_displayed_line;
> +		*last = &s->last_displayed_line;
> +		*selected = &s->selected_line;
> +		break;
> +	}
> +	default:
> +		return got_error_msg(GOT_ERR_NOT_IMPL,
> +		    "view search not supported");
> +	}
> +
> +	return NULL;
> +}

The above function could also be modelled as a new function pointer in
struct tog_view, so the different cases above could be called individually.
This is matter of taste, though, and just cosmetic.

> +/*
> + * Write keymap section headers, keys, and key info in km to f.
> + * Save line offset to *off. If terminal has UTF8 encoding enabled,
> + * wrap control and symbolic keys in guillemets, else use <>.
> + * For example (top=UTF8, bottom=ASCII):
> + * Global
> + *   k ❬C-p❭ ❬Up❭               Move cursor or page up one line
> + * Global
> + *   k <C-p> <Up>               Move cursor or page up one line
> + */
> +static const struct got_error *
> +format_help_line(off_t *off, FILE *f, const struct tog_key_map *km, int width)
> +{
> +	int n, len = width;
> +
> +	if (km->keys) {
> +		char	*t0, *t, *k;
> +		int	 cs, s, first = 1;
> +
> +		cs = got_locale_is_utf8();
> +
> +		t = t0 = strdup(km->keys);
> +		if (t0 == NULL)
> +			return got_error_from_errno("strdup");
> +
> +		len = strlen(km->keys);
> +		while ((k = strsep(&t, " ")) != NULL) {
> +			s = strlen(k) > 1;  /* control or symbolic key */
> +			n = fprintf(f, "%s%s%s%s%s", first ? "  " : "",
> +			    cs && s ? "❬" : s ? "<" : "", k,
> +			    cs && s ? "❭" : s ? ">" : "", t ? " " : "");

The above embeds UTF-8 characters into the source code. This means tog.c
can only be viewed and edited with an editor that is aware of UTF-8, and
there are people among OpenBSD developers who run in the C locale.
So making UTF-8 a requirement is not ideal.

For example, tmux deals with this by using lookup-tables such as the
one below:

(from usr.bin/tmux/tty-acs.c)
/* UTF-8 heavy borders. */
static const struct utf8_data tty_acs_heavy_borders_list[] = {
	{ "", 0, 0, 0 },
	{ "\342\224\203", 0, 3, 1 }, /* U+2503 */
	{ "\342\224\201", 0, 3, 1 }, /* U+2501 */
	{ "\342\224\217", 0, 3, 1 }, /* U+250F */
	{ "\342\224\223", 0, 3, 1 }, /* U+2513 */
	{ "\342\224\227", 0, 3, 1 }, /* U+2517 */
	{ "\342\224\233", 0, 3, 1 }, /* U+251B */
	{ "\342\224\263", 0, 3, 1 }, /* U+2533 */
	{ "\342\224\273", 0, 3, 1 }, /* U+253B */
	{ "\342\224\243", 0, 3, 1 }, /* U+2523 */
	{ "\342\224\253", 0, 3, 1 }, /* U+252B */
	{ "\342\225\213", 0, 3, 1 }, /* U+254B */
	{ "\302\267",	  0, 2, 1 }  /* U+00B7 */
};

We should probably be using a similar approach to render "❬" and "❭".

> @@ -8376,6 +8985,14 @@ view_dispatch_request(struct tog_view **new_view, stru
>  		if (err)
>  			view_close(*new_view);
>  		break;
> +	case TOG_VIEW_HELP:
> +		*new_view = view_open(0, 0, y, x, TOG_VIEW_HELP);
> +		if (*new_view == NULL)
> +			return got_error_from_errno("view_open");
> +		err = open_help_view(*new_view, view);
> +		if (err)
> +			view_close(*new_view);
> +		break;

Because the help view is implemented as a normal view, tog's parent/child
view behaviour determines where a new help view appears. For example,
when in horizontal split mode, the help view appears either full-screen
or in a split-screen, depending on which view has focus when the help view
is opened. I would expect help text to always appear in the same frame as
the view which is being documented. This would likely require some further
refactoring of the way new views are opened?